Re: [PATCH] nss: Fix memory leak in findLease

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.fedin(a)fobos-nt.ru&gt; > > Signed-off-by: Alexander Kuznetsov <kuznetsovam(a)altlinux.org&gt; > > --- > > 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.