[PATCH 0/4] Refactor timespec to microsecond conversion

[PATCH 0/4] Refactor timespec to microsecond conversion

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

An integer overflow fix for a timespec to microsecond conversion in
l_path_get_mtime() was previously committed but two overflows could
still occur: (1) the multiplication in the seconds (tv_sec) to
microsecond conversion could still exceed UINT64_MAX, and (2) addition
of the microseconds obtained from the nanoseconds (tv_nsec) member
could also result in a sum that exceeds UINT64_MAX.

This set of patches refactors the timespec to microsecond to a new
l_timespec_to_usecs() function that checks for overflow in both of the
cases mentioned above.  The l_path_get_mtime() function now uses
l_timespec_to_usecs() to address the remaining potential integer
overflows.  A potential integer overflow in the l_time_now() function
was corrected in a similar manner.


Ossama Othman (4):
  time: Add timespec to microseconds converter
  unit: Add l_timespec_to_usecs() unit test.
  time: Use l_timespec_to_usecs() to avoid overflow.
  path: Use l_timespec_to_usecs() to avoid overflow.

 ell/ell.sym      |  1 +
 ell/path.c       |  3 +--
 ell/time.c       | 26 +++++++++++++++++++++++++-
 ell/time.h       |  3 +++
 unit/test-time.c | 34 ++++++++++++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH 1/4] time: Add timespec to microseconds converter

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

Implement a new function, l_timespec_to_usecs(), that checks for
integer overflow in the multiplication and addition operations that
occur during the conversion of the timespec members to microseconds.
---
 ell/ell.sym |  1 +
 ell/time.c  | 24 ++++++++++++++++++++++++
 ell/time.h  |  3 +++
 3 files changed, 28 insertions(+)

diff --git a/ell/ell.sym b/ell/ell.sym
index 0c83b87..776a94c 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -526,6 +526,7 @@ global:
 	l_ecdh_generate_key_pair;
 	l_ecdh_generate_shared_secret;
 	/* time */
+	l_timespec_to_usecs;
 	l_time_now;
 	/* gpio */
 	l_gpio_chips_with_line_label;
diff --git a/ell/time.c b/ell/time.c
index 6150d03..becf985 100644
--- a/ell/time.c
+++ b/ell/time.c
@@ -30,6 +30,30 @@
 #include "time.h"
 #include "private.h"
 
