Libguestfs - Sep 2019 - Re: [nbdkit PATCH v2 5/7] server: Allow longer NBD_OPT

On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake
wrote:
> Fixes the fact that clients could not request the maximum string
> length except with NBD_OPT_EXPORT_LEN.  Updates the testsuite to
> match.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  server/protocol-handshake-newstyle.c | 12 +++++++-----
>  tests/test-long-name.sh              | 10 ++++------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
> index 34958360..3b5d144e 100644
> --- a/server/protocol-handshake-newstyle.c
> +++ b/server/protocol-handshake-newstyle.c
> @@ -48,7 +48,7 @@
>  #define MAX_NR_OPTIONS 32
> 
>  /* Maximum length of any option data (bytes). */
> -#define MAX_OPTION_LENGTH 4096
> +#define MAX_OPTION_LENGTH (NBD_MAX_STRING * 4)
> 
>  /* Receive newstyle options. */
>  static int
> @@ -255,7 +255,7 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
>    uint64_t version;
>    uint32_t option;
>    uint32_t optlen;
> -  char data[MAX_OPTION_LENGTH+1];
> +  CLEANUP_FREE char *data = NULL;
Even though you have the CLEANUP here ...
>    struct nbd_export_name_option_reply handshake_finish;
>    const char *optname;
>    uint64_t exportsize;
> @@ -281,6 +281,11 @@ negotiate_handshake_newstyle_options (struct
connection *conn)
>        nbdkit_error ("client option data too long (%" PRIu32
")", optlen);
>        return -1;
>      }
> +    data = malloc (optlen + 1); /* Allowing a trailing NUL helps some uses
*/
> +    if (data == NULL) {
> +      nbdkit_error ("malloc: %m");
> +      return -1;
> +    }
... when I run this patch series under valgrind I get mainly errors
originating at this malloc:

==1251605== 58 bytes in 4 blocks are definitely lost in loss record 4 of 10
==1251605==    at 0x896180B: malloc (vg_replace_malloc.c:309)
==1251605==    by 0x11909F: protocol_handshake_newstyle (protocol-handshake-news
tyle.c:288)
==1251605==    by 0x118804: protocol_handshake (protocol-handshake.c:55)
==1251605==    by 0x112080: handle_single_connection (connections.c:165)
==1251605==    by 0x11B84D: start_thread (sockets.c:276)
==1251605==    by 0x8BB74E1: start_thread (pthread_create.c:479)
==1251605==    by 0x8CD3642: clone (clone.S:95)

