On 05.06.2019 23:42, Robert Elz wrote: > Date: Wed, 5 Jun 2019 22:25:39 +0200 > From: Kamil Rytarowski <n54%gmx.com@localhost> > Message-ID: <05d25ffa-588b-464f-56c9-099fea3f3329%gmx.com@localhost> > > | Does this patch look good? > > Personally, I would hesitate to change any of the NTP related > code without an extremely good reason, and without a lot of > testing first (in all kinds of conditions). > > Further, I'd never do it without a thorough review of the code, > if you looked, you'd also see > > freq = (ntv->freq * 1000LL) >> 16; > > and > > ntv->ppsfreq = L_GINT((pps_freq / 1000LL) << 16); > > (and perhaps more) - if one of those is a shift of a negative number, > the others potentially are as well (not that shifts of negative numbers > bother me at all if the code author understood what is happening, which > I suspect that they did here.) > > kre > This code looks suspicious as kernl_ntptime.c in general looks like trying to prevent UB in shift operations... but not fully. The L_GINT() macro handles negative values... but someone (?) overlooked that the passed argument is shifted for negative values. In my patch, I'm assuming that this can happen legitimately (and it does happen) and I'm replacing it with a portable form. There is an option to keep the shift there but use casts: ntv->freq = L_GINT((int64_t)((uint64_t)(time_freq / 1000LL) << 16)); I can switch all the other places of this code in one go.
Attachment:
signature.asc
Description: OpenPGP digital signature