[Libguestfs] [nbdkit PATCH v2 5/7] server: Allow longer NBD_OPT

[Richard W.M. Jones]

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