Message ID | jlgfsvyib54.fsf@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v1] nis: Fix leak on realloc failure in nis_getnames | expand |
* Robbie Harwood via Libc-alpha: > From 7aa0a9f879d5b2117beb06771bb4fdbaf25699a9 Mon Sep 17 00:00:00 2001 > From: Robbie Harwood <rharwood@redhat.com> > Date: Wed, 28 Jul 2021 12:54:44 -0400 > Subject: [PATCH v1] nis: Fix leak on realloc failure in nis_getnames > To: libc-alpha@sourceware.org > Cc: kukuk@suse.de > > If pos >= count but realloc fails, tmp will not have been placed in > getnames[pos] yet, and so will not be freed in free_null. Detected > by Coverity. > > Also remove misleading comment from nis_getnames(), since it actually > did properly release getnames when out of memory. > --- > nis/nis_subr.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/nis/nis_subr.c b/nis/nis_subr.c > index dd0e30071d..6784fc353f 100644 > --- a/nis/nis_subr.c > +++ b/nis/nis_subr.c > @@ -103,9 +103,6 @@ count_dots (const_nis_name str) > return count; > } > > -/* If we run out of memory, we don't give already allocated memory > - free. The overhead for bringing getnames back in a safe state to > - free it is to big. */ > nis_name * > nis_getnames (const_nis_name name) > { > @@ -271,7 +268,10 @@ nis_getnames (const_nis_name name) > nis_name *newp = realloc (getnames, > (count + 1) * sizeof (char *)); > if (__glibc_unlikely (newp == NULL)) > - goto free_null; > + { > + free (tmp); > + goto free_null; > + } > getnames = newp; > } > getnames[pos] = tmp; The patch looks correct to me. (There some similar code above, but it is already correct because the tmp ownership transfer is different.) Would you please open a bug in Bugzilla and reference it in the commit message. I don't think we need to treat this as a security vulnerability (denial of service) because since bug only happens after a memory allocation failure, and at that point, the service is already denied, so to speak. Thanks, Florian
From 7aa0a9f879d5b2117beb06771bb4fdbaf25699a9 Mon Sep 17 00:00:00 2001 From: Robbie Harwood <rharwood@redhat.com> Date: Wed, 28 Jul 2021 12:54:44 -0400 Subject: [PATCH v1] nis: Fix leak on realloc failure in nis_getnames To: libc-alpha@sourceware.org Cc: kukuk@suse.de If pos >= count but realloc fails, tmp will not have been placed in getnames[pos] yet, and so will not be freed in free_null. Detected by Coverity. Also remove misleading comment from nis_getnames(), since it actually did properly release getnames when out of memory. --- nis/nis_subr.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nis/nis_subr.c b/nis/nis_subr.c index dd0e30071d..6784fc353f 100644 --- a/nis/nis_subr.c +++ b/nis/nis_subr.c @@ -103,9 +103,6 @@ count_dots (const_nis_name str) return count; } -/* If we run out of memory, we don't give already allocated memory - free. The overhead for bringing getnames back in a safe state to - free it is to big. */ nis_name * nis_getnames (const_nis_name name) { @@ -271,7 +268,10 @@ nis_getnames (const_nis_name name) nis_name *newp = realloc (getnames, (count + 1) * sizeof (char *)); if (__glibc_unlikely (newp == NULL)) - goto free_null; + { + free (tmp); + goto free_null; + } getnames = newp; } getnames[pos] = tmp; -- 2.32.0