[PATCH] nss: Fix memory leak in findLease
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@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@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); } errno = 0; -- 2.42.4
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@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@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. 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 :|
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@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@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.
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.fedin@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@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 :|
On Mon, Apr 14, 2025 at 13:27:16 +0100, Daniel P. Berrangé wrote:
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:
[...]
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
Yes; defining it locally or using attribute(cleanup) directly will definitely work. Although I'd suggest to not use 'g_autofree' as name as then it could evoke the impression that the code does use glib.
On Mon, Apr 14, 2025 at 15:06:09 +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@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@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;
The path is added to the array ...
#if defined(LIBVIRT_NSS_GUEST) @@ -155,8 +158,8 @@ findLease(const char *name, free(path); goto cleanup; } - free(path); #endif /* LIBVIRT_NSS_GUEST */
So if you move this after the definition check, and the definition is not defined ...
+ free(path);
... this free will become part of the upper block and free the path filled into the array.
}
errno = 0; -- 2.42.4
On Mon, Apr 14, 2025 at 15:06:09 +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@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@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; + }
This potential leak can be also addressed by rearranging the code so that the array is realloc'd first and the path is formatted just after the realloc. Since the freeing of members in 'leaseFiles' is done based on 'nleaseFiles' it's safe to do even without clearing the new extra memory. This way no 'free' is needed.
+ leaseFiles = tmpLease; leaseFiles[nleaseFiles++] = path;
v2: - don't touch free() call inside LIBVIRT_NSS_GUEST macro - rearrange asprintf to be called after realloc, so no extra free call is needed Alexander Kuznetsov (1): nss: Fix memory leak in findLease tools/nss/libvirt_nss.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) -- 2.42.4
path is allocated by asprintf() and must be freed later if realloc() fails Restructure the code to allocate path only after realloc() succeeds, avoiding the need for an extra free() Found by Linux Verification Center (linuxtesting.org) with Svace. Reported-by: Dmitry Fedin <d.fedin@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org> --- tools/nss/libvirt_nss.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index d79a00a1b0..a1f33ee27b 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -137,12 +137,13 @@ findLease(const char *name, if (dlen >= 7 && !strcmp(entry->d_name + dlen - 7, ".status")) { char **tmpLease; - if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) - goto cleanup; - tmpLease = realloc(leaseFiles, sizeof(char *) * (nleaseFiles + 1)); if (!tmpLease) goto cleanup; + + if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) + goto cleanup; + leaseFiles = tmpLease; leaseFiles[nleaseFiles++] = path; #if defined(LIBVIRT_NSS_GUEST) -- 2.42.4
On 4/15/25 13:48, Alexander Kuznetsov wrote:
path is allocated by asprintf() and must be freed later if realloc() fails
Restructure the code to allocate path only after realloc() succeeds, avoiding the need for an extra free()
Found by Linux Verification Center (linuxtesting.org) with Svace.
Reported-by: Dmitry Fedin <d.fedin@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org> --- tools/nss/libvirt_nss.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c index d79a00a1b0..a1f33ee27b 100644 --- a/tools/nss/libvirt_nss.c +++ b/tools/nss/libvirt_nss.c @@ -137,12 +137,13 @@ findLease(const char *name,
if (dlen >= 7 && !strcmp(entry->d_name + dlen - 7, ".status")) { char **tmpLease; - if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) - goto cleanup; - tmpLease = realloc(leaseFiles, sizeof(char *) * (nleaseFiles + 1)); if (!tmpLease) goto cleanup; + + if (asprintf(&path, "%s/%s", leaseDir, entry->d_name) < 0) + goto cleanup; +
Almost. Consider this is the first iteration of the while() loop. When the control enters the while loop, leaseFiles is still set to NULL (due to being initialized that way). realloc() succeeds and thus tmpLease contains now a pointer to allocated memory. But then, if asprintf() fails, the control jumps onto the cleanup label, where free(leaseFiles) is called, but ...
leaseFiles = tmpLease;
... this ^^ was never executed. Thus there's still a memleak except this time a different one.
leaseFiles[nleaseFiles++] = path; #if defined(LIBVIRT_NSS_GUEST)
What we really need is: tmpLease = realloc(leaseFiles, ...); if (!tmpLease) goto cleanup; leaseFiles = tmpLease; if (asprintf(&path, ...) < 0) goto cleanup; leaseFiles[nleaseFiles++] = path; I'm changing your patch accordingly and merging. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Alexander Kuznetsov -
Daniel P. Berrangé -
Michal Prívozník -
Peter Krempa