On Mon, Apr 14, 2025 at 02:25:05PM +0200, Peter Krempa wrote:
> On Mon, Apr 14, 2025 at 13:20:55 +0100, Daniel P. Berrangé via Devel wrote:
> > On Mon, Apr 14, 2025 at 03:06:09PM +0300, Alexander Kuznetsov wrote:
> > > path is allocated by asprintf() and must be freed later if realloc()
> > > fails or at
> > > the end of each while() iteration
> > >
> > > Move the free() call out of LIBVIRT_NSS_GUEST macro and add another one if
> > > realloc() fails
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with Svace.
> > >
> > > Reported-by: Dmitry Fedin <d.fe...@fobos-nt.ru>
> > > Signed-off-by: Alexander Kuznetsov <kuznetso...@altlinux.org>
> > > ---
> > > tools/nss/libvirt_nss.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
> > > index d79a00a1b0..190cc7a3dd 100644
> > > --- a/tools/nss/libvirt_nss.c
> > > +++ b/tools/nss/libvirt_nss.c
> > > @@ -141,8 +141,11 @@ findLease(const char *name,
> > > goto cleanup;
> > >
> > > tmpLease = realloc(leaseFiles, sizeof(char *) * (nleaseFiles
> > > + 1));
> > > - if (!tmpLease)
> > > + if (!tmpLease) {
> > > + free(path);
> > > goto cleanup;
> > > + }
> > > +
> > > leaseFiles = tmpLease;
> > > leaseFiles[nleaseFiles++] = path;
> > > #if defined(LIBVIRT_NSS_GUEST)
> > > @@ -155,8 +158,8 @@ findLease(const char *name,
> > > free(path);
> > > goto cleanup;
> > > }
> > > - free(path);
> > > #endif /* LIBVIRT_NSS_GUEST */
> > > + free(path);
> >
> > AFAICT in this 'else if' branch, 'path' is only allocated inside the
> > 'LIBVIRT_NSS_GUEST' condition, so movnig the free outside is pointless.
> >
> > Rather than adding more "free" calls, I think this code would be
> > improved by declaring 'path' with "g_autofree" and using
> > g_steal_pointer when assigning it to 'leaseFiles[nleaseFiles++]',
> > then existing 'free' calls can be removed.
>
> IIUC the NSS module code is supposed to be glib-free to avoid pulling it
> into the code that uses lookups.
Oh yes, but we can still use attribute(cleanup) directly though without
glib. Or even just #define g_autofree locally so we match the codestyle
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