+/**
+ * l_timespec_to_usecs() - convert timespec value to microseconds
+ * @t: pointer to timespec value to be converted to microseconds.
+ *
+ * Convert the timespec value @t to microseconds, being careful to
+ * avoid integer overflows during the conversion.
+ *
+ * Return: Number of microseconds in @t, UINT64_MAX on overflow, and
+ *         zero if @t is NULL.
+ */
+LIB_EXPORT uint64_t l_timespec_to_usecs(const struct timespec *t)
+{
+	if (!t)
+		return 0;
+
+	uint64_t usecs = t->tv_sec * L_USEC_PER_SEC;
+
+	/* check overflow */
+	if (usecs / L_USEC_PER_SEC != t->tv_sec)
+		return UINT64_MAX;
+
+	return l_time_offset(usecs, t->tv_nsec / L_NSEC_PER_USEC);
+}
+
 /**
  * l_time_now:
  *
diff --git a/ell/time.h b/ell/time.h
index 6976280..6011b80 100644
--- a/ell/time.h
+++ b/ell/time.h
@@ -38,6 +38,9 @@ extern "C" {
 #define L_NSEC_PER_USEC 1000ULL
 #define L_TIME_INVALID ((uint64_t) -1)
 
+struct timespec;
+
+uint64_t l_timespec_to_usecs(const struct timespec *t);
 uint64_t l_time_now(void);
 
 static inline bool l_time_after(uint64_t a, uint64_t b)
-- 
2.20.1

[PATCH 2/4] unit: Add l_timespec_to_usecs() unit test.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

---
 unit/test-time.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/unit/test-time.c b/unit/test-time.c
index 1301891..3c8fd0d 100644
--- a/unit/test-time.c
+++ b/unit/test-time.c
@@ -25,6 +25,7 @@
 #endif
 
 #include <assert.h>
+#include <limits.h>
 
 #include <ell/ell.h>
 
@@ -49,12 +50,45 @@ static void test_offset(const void *data)
 	assert(l_time_offset(max_minus, 1001) == UINT64_MAX);
 }
 
+static void test_timespec_to_usecs(const void *data)
+{
+	static const time_t max_secs =
+		sizeof(time_t) == 4 ? UINT32_MAX : UINT64_MAX;
+	static const long max_nsecs = LONG_MAX;
+
+	static const time_t secs = 2000;
+	static const long nsecs = 1001;
+
+	/* force integer multiplication overflow */
+	static const struct timespec t_max_secs = { .tv_sec  = max_secs };
+
+	/* force integer addition overflow */
+	static const struct timespec t_max_nsecs = {
+		.tv_sec	 = (max_secs / L_USEC_PER_SEC) - 1,
+		.tv_nsec = max_nsecs
+	};
+
+	static const struct timespec t = {
+		.tv_sec	 = secs,
+		.tv_nsec = nsecs
+	};
+
+	uint64_t usecs = t.tv_sec * L_USEC_PER_SEC +
+		t.tv_nsec / L_NSEC_PER_USEC;
+
+	assert(l_timespec_to_usecs(&t_max_secs) == UINT64_MAX);
+	assert(l_timespec_to_usecs(&t_max_nsecs) == UINT64_MAX);
+	assert(l_timespec_to_usecs(&t) == usecs);
+	assert(l_timespec_to_usecs(NULL) == 0);
+}
+
 int main(int argc, char *argv[])
 {
 	l_test_init(&argc, &argv);
 
 	l_test_add("Test before/after/diff", test_before_after_diff, NULL);
 	l_test_add("Test offset", test_offset, NULL);
+	l_test_add("Test timespec to usecs", test_timespec_to_usecs, NULL);
 
 	return l_test_run();
 }
-- 
2.20.1

[PATCH 3/4] time: Use l_timespec_to_usecs() to avoid overflow.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

---
 ell/time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/time.c b/ell/time.c
index becf985..3bf75e6 100644
--- a/ell/time.c
+++ b/ell/time.c
@@ -67,7 +67,7 @@ LIB_EXPORT uint64_t l_time_now(void)
 
 	clock_gettime(CLOCK_BOOTTIME, &now);
 
