| Message ID | 1478551093-32757-1-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
[updating subject line, to make sure this regression is fixed] On 11/07/2016 02:38 PM, Eric Blake wrote: > Commit 7d3123e converted a single read_sync() into a while loop > that assumed that read_sync() would either make progress or give > an error. But when the server hangs up early, the client sees > EOF (a read_sync() of 0) and never makes progress, which in turn > caused qemu-iotest './check -nbd 83' to go into an infinite loop. > > Rework the loop to accomodate reads cut short by EOF. and s/accomodate/accommodate/ if the maintainer would be so nice (not every day I get to correct my own typos) > > Reported-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > nbd/client.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 5d94e34..29b6edf 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -79,20 +79,21 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports); > * the amount of bytes consumed. */ > static ssize_t drop_sync(QIOChannel *ioc, size_t size) > { > - ssize_t ret, dropped = size; > + ssize_t ret = 0; > char small[1024]; > char *buffer; > > buffer = sizeof(small) < size ? small : g_malloc(MIN(65536, size)); > while (size > 0) { > - ret = read_sync(ioc, buffer, MIN(65536, size)); > - if (ret < 0) { > + ssize_t count = read_sync(ioc, buffer, MIN(65536, size)); > + > + if (count <= 0) { > goto cleanup; > } > - assert(ret <= size); > - size -= ret; > + assert(count <= size); > + size -= count; > + ret += count; > } > - ret = dropped; > > cleanup: > if (buffer != small) { >
On 07.11.2016 21:38, Eric Blake wrote: > Commit 7d3123e converted a single read_sync() into a while loop > that assumed that read_sync() would either make progress or give > an error. But when the server hangs up early, the client sees > EOF (a read_sync() of 0) and never makes progress, which in turn > caused qemu-iotest './check -nbd 83' to go into an infinite loop. > > Rework the loop to accomodate reads cut short by EOF. > > Reported-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > nbd/client.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> But what about the server's nbd_negotiate_drop_sync()? It uses pretty much the same code, so it seems susceptible to the same issue (only that we don't have a test for that side). Max
On 11/07/2016 04:22 PM, Max Reitz wrote: > On 07.11.2016 21:38, Eric Blake wrote: >> Commit 7d3123e converted a single read_sync() into a while loop >> that assumed that read_sync() would either make progress or give >> an error. But when the server hangs up early, the client sees >> EOF (a read_sync() of 0) and never makes progress, which in turn >> caused qemu-iotest './check -nbd 83' to go into an infinite loop. >> >> Rework the loop to accomodate reads cut short by EOF. >> >> Reported-by: Max Reitz <mreitz@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> nbd/client.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > But what about the server's nbd_negotiate_drop_sync()? It uses pretty > much the same code, so it seems susceptible to the same issue (only that > we don't have a test for that side). If so, that's an older bug (pre-existing back to at least 2.6?), so it should be a separate fix, if anything. I guess it's time to figure out how to test the server against ill-behaved clients...
On 07/11/2016 23:45, Eric Blake wrote: > On 11/07/2016 04:22 PM, Max Reitz wrote: >> On 07.11.2016 21:38, Eric Blake wrote: >>> Commit 7d3123e converted a single read_sync() into a while loop >>> that assumed that read_sync() would either make progress or give >>> an error. But when the server hangs up early, the client sees >>> EOF (a read_sync() of 0) and never makes progress, which in turn >>> caused qemu-iotest './check -nbd 83' to go into an infinite loop. >>> >>> Rework the loop to accomodate reads cut short by EOF. >>> >>> Reported-by: Max Reitz <mreitz@redhat.com> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >>> nbd/client.c | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> But what about the server's nbd_negotiate_drop_sync()? It uses pretty >> much the same code, so it seems susceptible to the same issue (only that >> we don't have a test for that side). > > If so, that's an older bug (pre-existing back to at least 2.6?), so it > should be a separate fix, if anything. > > I guess it's time to figure out how to test the server against > ill-behaved clients... Using afl perhaps? Paolo
diff --git a/nbd/client.c b/nbd/client.c index 5d94e34..29b6edf 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -79,20 +79,21 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports); * the amount of bytes consumed. */ static ssize_t drop_sync(QIOChannel *ioc, size_t size) { - ssize_t ret, dropped = size; + ssize_t ret = 0; char small[1024]; char *buffer; buffer = sizeof(small) < size ? small : g_malloc(MIN(65536, size)); while (size > 0) { - ret = read_sync(ioc, buffer, MIN(65536, size)); - if (ret < 0) { + ssize_t count = read_sync(ioc, buffer, MIN(65536, size)); + + if (count <= 0) { goto cleanup; } - assert(ret <= size); - size -= ret; + assert(count <= size); + size -= count; + ret += count; } - ret = dropped; cleanup: if (buffer != small) {
Commit 7d3123e converted a single read_sync() into a while loop that assumed that read_sync() would either make progress or give an error. But when the server hangs up early, the client sees EOF (a read_sync() of 0) and never makes progress, which in turn caused qemu-iotest './check -nbd 83' to go into an infinite loop. Rework the loop to accomodate reads cut short by EOF. Reported-by: Max Reitz <mreitz@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- nbd/client.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)