Keep poll and precision intervals inside a duration

[SashaMIT]

Summary

A response precision of -128 panics while building the Response. Negating that int8 overflows, and the shift count is negative.

A poll exponent of 34 wraps to a negative duration. Exponents that do not fit now use the nearest duration that does. A poll of 4 is still 16s. A precision of -1 is still 500ms. A precision of -29 is still 1ns.

Test plan

• go test -count=1 -run 'TestToIntervalBounds|TestOffline' .
• With the old shift, TestToIntervalBounds panics: negative shift amount
• A poll of 34 on the old shift is a negative duration

[beevik]

Thanks for the contribution.

I'm curious, are you aware of any NTP server implementations that produce precision or poll values caught by this new bounds check?

[beevik]

For consistency, let's use a minShift constant.

[beevik]

Actually, the more I think about it, the more I think this function shouldn't be allowed to return zero. From the client's perspective, they're interested in the poll interval or the precision. In neither case is an interval of zero particularly helpful.

Perhaps the function should be more like this:

func toInterval(t int8) time.Duration {
	const minShift = -29
	const maxShift = 33
	shift := min(maxShift, max(minShift, int(t)))
	switch {
	case shift > 0:
		return time.Duration(int64(time.Second) << shift)
	case shift < 0:
		return time.Duration(int64(time.Second) >> -shift)
	default:
		return time.Second
	}
}

[SashaMIT]

Thanks for taking a look.

I don't have a server that sends these. Precision and poll are signed 8-bit fields, so -128 and 34 fit in the packet. Negating a precision of -128 overflows an int8, and that panic happens while building the Response. A poll of 34 shifts time.Second far enough that the duration comes back negative. Values in the usual range are unchanged: a poll of 4 is still 16s, a precision of -1 is still 500ms, and a precision of -29 is still 1ns.