Skip to content

Conversation

@pfree
Copy link

@pfree pfree commented Jan 3, 2019

The adjtimex system call returns type long (implied signed) for maxerror and esterror values.

See:

The expectation and casting to unsigned long results in UB when running ntptime. The below segments are output from ntptime run 3 times on the same host within ~10 seconds. Note the variance between "maximum error" and "estimated error" values of each run.

ntp_gettime() returns code 0 (OK) time dfd7f65c.52672fbc Wed, Jan 2 2019 21:47:56.321, (.321887282), maximum error 496 us, estimated error 18446744073709551612 us, TAI offset 37 ntp_adjtime() returns code 0 (OK) modes 0x0 (), offset 0.000 us, frequency 7.972 ppm, interval 1 s, maximum error 496 us, estimated error 18446744073709551612 us, status 0x2001 (PLL,NANO), time constant 6, precision 0.001 us, tolerance 500 ppm,

ntp_gettime() returns code 0 (OK) time dfd7f66a.e34a1274 Wed, Jan 2 2019 21:48:10.887, (.887849349), maximum error 18446744073709551615 us, estimated error 18446744073709551615 us, TAI offset 37 ntp_adjtime() returns code 0 (OK) modes 0x0 (), offset 0.000 us, frequency 7.668 ppm, interval 1 s, maximum error 18446744073709551615 us, estimated error 18446744073709551615 us, status 0x2001 (PLL,NANO), time constant 6, precision 0.001 us, tolerance 500 ppm,

ntp_gettime() returns code 0 (OK) time dfd7f678.9e5f0f5c Wed, Jan 2 2019 21:48:24.618, (.618638187), maximum error 504 us, estimated error 4 us, TAI offset 37 ntp_adjtime() returns code 0 (OK) modes 0x0 (), offset 0.000 us, frequency 7.028 ppm, interval 1 s, maximum error 504 us, estimated error 4 us, status 0x2001 (PLL,NANO), time constant 6, precision 0.001 us, tolerance 500 ppm,

@jnperlin
Copy link
Collaborator

jnperlin commented Jan 3, 2019

I think this is just a symptom for something more sinister going on. The error values are expected to be non-negative all the time, but there's a code path in NTPD that looks dubious to me right now. Still digging.

But a nice catch anyway.

Oh, and it's not UB when casting from long to unsigned long. Unless the 'U' stands for 'unexpected' instead of 'undefined' ;)

@pfree
Copy link
Author

pfree commented Feb 6, 2019

@jnperlin Do you have additional insight to this issue?

@jnperlin
Copy link
Collaborator

jnperlin commented Feb 6, 2019

A fix for this (and some more cleanup around STA_NANO/MOD_NANO) was filed in bugzilla (https://bugs.ntp.org/show_bug.cgi?id=3569) and is ready to merge.

You do not argue about kernel call parameters: The kernel is the reality. Since the error error estimates have to be set by some application, it is up to the implementation to set them properly. Setting the maximum error to a negative value is advanced nonsense, and the kernel should possibly catch it. It does not, so it's GIGO (garbage in, garbage out) when you set the errors to negative values.

But the type of the value is (signed) long, and treating them as unsigned is a gross mistake indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants