Re: [PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

[Alexander Ivanov]

On 6/10/24 14:33, Eric Blake wrote:
> On Sat, Jun 08, 2024 at 11:36:59AM GMT, Alexander Ivanov wrote:
> > There is a bug reproducer in the attachment.
> Summarizing the reproducer, you are repeatedly calling QMP
> nbd-server-start/nbd-server-stop on qemu as NBD server in one thread,
> and repeatedly calling 'qemu-nbd -L' in another, to try and provoke
> the race where the server is stopped while qemu-nbd -L is still trying
> to grab information.
Yes, you are right.
> > On 6/7/24 17:00, Alexander Ivanov wrote:
> > > In some cases, the NBD server can be stopped before
> > > nbd_blockdev_client_closed() is called, causing the nbd_server variable
> > > to be nullified. This leads to a NULL pointer dereference when accessing
> > > nbd_server.
> Am I correct that the NULL pointer deref was in qemu-nbd in your
> reproducer, and not in qemu-kvm?
No, it happened in qemu-kvm.
> > > Add a NULL check for nbd_server to the nbd_blockdev_client_closed()
> > > function to prevent NULL pointer dereference.
> > >
> > > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > > ---
> > >    blockdev-nbd.c | 3 +++
> > >    1 file changed, 3 insertions(+)
> > >
> > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> > > index 213012435f..fb1f30ae0d 100644
> > > --- a/blockdev-nbd.c
> > > +++ b/blockdev-nbd.c
> > > @@ -52,6 +52,9 @@ int nbd_server_max_connections(void)
> > >    static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
> > >    {
> > >        nbd_client_put(client);
> > > +    if (nbd_server == NULL) {
> > > +        return;
> > > +    }
> > >        assert(nbd_server->connections > 0);
> While this does indeed avoid the NULL dereference right here, I still
> want to understand why nbd_server is getting set to NULL while there
> is still an active client, and make sure we don't have any other NULL
> derefs.  I'll respond again once I've studied the code a bit more.
I agree that the patch fixes only symptoms, but haven't succeeded with the
bug roots investigation, and decided to raise this issue with the community.

Also, later I reproduced another bug with the patch applied and with the
same reproducer. There was an

   assert(nbd_server->connections > 0);

violation in nbd_blockdev_client_closed(). It happens much less frequently,
however. Think the reasons are similar to the original bug.
> > >        nbd_server->connections--;
> > >        nbd_update_server_watch(nbd_server);
> > --
> > Best regards,
> > Alexander Ivanov

--
Best regards,
Alexander Ivanov