I didn't look at it closely but there does appear to be a memory leak
in this patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Sat, Sep 28, 2019 at 10:38:32PM +0100, Richard W.M. Jones
wrote:
> ... when I run this patch series under valgrind I get mainly errors
"many" not "mainly" :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 9/28/19 4:38 PM, Richard W.M. Jones wrote:
> On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote:
>> Fixes the fact that clients could not request the maximum string
>> length except with NBD_OPT_EXPORT_LEN.  Updates the testsuite to
>> match.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> @@ -255,7 +255,7 @@ negotiate_handshake_newstyle_options (struct
connection *conn)
>>     uint64_t version;
>>     uint32_t option;
>>     uint32_t optlen;
>> -  char data[MAX_OPTION_LENGTH+1];
>> +  CLEANUP_FREE char *data = NULL;
> 
> Even though you have the CLEANUP here ...
Scope is too wide.  I need to sink the declaration...
> 
>>     struct nbd_export_name_option_reply handshake_finish;
>>     const char *optname;
>>     uint64_t exportsize;
>> @@ -281,6 +281,11 @@ negotiate_handshake_newstyle_options (struct
connection *conn)
>>         nbdkit_error ("client option data too long (%" PRIu32
")", optlen);
>>         return -1;
>>       }
>> +    data = malloc (optlen + 1); /* Allowing a trailing NUL helps some
uses */
>> +    if (data == NULL) {
>> +      nbdkit_error ("malloc: %m");
>> +      return -1;
>> +    }
...inside the while loop, so that each iteration of the loop frees and 
reallocates a data buffer.
> 
> ... when I run this patch series under valgrind I get mainly errors
> originating at this malloc:
> 
> ==1251605== 58 bytes in 4 blocks are definitely lost in loss record 4 of 10
> ==1251605==    at 0x896180B: malloc (vg_replace_malloc.c:309)
> ==1251605==    by 0x11909F: protocol_handshake_newstyle
(protocol-handshake-news
> tyle.c:288)
> ==1251605==    by 0x118804: protocol_handshake (protocol-handshake.c:55)
> ==1251605==    by 0x112080: handle_single_connection (connections.c:165)
> ==1251605==    by 0x11B84D: start_thread (sockets.c:276)
> ==1251605==    by 0x8BB74E1: start_thread (pthread_create.c:479)
> ==1251605==    by 0x8CD3642: clone (clone.S:95)
> 
> I didn't look at it closely but there does appear to be a memory leak
> in this patch.
Yep, and you pointed it out very nicely. I'll fix before pushing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Sep 30, 2019 at 11:22:03AM -0500, Eric Blake
wrote:
> On 9/28/19 4:38 PM, Richard W.M. Jones wrote:
> >On Fri, Sep 27, 2019 at 11:48:47PM -0500, Eric Blake wrote:
> >>Fixes the fact that clients could not request the maximum string
> >>length except with NBD_OPT_EXPORT_LEN.  Updates the testsuite to
> >>match.
> >>
> >>Signed-off-by: Eric Blake <eblake@redhat.com>
> >>---
> 
> >>@@ -255,7 +255,7 @@ negotiate_handshake_newstyle_options (struct
connection *conn)
> >>    uint64_t version;
> >>    uint32_t option;
> >>    uint32_t optlen;
> >>-  char data[MAX_OPTION_LENGTH+1];
> >>+  CLEANUP_FREE char *data = NULL;
> >
> >Even though you have the CLEANUP here ...
> 
> Scope is too wide.  I need to sink the declaration...
> 
> >
> >>    struct nbd_export_name_option_reply handshake_finish;
> >>    const char *optname;
> >>    uint64_t exportsize;
> >>@@ -281,6 +281,11 @@ negotiate_handshake_newstyle_options (struct
connection *conn)
> >>        nbdkit_error ("client option data too long (%"
PRIu32 ")", optlen);
> >>        return -1;
> >>      }
> >>+    data = malloc (optlen + 1); /* Allowing a trailing NUL helps
some uses */
> >>+    if (data == NULL) {
> >>+      nbdkit_error ("malloc: %m");
> >>+      return -1;
> >>+    }
> 
> ...inside the while loop, so that each iteration of the loop frees
> and reallocates a data buffer.
> 
> >
> >... when I run this patch series under valgrind I get mainly errors
> >originating at this malloc:
> >
> >==1251605== 58 bytes in 4 blocks are definitely lost in loss record 4
of 10
> >==1251605==    at 0x896180B: malloc (vg_replace_malloc.c:309)
> >==1251605==    by 0x11909F: protocol_handshake_newstyle
(protocol-handshake-news
> >tyle.c:288)
> >==1251605==    by 0x118804: protocol_handshake
(protocol-handshake.c:55)
> >==1251605==    by 0x112080: handle_single_connection
(connections.c:165)
> >==1251605==    by 0x11B84D: start_thread (sockets.c:276)
> >==1251605==    by 0x8BB74E1: start_thread (pthread_create.c:479)
> >==1251605==    by 0x8CD3642: clone (clone.S:95)
> >
> >I didn't look at it closely but there does appear to be a memory
leak
> >in this patch.
> 
> Yep, and you pointed it out very nicely. I'll fix before pushing.
Ah yes of course, I didn't spot that there was a loop :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW