| Message ID | 1613645919-15705-1-git-send-email-ayush.m@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] libiberty(argv.c): Fix memory leak in expandargv. | expand |
On Thu, 18 Feb 2021 at 16:32, Ayush Mittal via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Dynamic memory referenced by 'buffer' was allocated using xmalloc but fails to free it > when jump to 'error' label. > > Issue as per static analysis tool. > > Signed-off-by: Ayush Mittal <ayush.m@samsung.com> > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > --- > libiberty/ChangeLog | 4 ++++ > libiberty/argv.c | 5 ++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog > index e472553..96cacba 100644 > --- a/libiberty/ChangeLog > +++ b/libiberty/ChangeLog > @@ -1,3 +1,7 @@ > +2021-02-18 Ayush Mittal <ayush.m@samsung.com> > + > + * argv.c (expandargv): Fix memory leak for buffer allocated to read file contents. AFAIK, the policy has changed to not include ChangeLog in patches anymore. Instead the ChangeLog bits should be written in commit messages, and the ChangeLog file is updated automatically based on that. > + > 2021-02-01 Martin Sebor <msebor@redhat.com> > > * dyn-string.c (dyn_string_insert_cstr): Use memcpy instead of strncpy > diff --git a/libiberty/argv.c b/libiberty/argv.c > index cd97f90..7199c7d 100644 > --- a/libiberty/argv.c > +++ b/libiberty/argv.c > @@ -442,7 +442,10 @@ expandargv (int *argcp, char ***argvp) > due to CR/LF->CR translation when reading text files. > That does not in-and-of itself indicate failure. */ > && ferror (f)) > - goto error; > + { > + free(buffer); > + goto error; > + } Sorry to nitpick for formatting issues - IIRC, the convention is to use free (buffer) with space between function name and "(". As a general note, please run patches thru contrib/check_GNU_style.py. The patch looks OK to me, but I can't approve. Thanks, Prathamesh > /* Add a NUL terminator. */ > buffer[len] = '\0'; > /* If the file is empty or contains only whitespace, buildargv would > -- > 1.9.1 >
On 2/18/21 3:58 AM, Ayush Mittal via Gcc-patches wrote: > Dynamic memory referenced by 'buffer' was allocated using xmalloc but fails to free it > when jump to 'error' label. > > Issue as per static analysis tool. The change looks okay to me although I can't approve it. Since GCC is a regression fixing stage, unless the leak is a recent regression the fix is (strictly speaking) out of scope for GCC 11. Either a libiberty or a global maintainer might be comfortable approving it regardless. That said, rather than adding another call to free, I would suggest to consider initializing buffer to null and moving the existing call to free the buffer under the error: label. Martin > > Signed-off-by: Ayush Mittal <ayush.m@samsung.com> > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > --- > libiberty/ChangeLog | 4 ++++ > libiberty/argv.c | 5 ++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog > index e472553..96cacba 100644 > --- a/libiberty/ChangeLog > +++ b/libiberty/ChangeLog > @@ -1,3 +1,7 @@ > +2021-02-18 Ayush Mittal <ayush.m@samsung.com> > + > + * argv.c (expandargv): Fix memory leak for buffer allocated to read file contents. > + > 2021-02-01 Martin Sebor <msebor@redhat.com> > > * dyn-string.c (dyn_string_insert_cstr): Use memcpy instead of strncpy > diff --git a/libiberty/argv.c b/libiberty/argv.c > index cd97f90..7199c7d 100644 > --- a/libiberty/argv.c > +++ b/libiberty/argv.c > @@ -442,7 +442,10 @@ expandargv (int *argcp, char ***argvp) > due to CR/LF->CR translation when reading text files. > That does not in-and-of itself indicate failure. */ > && ferror (f)) > - goto error; > + { > + free(buffer); > + goto error; > + } > /* Add a NUL terminator. */ > buffer[len] = '\0'; > /* If the file is empty or contains only whitespace, buildargv would >
Hi Martin, >> Dynamic memory referenced by 'buffer' was allocated using xmalloc but fails to free it >> when jump to 'error' label. >> >> Issue as per static analysis tool. > >The change looks okay to me although I can't approve it. Since GCC >is a regression fixing stage, unless the leak is a recent regression >the fix is (strictly speaking) out of scope for GCC 11. Either >a libiberty or a global maintainer might be comfortable approving it >regardless. OK >That said, rather than adding another call to free, I would suggest >to consider initializing buffer to null and moving the existing call >to free the buffer under the error: label. > We thought same, but then it will add un-necessary call to free in case of earlier 3 goto cases and thus impact code's readability (going for free without allocating). if (fseek (f, 0L, SEEK_END) == -1) goto error; pos = ftell (f); if (pos == -1) goto error; if (fseek (f, 0L, SEEK_SET) == -1) goto error; thats why we tried with current change. Other option was to create one more label in case of free. Thanks, Maninder Singh >Martin > >> >> Signed-off-by: Ayush Mittal <ayush.m@samsung.com> >> Signed-off-by: Maninder Singh <maninder1.s@samsung.com> >> --- > libiberty/ChangeLog | 4 ++++ > libiberty/argv.c | 5 ++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog > index e472553..96cacba 100644 > --- a/libiberty/ChangeLog > +++ b/libiberty/ChangeLog > @@ -1,3 +1,7 @@ > +2021-02-18 Ayush Mittal <ayush.m@samsung.com> > + > + * argv.c (expandargv): Fix memory leak for buffer allocated to read file contents. > + > 2021-02-01 Martin Sebor <msebor@redhat.com> > > * dyn-string.c (dyn_string_insert_cstr): Use memcpy instead of strncpy > diff --git a/libiberty/argv.c b/libiberty/argv.c > index cd97f90..7199c7d 100644 > --- a/libiberty/argv.c > +++ b/libiberty/argv.c > @@ -442,7 +442,10 @@ expandargv (int *argcp, char ***argvp) > due to CR/LF->CR translation when reading text files. > That does not in-and-of itself indicate failure. */ > && ferror (f)) > - goto error; > + { > + free(buffer); > + goto error; > + } > /* Add a NUL terminator. */ > buffer[len] = '\0'; > /* If the file is empty or contains only whitespace, buildargv would >
diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog index e472553..96cacba 100644 --- a/libiberty/ChangeLog +++ b/libiberty/ChangeLog @@ -1,3 +1,7 @@ +2021-02-18 Ayush Mittal <ayush.m@samsung.com> + + * argv.c (expandargv): Fix memory leak for buffer allocated to read file contents. + 2021-02-01 Martin Sebor <msebor@redhat.com> * dyn-string.c (dyn_string_insert_cstr): Use memcpy instead of strncpy diff --git a/libiberty/argv.c b/libiberty/argv.c index cd97f90..7199c7d 100644 --- a/libiberty/argv.c +++ b/libiberty/argv.c @@ -442,7 +442,10 @@ expandargv (int *argcp, char ***argvp) due to CR/LF->CR translation when reading text files. That does not in-and-of itself indicate failure. */ && ferror (f)) - goto error; + { + free(buffer); + goto error; + } /* Add a NUL terminator. */ buffer[len] = '\0'; /* If the file is empty or contains only whitespace, buildargv would