Re: [Libguestfs] [PATCH v4 23/24] nbd/server: Prepare for per-request filtering of BLOCK_STATUS

[Vladimir Sementsov-Ogievskiy]

On 08.06.23 22:15, Eric Blake wrote:
> On Thu, Jun 08, 2023 at 08:56:52AM -0500, Eric Blake wrote:
> > The next commit will add support for the optional extension
> > NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can
> > request that the server only return a subset of negotiated contexts,
> > rather than all contexts.  To make that task easier, this patch
> > populates the list of contexts to return on a per-command basis (for
> > now, identical to the full set of negotiated contexts).
> >
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
>
> > +++ b/nbd/server.c
> > @@ -2491,6 +2491,8 @@ static int coroutine_fn 
> > nbd_co_receive_request(NBDRequestData *req, NBDRequest *
> >               error_setg(errp, "No memory");
> >               return -ENOMEM;
> >           }
> > +    } else if (request->type == NBD_CMD_BLOCK_STATUS) {
> > +        request->contexts = &client->contexts;
> >       }
>
> THere are paths where request->contexts is left NULL but request->type
> was set...
>
> > @@ -2841,6 +2848,11 @@ static coroutine_fn void nbd_trip(void *opaque)
> >       } else {
> >           ret = nbd_handle_request(client, &request, req->data, &local_err);
> >       }
> > +    if (request.type == NBD_CMD_BLOCK_STATUS &&
> > +        request.contexts != &client->contexts) {
> > +        g_free(request.contexts->bitmaps);
> > +        g_free(request.contexts);
> > +    }
>
> so I think this also needs to be tweaked to check that
> request.contexts is non-NULL before dereferencing to free bitmaps.

agree (and I missed this during my review :)

suggest:

if (request.contexts && request.contexts != &client->contexts) {
  g_free(..)
  g_free(..)
}


--
Best regards,
Vladimir