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: > > $ nbdkit --filter=delay null delay-close=3 \ > --run 'time nbdsh -u $uri -c "h.shutdown()" > time nbdsh -u $uri -c "pass"' > > real 0m3.061s # Client used shutdown, was delayed > user 0m0.028s > sys 0m0.030s > > real 0m0.058s # Client disconnected, was not delayed > user 0m0.029s > sys 0m0.027s > > Reported-by: Ming Xie > Fixes: commit de8dcd3a34a38b088a0f9a6f8ca754702ad1f598 > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1991652 > --- > filters/delay/nbdkit-delay-filter.pod | 13 +++++++-- > filters/delay/delay.c | 42 +++++++++++++++++++-------- > 2 files changed, 40 insertions(+), 15 deletions(-) > > diff --git a/filters/delay/nbdkit-delay-filter.pod b/filters/delay/nbdkit-delay-filter.pod > index 11ae544b..bfc90dae 100644 > --- a/filters/delay/nbdkit-delay-filter.pod > +++ b/filters/delay/nbdkit-delay-filter.pod > @@ -117,15 +117,22 @@ the plugin. > > =item B<delay-open=>NNB<ms> > > +(nbdkit E<ge> 1.28) > + > +Delay open (client connection) by C<SECS> seconds or C<NN> > +milliseconds. > + > =item B<delay-close=>SECS > > =item B<delay-close=>NNB<ms> > > (nbdkit E<ge> 1.28) > > -Delay open and close operations by C<SECS> seconds or C<NN> > -milliseconds. Open corresponds to client connection. Close may not > -be visible to clients if they abruptly disconnect. > +Delay close (client disconnection) by C<SECS> seconds of C<NN>or> +milliseconds. This will also cause server shutdown to be delayed. > +This only affects clients that gracefully disconnect (using > +C<NBD_CMD_DISC> / libnbd function L<nbd_shutdown(3)>). Clients that > +abruptly close the socket cannot be delayed. > > =back >> +/* Close connection. > + * > + * We cannot call nbdkit_nanosleep here because the socket may have > + * been closed and that function will abort and return immediately. > + * However we want for force a sleep (even if the server is shuttings/for /to /> + * down) so use regular nanosleep instead. > + * > + * We cannot use the .close callback because that happens after the > + * socket has closed, thus not delaying the client. By using > + * .finalize we can delay well-behaved clients (those that use > + * NBD_CMD_DISC). We cannot delay clients that drop the connection. > + */ > +static int > +delay_finalize (nbdkit_next *next, void *handle) > {With those typos fixed, series looks good to me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org