[Qemu-devel] [PATCH] nbd: Don't inf-loop on early EOF

[Qemu-devel] [PATCH] nbd: Don't inf-loop on early EOF

[Eric Blake]

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(-)

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) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.8] nbd: Don't inf-loop on early EOF

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

[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) {
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] nbd: Don't inf-loop on early EOF

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH] nbd: Don't inf-loop on early EOF

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

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...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] nbd: Don't inf-loop on early EOF

[Paolo Bonzini]

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