-	return now.tv_sec * 1000000 + now.tv_nsec / 1000;
+	return l_timespec_to_usecs(&now);
 }
 
 /**
-- 
2.20.1

[PATCH 4/4] path: Use l_timespec_to_usecs() to avoid overflow.

[Ossama Othman]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

---
 ell/path.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ell/path.c b/ell/path.c
index df5a703..e911c01 100644
--- a/ell/path.c
+++ b/ell/path.c
@@ -162,8 +162,7 @@ LIB_EXPORT uint64_t l_path_get_mtime(const char *path)
 	if (ret < 0)
 		return L_TIME_INVALID;
 
-	return (uint64_t) sb.st_mtim.tv_sec * 1000000 +
-		sb.st_mtim.tv_nsec / 1000;
+	return l_timespec_to_usecs(&sb.st_mtim);
 }
 
 /**
-- 
2.20.1

Re: [PATCH 1/4] time: Add timespec to microseconds converter

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2779 bytes --]

Hi Ossama,

On 3/2/20 3:28 PM, Ossama Othman wrote:
> Implement a new function, l_timespec_to_usecs(), that checks for
> integer overflow in the multiplication and addition operations that
> occur during the conversion of the timespec members to microseconds.
> ---
>   ell/ell.sym |  1 +
>   ell/time.c  | 24 ++++++++++++++++++++++++
>   ell/time.h  |  3 +++
>   3 files changed, 28 insertions(+)

ell/time.c: In function ‘l_timespec_to_usecs’:
ell/time.c:48:2: error: ISO C90 forbids mixed declarations and code 
[-Werror=declaration-after-statement]
    48 |  uint64_t usecs = t->tv_sec * L_USEC_PER_SEC;
       |  ^~~~~~~~
ell/time.c:51:29: error: comparison of integer expressions of different 
signedness: ‘long long unsigned int’ and ‘__time_t’ {aka ‘const long 
int’} [-Werror=sign-compare]
    51 |  if (usecs / L_USEC_PER_SEC != t->tv_sec)
       |                             ^~
cc1: all warnings being treated as errors


> 
> diff --git a/ell/ell.sym b/ell/ell.sym
> index 0c83b87..776a94c 100644
> --- a/ell/ell.sym
> +++ b/ell/ell.sym
> @@ -526,6 +526,7 @@ global:
>   	l_ecdh_generate_key_pair;
>   	l_ecdh_generate_shared_secret;
>   	/* time */
> +	l_timespec_to_usecs;
>   	l_time_now;
>   	/* gpio */
>   	l_gpio_chips_with_line_label;
> diff --git a/ell/time.c b/ell/time.c
> index 6150d03..becf985 100644
> --- a/ell/time.c
> +++ b/ell/time.c
> @@ -30,6 +30,30 @@
>   #include "time.h"
>   #include "private.h"
>   
> +/**
> + * l_timespec_to_usecs() - convert timespec value to microseconds
> + * @t: pointer to timespec value to be converted to microseconds.
> + *
> + * Convert the timespec value @t to microseconds, being careful to
> + * avoid integer overflows during the conversion.
> + *
> + * Return: Number of microseconds in @t, UINT64_MAX on overflow, and
> + *         zero if @t is NULL.
> + */
> +LIB_EXPORT uint64_t l_timespec_to_usecs(const struct timespec *t)
> +{
> +	if (!t)
> +		return 0;
> +
> +	uint64_t usecs = t->tv_sec * L_USEC_PER_SEC;
> +
> +	/* check overflow */
> +	if (usecs / L_USEC_PER_SEC != t->tv_sec)

There has to be a better way than this division...

> +		return UINT64_MAX;
> +
> +	return l_time_offset(usecs, t->tv_nsec / L_NSEC_PER_USEC);
> +}
> +
>   /**
>    * l_time_now:
>    *
> diff --git a/ell/time.h b/ell/time.h
> index 6976280..6011b80 100644
> --- a/ell/time.h
> +++ b/ell/time.h
> @@ -38,6 +38,9 @@ extern "C" {
>   #define L_NSEC_PER_USEC 1000ULL
>   #define L_TIME_INVALID ((uint64_t) -1)
>   
> +struct timespec;
> +
> +uint64_t l_timespec_to_usecs(const struct timespec *t);
>   uint64_t l_time_now(void);
>   
>   static inline bool l_time_after(uint64_t a, uint64_t b)
> 

Regards,
-Denis

Re: [PATCH 2/4] unit: Add l_timespec_to_usecs() unit test.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]

Hi Ossama,

On 3/2/20 3:28 PM, Ossama Othman wrote:
> ---
>   unit/test-time.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/unit/test-time.c b/unit/test-time.c
> index 1301891..3c8fd0d 100644
> --- a/unit/test-time.c
> +++ b/unit/test-time.c
> @@ -25,6 +25,7 @@
>   #endif
>   
>   #include <assert.h>
> +#include <limits.h>
>   
>   #include <ell/ell.h>
>   
> @@ -49,12 +50,45 @@ static void test_offset(const void *data)
>   	assert(l_time_offset(max_minus, 1001) == UINT64_MAX);
>   }
>   
> +static void test_timespec_to_usecs(const void *data)
> +{
> +	static const time_t max_secs =
> +		sizeof(time_t) == 4 ? UINT32_MAX : UINT64_MAX;
> +	static const long max_nsecs = LONG_MAX;

The maximum is 999 999 999.  man clock_gettime

> +
> +	static const time_t secs = 2000;
> +	static const long nsecs = 1001;
> +
> +	/* force integer multiplication overflow */
> +	static const struct timespec t_max_secs = { .tv_sec  = max_secs };
> +
> +	/* force integer addition overflow */
> +	static const struct timespec t_max_nsecs = {
> +		.tv_sec	 = (max_secs / L_USEC_PER_SEC) - 1,
> +		.tv_nsec = max_nsecs
> +	};
> +
> +	static const struct timespec t = {
> +		.tv_sec	 = secs,
> +		.tv_nsec = nsecs
> +	};
> +
> +	uint64_t usecs = t.tv_sec * L_USEC_PER_SEC +
> +		t.tv_nsec / L_NSEC_PER_USEC;
> +
> +	assert(l_timespec_to_usecs(&t_max_secs) == UINT64_MAX);
> +	assert(l_timespec_to_usecs(&t_max_nsecs) == UINT64_MAX);
> +	assert(l_timespec_to_usecs(&t) == usecs);
> +	assert(l_timespec_to_usecs(NULL) == 0);
> +}
> +
>   int main(int argc, char *argv[])
>   {
>   	l_test_init(&argc, &argv);
>   
>   	l_test_add("Test before/after/diff", test_before_after_diff, NULL);
>   	l_test_add("Test offset", test_offset, NULL);
> +	l_test_add("Test timespec to usecs", test_timespec_to_usecs, NULL);
>   
>   	return l_test_run();
>   }
> 

