Libguestfs - Jul 2019 - [nbdkit PATCH] delay: Avoid numeric overflow

Attempting delay-read=1000 results in no delay whatsoever: 1000
seconds, scaled to milliseconds, stored in int, then subjected to

      .tv_nsec = (ms * 1000000) % 1000000000

results in .tv_nsec being set to 567587328 thanks to 32-bit overflow,
but that in turn results in instant EINVAL failure of nanosleep().

Fix it by diagnosing failure to fit in an int during config, and avoid
math that overflows during delay().  Forbid negative delays while at it.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

I was trying to test what happens when a non-fatal signal is sent to
nbdkit (by adding a no-op SIGUSR1 handler). Most of the time, sending
SIGUSR1 to the process as a whole ended up cutting the poll() in a
different thread short (and not the nanosleep()), but by directing
SIGUSR1 to the thread stuck in nanosleep, I confirmed that nanosleep()
indeed returns early with EINTR, and we end up with insufficient
delay.  But getting to that point in my testing required that I first
diagnosed why my attempt at a 1000-second sleep acted like no delay at
all.

I plan on adding an nbdkit_sleep() wrapper function, which will resume
the sleep on unrelated EINTR (fixing the theoretical problem of not
sleeping long enough - theoretical since it only matters if we handle
a signal but want to continue execution, but all our current handled
signals instead request process termination) as well as the problem of
keeping nbdkit alive too long (by specifically returning an error to
let the delay filter know that it is time to exit, rather than
finishing the sleep, whether it was a signal that interrupted a
different thread or merely detection of EOF from the client).

 filters/delay/delay.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/filters/delay/delay.c b/filters/delay/delay.c
index 11681c88..80eb33b0 100644
--- a/filters/delay/delay.c
+++ b/filters/delay/delay.c
@@ -32,6 +32,7 @@

 #include <config.h>

+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
@@ -54,7 +55,7 @@ parse_delay (const char *key, const char *value)
   int r;

   if (len > 2 && strcmp (&value[len-2], "ms") == 0) {
-    if (sscanf (value, "%d", &r) == 1)
+    if (sscanf (value, "%d", &r) == 1 && r >= 0)
       return r;
     else {
       nbdkit_error ("cannot parse %s in milliseconds parameter: %s",
@@ -63,8 +64,14 @@ parse_delay (const char *key, const char *value)
     }
   }
   else {
-    if (sscanf (value, "%d", &r) == 1)
+    if (sscanf (value, "%d", &r) == 1 && r >= 0) {
+      if (r * 1000LL > INT_MAX) {
+        nbdkit_error ("seconds parameter %s is too large: %s",
+                      key, value);
+        return -1;
+      }
       return r * 1000;
+    }
     else {
       nbdkit_error ("cannot parse %s in seconds parameter: %s",
                     key, value);
@@ -79,7 +86,7 @@ delay (int ms)
   if (ms > 0) {
     const struct timespec ts = {
       .tv_sec = ms / 1000,
-      .tv_nsec = (ms * 1000000) % 1000000000
+      .tv_nsec = (ms % 1000) * 1000000,
     };
     nanosleep (&ts, NULL);
   }
-- 
2.20.1

On Thu, Jul 25, 2019 at 09:24:19PM -0500, Eric Blake
wrote:
> Attempting delay-read=1000 results in no delay whatsoever: 1000
> seconds, scaled to milliseconds, stored in int, then subjected to
> 
>       .tv_nsec = (ms * 1000000) % 1000000000
> 
> results in .tv_nsec being set to 567587328 thanks to 32-bit overflow,
> but that in turn results in instant EINVAL failure of nanosleep().
> 
> Fix it by diagnosing failure to fit in an int during config, and avoid
> math that overflows during delay().  Forbid negative delays while at it.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> I was trying to test what happens when a non-fatal signal is sent to
> nbdkit (by adding a no-op SIGUSR1 handler). Most of the time, sending
> SIGUSR1 to the process as a whole ended up cutting the poll() in a
> different thread short (and not the nanosleep()), but by directing
> SIGUSR1 to the thread stuck in nanosleep, I confirmed that nanosleep()
> indeed returns early with EINTR, and we end up with insufficient
> delay.  But getting to that point in my testing required that I first
> diagnosed why my attempt at a 1000-second sleep acted like no delay at
> all.
> 
> I plan on adding an nbdkit_sleep() wrapper function, which will resume
> the sleep on unrelated EINTR (fixing the theoretical problem of not
> sleeping long enough - theoretical since it only matters if we handle
> a signal but want to continue execution, but all our current handled
> signals instead request process termination) as well as the problem of
> keeping nbdkit alive too long (by specifically returning an error to
> let the delay filter know that it is time to exit, rather than
> finishing the sleep, whether it was a signal that interrupted a
> different thread or merely detection of EOF from the client).
> 
>  filters/delay/delay.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/filters/delay/delay.c b/filters/delay/delay.c
> index 11681c88..80eb33b0 100644
> --- a/filters/delay/delay.c
> +++ b/filters/delay/delay.c
> @@ -32,6 +32,7 @@
> 
>  #include <config.h>
> 
> +#include <limits.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdint.h>
> @@ -54,7 +55,7 @@ parse_delay (const char *key, const char *value)
>    int r;
> 
>    if (len > 2 && strcmp (&value[len-2], "ms") ==
0) {
> -    if (sscanf (value, "%d", &r) == 1)
> +    if (sscanf (value, "%d", &r) == 1 && r >= 0)
>        return r;
>      else {
>        nbdkit_error ("cannot parse %s in milliseconds parameter:
%s",
> @@ -63,8 +64,14 @@ parse_delay (const char *key, const char *value)
>      }
>    }
>    else {
> -    if (sscanf (value, "%d", &r) == 1)
> +    if (sscanf (value, "%d", &r) == 1 && r >= 0)
{
> +      if (r * 1000LL > INT_MAX) {
> +        nbdkit_error ("seconds parameter %s is too large: %s",
> +                      key, value);
> +        return -1;
> +      }
>        return r * 1000;
> +    }
>      else {
>        nbdkit_error ("cannot parse %s in seconds parameter: %s",
>                      key, value);
> @@ -79,7 +86,7 @@ delay (int ms)
>    if (ms > 0) {
>      const struct timespec ts = {
>        .tv_sec = ms / 1000,
> -      .tv_nsec = (ms * 1000000) % 1000000000
> +      .tv_nsec = (ms % 1000) * 1000000,
>      };
>      nanosleep (&ts, NULL);
>    }
> -- 
Oops :-(

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW