Re: [Libguestfs] [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(a)redhat.com&gt; --- 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); } --