Regards,
-Denis

Re: [PATCH 3/4] time: Use l_timespec_to_usecs() to avoid overflow.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

Hi Ossama,

On 3/2/20 3:28 PM, Ossama Othman wrote:
> ---
>   ell/time.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ell/time.c b/ell/time.c
> index becf985..3bf75e6 100644
> --- a/ell/time.c
> +++ b/ell/time.c
> @@ -67,7 +67,7 @@ LIB_EXPORT uint64_t l_time_now(void)
>   
>   	clock_gettime(CLOCK_BOOTTIME, &now);
>   
> -	return now.tv_sec * 1000000 + now.tv_nsec / 1000;
> +	return l_timespec_to_usecs(&now);

This is time since boot, so I sincerely doubt we want to care about 68+ 
year uptimes.

It might be worthwhile to use ULL constants though just to be pedantic.

>   }
>   
>   /**
> 

Regards,
-Denis

Re: [PATCH 4/4] path: Use l_timespec_to_usecs() to avoid overflow.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

Hi Ossama,

On 3/2/20 3:28 PM, Ossama Othman wrote:
> ---
>   ell/path.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/ell/path.c b/ell/path.c
> index df5a703..e911c01 100644
> --- a/ell/path.c
> +++ b/ell/path.c
> @@ -162,8 +162,7 @@ LIB_EXPORT uint64_t l_path_get_mtime(const char *path)
>   	if (ret < 0)
>   		return L_TIME_INVALID;
>   
> -	return (uint64_t) sb.st_mtim.tv_sec * 1000000 +
> -		sb.st_mtim.tv_nsec / 1000;
> +	return l_timespec_to_usecs(&sb.st_mtim);

So while theoretically possible to overflow, this is _highly_ unlikely. 
I think we are looking at 35-36 bits of sec info, which is 1-2 thousand 
years in the future.

I guess I'm ok if we try and deal with overflow just in case someone is 
setting some really large mtime values as long as l_timespec_to_usecs 
isn't overly expensive.

>   }
>   
>   /**
> 

Regards,
-Denis

Re: [PATCH 4/4] path: Use l_timespec_to_usecs() to avoid overflow.

[Othman, Ossama]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

Hi Denis,

On Mon, Mar 2, 2020 at 2:06 PM Denis Kenzior <denkenz@gmail.com> wrote:
> So while theoretically possible to overflow, this is _highly_ unlikely.
> I think we are looking at 35-36 bits of sec info, which is 1-2 thousand
> years in the future.

That's a good point.

> I guess I'm ok if we try and deal with overflow just in case someone is
> setting some really large mtime values as long as l_timespec_to_usecs
> isn't overly expensive.

As you point out that the overflows are highly unlikely to occur, I'm
fine with dropping my patches.

Thanks Denis!
-Ossama