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.
