[1/1] libiberty(argv.c): Fix memory leak in expandargv.

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.
Related show

Commit Message

Marek Polacek via Gcc-patches Feb. 18, 2021, 10:58 a.m.
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(-)

-- 
1.9.1

Comments

Marek Polacek via Gcc-patches Feb. 18, 2021, 11:34 a.m. | #1
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

>
Marek Polacek via Gcc-patches Feb. 18, 2021, 5:04 p.m. | #2
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

>
Marek Polacek via Gcc-patches Feb. 19, 2021, 3:31 a.m. | #3
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

>

Patch

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