[PATCH] lib/util: fix timespec normalization

[Philipp Gesang]

Hi Volker,

-<| Quoting Philipp Gesang <philipp.gesang at intra2net.com>, on Wednesday, 2019-02-13 09:50:22 AM |>-
> -<| Quoting Philipp Gesang <philipp.gesang at intra2net.com>, on Friday, 2019-01-18 04:46:46 PM |>-
> > -<| Quoting Volker Lendecke via samba-technical <Volker.Lendecke at SerNet.DE>, on Friday, 2019-01-18 11:36:11 AM |>-
> > > 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.
> > 
> > Ok.
> > 
> > > > +	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.
> > 
> > I’ve gone with saturating the values instead of aborting outright.
> > 
> > > > +	/* if the ns part was positive or muliple of -1000000000, we're done */
> > > > +	if (ts->tv_nsec > 0 || dres.rem == 0) return;
> > > 
> > > { } please :-)
> > 
> > No problem ;)
> > 
> > New patch attached.
> 
> Ping.
> 
> The current patch is attached which contains a minor change
> (avoid C99 style loop variable declaration) and also removes a
> commented debug statement.

ping++

Best regards,
Philipp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190329/bc35b36d/signature.sig>