[PATCH] lib/util: fix timespec normalization

[Volker Lendecke]

On Thu, Jan 17, 2019 at 05:07:18PM +0100, Philipp Gesang wrote:
> -<| Quoting Volker Lendecke via samba-technical <Volker.Lendecke at SerNet.DE>, on Thursday, 2019-01-17 11:58:16 AM |>-
> > On Thu, Jan 17, 2019 at 11:40:48AM +0100, Philipp Gesang via samba-technical wrote:
> > > +
> > > +/****************************************************************************
> > > + Deal with nanoseconds overflow.
> > > +****************************************************************************/
> > > +
> > > +void normalize_timespec(struct timespec *ts)
> > > +{
> > > +	while (ts->tv_nsec >= NSEC_PER_SEC) {
> > > +		++ts->tv_sec;
> > > +		ts->tv_nsec -= NSEC_PER_SEC;
> > > +	}
> > > +	while (ts->tv_nsec < 0) {
> > > +		--ts->tv_sec;
> > > +		ts->tv_nsec += NSEC_PER_SEC;
> > > +	}
> > > +}
> > 
> > This looks a bit inefficient to be honest. Shouldn't that be faster
> > doing some modulo-magic?
> 
> Unless something is severely wrong, there shouldn’t be more than
> the sum of two valid tv_nsec in there so the loop is unlikely to
> run more than once. Also on 32 bit, it’ll run at most twice on
> account of tv_nsec being long.
> 
> Anyways, attached is a variant that uses lldiv instead.

Better, thanks!

I know it's tedious, but if we touch this lowlevel code we should get
the details right with our coding guidelines. Thus two nit-picks:

> +/****************************************************************************
> + Deal with nanoseconds overflow.
> +****************************************************************************/
> +
> +void normalize_timespec(struct timespec *ts)
> +{
> +	lldiv_t dres;
> +
> +	/* most likely case: nsec is valid */
> +	if ((unsigned long)ts->tv_nsec < NSEC_PER_SEC) return;

Please add { } even around a single statement to avoid something like
CVE-2014-1266.

> +	dres = lldiv(ts->tv_nsec, NSEC_PER_SEC);
> +	ts->tv_nsec = dres.rem;
> +	ts->tv_sec += dres.quot;

tv_sec is a signed number. We should check for overflow here. I know
this is a "can't happen", but you never know. Here we might just abort
if it flows over.

> +
> +	/* if the ns part was positive or muliple of -1000000000, we're done */
> +	if (ts->tv_nsec > 0 || dres.rem == 0) return;

{ } please :-)

Thanks,

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: 0551-370000-0, mailto:kontakt at sernet.de
Gesch.F.: Dr. Johannes Loxen und Reinhild Jung
AG Göttingen: HR-B 2816 - http://www.sernet.de