Re: [Libguestfs] [PATCH nbdkit 2/3] delay: Fix delay-close

On Tue, Aug 10, 2021 at 09:25:28AM -0500, Eric Blake wrote: > On Tue, Aug 10, 2021 at 09:19:14AM +0100, Richard W.M. Jones wrote: > > See comments in the code for how this has been fixed. > > > > This only delays clients which use NBD_CMD_DISC (libnbd > > nbd_shutdown(3)). Clients which drop the connection obviously cannot > > be delayed. For example: I also noticed that you added another commit 58187831a not posted on list at the time to make delay parsing more robust. However, it uses sscanf() to parse an integer, which is risky on untrusted data (such as the command-line input from the user). First, the code paths that don't use sscanf: $ ./nbdkit -f memory 1 --filter=delay delay-read=1oops nbdkit: error: delay-read: could not parse number: "1oops": trailing garbage Good - this is what we want to tell the user. $ ./nbdkit -f memory 1 --filter=delay delay-read=oops nbdkit: error: delay-read: empty string where we expected a number So-so - we correctly give the user an error, but the error message is poor quality: "oops" is not an empty string (this code path did not use sscanf) Now for the use of sscanf: $ ./nbdkit -f memory 1 --filter=delay delay-read=oopsms nbdkit: error: cannot parse delay-read in milliseconds parameter: oopsms Good - we detected a bug, although the message is worded differently now. $ ./nbdkit -f memory 1 --filter=delay delay-read=1oopsms Oops - our use of sscanf didn't check for trailing garbage, and this is behaving as delay-read=1ms. $ ./nbdkit -fv memory 1 --filter=delay delay-read=999999999999999999999ms Using gdb, I see that in glibc this results in the same as delay-read=4294967295, but that behavior is unspecified by POSIX and may result in other values on other platforms. Better would be detecting overflow, but sscanf() cannot detect numeric overflow. Detecting trailing garbage could be done with sscanf(value, "%ums%n", r, &n) == 1 followed by checking that n consumed strlen(value) bytes, but detecting overflow really needs strtol() rather than sscanf. We have other filters and plugins that use sscanf. As long as their inputs come from stable sources (such as scanning kernel /proc files) or don't parse numbers, that is safe; but in general, use of sscanf to parse user-provided data is risky.