Re: [PATCH] vdpa: Fix file descriptor leak on get features error

[Michael S. Tsirkin]

On Mon, Aug 01, 2022 at 08:48:41PM +0200, Eugenio Perez Martin wrote:
> On Mon, Aug 1, 2022 at 8:34 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 1 Aug 2022 at 19:31, Laurent Vivier <lvivier@redhat.com> wrote:
> > >
> > > On 01/08/2022 16:47, Eugenio Pérez wrote:
> > > > File descriptor vdpa_device_fd is not free in the case of returning
> > > > error from vhost_vdpa_get_features. Fixing it by making all errors go to
> > > > the same error path.
> > > >
> > > > Resolves: Coverity CID 1490785
> > > > Fixes: 8170ab3f43 ("vdpa: Extract get features part from 
> > > > vhost_vdpa_get_max_queue_pairs")
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >   net/vhost-vdpa.c | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 6abad276a6..303447a68e 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -566,7 +566,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const 
> > > > char *name,
> > > >       g_autofree NetClientState **ncs = NULL;
> > > >       g_autoptr(VhostIOVATree) iova_tree = NULL;
> > > >       NetClientState *nc;
> > > > -    int queue_pairs, r, i, has_cvq = 0;
> > > > +    int queue_pairs, r, i = 0, has_cvq = 0;
> > > >
> > > >       assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > > >       opts = &netdev->u.vhost_vdpa;
> > > > @@ -582,7 +582,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const 
> > > > char *name,
> > > >
> > > >       r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp);
> > > >       if (unlikely(r < 0)) {
> > > > -        return r;
> > > > +        goto err;
> > >
> > > Why don't you use "goto err_svq"?
> >
> > This patch is kind of half of the idea discussed in the thread where
> > this problem was reported, which is that by setting i = 0 you can
> > then consistently have all the error handling be 'goto err' and that
> > frees everything that needs to be freed regardless of whether it's
> > called after or before the initialization of the ncs[] entries. But it
> > doesn't do the other half of the job, which is making all the other
> > error handling code in the function also use 'goto err', so it looks
> > a bit odd as it stands.
> >
> 
> That's right, I thought just fixing the issue about the leaked file
> descriptor was the right thing to do in the hard feature freeze, and
> that other part should be left for the next dev phase. Is the unified
> error handling code acceptable for this period? I can send a patch
> either on top of this one or squashed if so for sure.
> 
> Thanks!

Yea I'd prefer a minimal patch, unless others object strongly.


-- 
MST