[PATCH] lib/util: fix timespec normalization
Philipp Gesang
philipp.gesang at intra2net.com
Wed Feb 13 08:50:22 UTC 2019
-<| 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.
Thank you,
Philipp
-------------- next part --------------
From 5ca7ada0d6007893dd80c7cbfe2e2f2d8a7d7f32 Mon Sep 17 00:00:00 2001
From: Philipp Gesang <philipp.gesang at intra2net.com>
Date: Thu, 17 Jan 2019 11:06:26 +0100
Subject: [PATCH v4 01/12] lib/util: fix timespec normalization
When fixing up timespec structs, negative values for the ns part
should be taken into account. Also, the range for a valid ns part
is [0, 1000000000), not [0, 1000000000].
Signed-off-by: Philipp Gesang <philipp.gesang at intra2net.com>
---
lib/util/tests/time.c | 53 +++++++++++++++++++++++++++++++++++++++++++
lib/util/time.c | 53 ++++++++++++++++++++++++++++++++++++-------
lib/util/time.h | 1 +
3 files changed, 99 insertions(+), 8 deletions(-)
diff --git a/lib/util/tests/time.c b/lib/util/tests/time.c
index fce0eef5e2e..041816dc3f6 100644
--- a/lib/util/tests/time.c
+++ b/lib/util/tests/time.c
@@ -82,6 +82,57 @@ static bool test_timestring(struct torture_context *tctx)
return true;
}
+static bool test_normalize_timespec(struct torture_context *tctx)
+{
+ int i;
+ const struct {
+ time_t in_s; long in_ns;
+ time_t out_s; long out_ns;
+ } data [] = {
+ { 0, 0, 0, 0 }
+ , { 1, 0, 1, 0 }
+ , { -1, 0, -1, 0 }
+ , { 0, 1000000000, 1, 0 }
+ , { 0, 2000000000, 2, 0 }
+ , { 0, 1000000001, 1, 1 }
+ , { 0, 2000000001, 2, 1 }
+ , { 0, -1000000000, -1, 0 }
+ , { 0, -2000000000, -2, 0 }
+ , { 0, -1000000001, -2, 999999999 }
+ , { 0, -2000000001, -3, 999999999 }
+ , { 0, -1, -1, 999999999 }
+ , { 1, -1, 0, 999999999 }
+ , { -1, -1, -2, 999999999 }
+ , { 0, 999999999, 0, 999999999 }
+ , { 0, 1999999999, 1, 999999999 }
+ , { 0, 2999999999, 2, 999999999 }
+ , { 0, -999999999, -1, 1 }
+ , { 0, -1999999999, -2, 1 }
+ , { 0, -2999999999, -3, 1 }
+ , { LONG_MAX, 1000000001, LONG_MAX, 999999999 } /* overflow */
+ , { LONG_MAX, 999999999, LONG_MAX, 999999999 } /* harmless */
+ , { LONG_MAX, -1, LONG_MAX-1, 999999999 } /* -1 */
+ , { LONG_MIN, -1000000001, LONG_MIN, 0 } /* overflow */
+ , { LONG_MIN, 0, LONG_MIN, 0 } /* harmless */
+ , { LONG_MIN, 1000000000, LONG_MIN+1, 0 } /* +1 */
+ };
+
+ for (i = 0; i < sizeof(data) / sizeof(data[0]); ++i) {
+ struct timespec ts = (struct timespec)
+ { .tv_sec = data[i].in_s
+ , .tv_nsec = data[i].in_ns };
+
+ normalize_timespec(&ts);
+
+ torture_assert_int_equal(tctx, ts.tv_sec, data[i].out_s,
+ "mismatch in tv_sec");
+ torture_assert_int_equal(tctx, ts.tv_nsec, data[i].out_ns,
+ "mismatch in tv_nsec");
+ }
+
+ return true;
+}
+
struct torture_suite *torture_local_util_time(TALLOC_CTX *mem_ctx)
{
struct torture_suite *suite = torture_suite_create(mem_ctx, "time");
@@ -92,6 +143,8 @@ struct torture_suite *torture_local_util_time(TALLOC_CTX *mem_ctx)
test_http_timestring);
torture_suite_add_simple_test(suite, "timestring",
test_timestring);
+ torture_suite_add_simple_test(suite, "normalize_timespec",
+ test_normalize_timespec);
return suite;
}
diff --git a/lib/util/time.c b/lib/util/time.c
index bd067f84e8e..9dfea1798b2 100644
--- a/lib/util/time.c
+++ b/lib/util/time.c
@@ -39,6 +39,7 @@
#endif
+#define NSEC_PER_SEC 1000000000
/**
External access to time_t_min and time_t_max.
@@ -88,10 +89,7 @@ _PUBLIC_ time_t time_mono(time_t *t)
time_t convert_timespec_to_time_t(struct timespec ts)
{
/* Ensure tv_nsec is less than 1sec. */
- while (ts.tv_nsec > 1000000000) {
- ts.tv_sec += 1;
- ts.tv_nsec -= 1000000000;
- }
+ normalize_timespec(&ts);
/* 1 ns == 1,000,000,000 - one thousand millionths of a second.
increment if it's greater than 500 millionth of a second. */
@@ -950,10 +948,7 @@ void round_timespec_to_usec(struct timespec *ts)
{
struct timeval tv = convert_timespec_to_timeval(*ts);
*ts = convert_timeval_to_timespec(tv);
- while (ts->tv_nsec > 1000000000) {
- ts->tv_sec += 1;
- ts->tv_nsec -= 1000000000;
- }
+ normalize_timespec(ts);
}
/****************************************************************************
@@ -982,3 +977,45 @@ _PUBLIC_ NTTIME unix_timespec_to_nt_time(struct timespec ts)
return d;
}
+
+/****************************************************************************
+ 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;
+ }
+
+ dres = lldiv(ts->tv_nsec, NSEC_PER_SEC);
+
+ /* if the operation would result in overflow, max out values and bail */
+ if (dres.quot > 0) {
+ if ((int64_t)LONG_MAX - dres.quot < ts->tv_sec) {
+ ts->tv_sec = LONG_MAX;
+ ts->tv_nsec = NSEC_PER_SEC - 1;
+ return;
+ }
+ } else {
+ if ((int64_t)LONG_MIN - dres.quot > ts->tv_sec) {
+ ts->tv_sec = LONG_MIN;
+ ts->tv_nsec = 0;
+ return;
+ }
+ }
+
+ ts->tv_nsec = dres.rem;
+ ts->tv_sec += dres.quot;
+
+ /* if the ns part was positive or a multiple of -1000000000, we're done */
+ if (ts->tv_nsec > 0 || dres.rem == 0) {
+ return;
+ }
+
+ ts->tv_nsec += NSEC_PER_SEC;
+ --ts->tv_sec;
+}
diff --git a/lib/util/time.h b/lib/util/time.h
index 1988b330576..2cfa1fa9039 100644
--- a/lib/util/time.h
+++ b/lib/util/time.h
@@ -329,5 +329,6 @@ int timespec_compare(const struct timespec *ts1, const struct timespec *ts2);
void round_timespec_to_sec(struct timespec *ts);
void round_timespec_to_usec(struct timespec *ts);
NTTIME unix_timespec_to_nt_time(struct timespec ts);
+void normalize_timespec(struct timespec *ts);
#endif /* _SAMBA_TIME_H_ */
--
2.20.1
-------------- 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/20190213/60c8cac2/signature.sig>
More information about the samba-technical
mailing list