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

[Eugenio Perez Martin]

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!