[PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[Costas Argyris]

[-- Attachment #1.1: Type: text/plain, Size: 425 bytes --]

Hi

It seems that the win32_spawn function in libiberty/pex-win32.c is leaking
the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).    The
problem here is that the cleanup code is written 3 times, one at each exit
scenario.

The proposed attached refactoring has the cleanup code appearing just once
and is executed for all exit scenarios, reducing the likelihood of such
leaks in the future.

Thanks,
Costas

[-- Attachment #2: 0001-libiberty-fix-memory-leak-in-pex-win32.c-and-refacto.patch --]
[-- Type: text/x-patch, Size: 2115 bytes --]

From ca1ff7b48db2e30963bd4c0e08ecd6653214a5db Mon Sep 17 00:00:00 2001
From: Costas Argyris <costas.argyris@gmail.com>
Date: Sun, 26 Feb 2023 16:34:11 +0000
Subject: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor

Fix memory leak of cmdline buffer and refactor to have
cleanup code appear once for all exit cases.
---
 libiberty/pex-win32.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c
index 02d3a3e839b..82303947de4 100644
--- a/libiberty/pex-win32.c
+++ b/libiberty/pex-win32.c
@@ -577,14 +577,12 @@ win32_spawn (const char *executable,
 	     LPSTARTUPINFO si,
 	     LPPROCESS_INFORMATION pi)
 {
-  char *full_executable;
-  char *cmdline;
+  char *full_executable = NULL;
+  char *cmdline = NULL;
+  pid_t pid = (pid_t) -1;
   char **env_copy;
   char *env_block = NULL;
 
-  full_executable = NULL;
-  cmdline = NULL;
-
   if (env)
     {
       int env_size;
@@ -622,13 +620,13 @@ win32_spawn (const char *executable,
 
   full_executable = find_executable (executable, search);
   if (!full_executable)
-    goto error;
+    goto exit;
   cmdline = argv_to_cmdline (argv);
   if (!cmdline)
-    goto error;
+    goto exit;
     
   /* Create the child process.  */  
-  if (!CreateProcess (full_executable, cmdline, 
+  if (CreateProcess (full_executable, cmdline, 
 		      /*lpProcessAttributes=*/NULL,
 		      /*lpThreadAttributes=*/NULL,
 		      /*bInheritHandles=*/TRUE,
@@ -638,26 +636,17 @@ win32_spawn (const char *executable,
 		      si,
 		      pi))
     {
-      free (env_block);
-
-      free (full_executable);
-
-      return (pid_t) -1;
+      CloseHandle (pi->hThread);
+      pid = (pid_t) pi->hProcess;
     }
-
+  
+ exit:
   /* Clean up.  */
-  CloseHandle (pi->hThread);
-  free (full_executable);
-  free (env_block);
-
-  return (pid_t) pi->hProcess;
-
- error:
   free (env_block);
   free (cmdline);
   free (full_executable);
 
-  return (pid_t) -1;
+  return pid;
 }
 
 /* Spawn a script.  This simulates the Unix script execution mechanism.
-- 
2.30.2

Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[Richard Biener]

On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi
>
> It seems that the win32_spawn function in libiberty/pex-win32.c is leaking
> the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).    The
> problem here is that the cleanup code is written 3 times, one at each exit
> scenario.
>
> The proposed attached refactoring has the cleanup code appearing just once
> and is executed for all exit scenarios, reducing the likelihood of such
> leaks in the future.

One could imagine that CreateProcess in case of success takes ownership of
the buffer pointed to by cmdline?  If you can confirm it is not then the patch
looks OK to me.

Thanks,
Richard.

> Thanks,
> Costas

Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[Costas Argyris]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

I forgot to mention that:

1) The CreateProcess documentation

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa

doesn't mention anything about taking ownership of this or any other buffer
passed to it.

2) The cmdline buffer gets created by the argv_to_cmdline function

https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L339

which has this comment right above it:

/* Return a Windows command-line from ARGV.  It is the caller's
   responsibility to free the string returned.  */

Thanks,
Costas

On Thu, 2 Mar 2023 at 07:32, Richard Biener <richard.guenther@gmail.com>
wrote:

> On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > Hi
> >
> > It seems that the win32_spawn function in libiberty/pex-win32.c is
> leaking
> > the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).
> The
> > problem here is that the cleanup code is written 3 times, one at each
> exit
> > scenario.
> >
> > The proposed attached refactoring has the cleanup code appearing just
> once
> > and is executed for all exit scenarios, reducing the likelihood of such
> > leaks in the future.
>
> One could imagine that CreateProcess in case of success takes ownership of
> the buffer pointed to by cmdline?  If you can confirm it is not then the
> patch
> looks OK to me.
>
> Thanks,
> Richard.
>
> > Thanks,
> > Costas
>

Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[Richard Biener]

On Thu, Mar 2, 2023 at 10:21 AM Costas Argyris <costas.argyris@gmail.com> wrote:
>
> I forgot to mention that:
>
> 1) The CreateProcess documentation
>
> https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa
>
> doesn't mention anything about taking ownership of this or any other buffer passed to it.

Thanks - thus the patch is OK.

Thanks,
Richard.

> 2) The cmdline buffer gets created by the argv_to_cmdline function
>
> https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L339
>
> which has this comment right above it:
>
> /* Return a Windows command-line from ARGV.  It is the caller's
>    responsibility to free the string returned.  */
>
> Thanks,
> Costas
>
> On Thu, 2 Mar 2023 at 07:32, Richard Biener <richard.guenther@gmail.com> wrote:
>>
>> On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>> >
>> > Hi
>> >
>> > It seems that the win32_spawn function in libiberty/pex-win32.c is leaking
>> > the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).    The
>> > problem here is that the cleanup code is written 3 times, one at each exit
>> > scenario.
>> >
>> > The proposed attached refactoring has the cleanup code appearing just once
>> > and is executed for all exit scenarios, reducing the likelihood of such
>> > leaks in the future.
>>
>> One could imagine that CreateProcess in case of success takes ownership of
>> the buffer pointed to by cmdline?  If you can confirm it is not then the patch
>> looks OK to me.
>>
>> Thanks,
>> Richard.
>>
>> > Thanks,
>> > Costas

Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[Costas Argyris]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

Thanks for the review.

What is the next step please?

Thanks,
Costas

On Thu, 2 Mar 2023 at 10:08, Richard Biener <richard.guenther@gmail.com>
wrote:

> On Thu, Mar 2, 2023 at 10:21 AM Costas Argyris <costas.argyris@gmail.com>
> wrote:
> >
> > I forgot to mention that:
> >
> > 1) The CreateProcess documentation
> >
> >
> https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa
> >
> > doesn't mention anything about taking ownership of this or any other
> buffer passed to it.
>
> Thanks - thus the patch is OK.
>
> Thanks,
> Richard.
>
> > 2) The cmdline buffer gets created by the argv_to_cmdline function
> >
> > https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L339
> >
> > which has this comment right above it:
> >
> > /* Return a Windows command-line from ARGV.  It is the caller's
> >    responsibility to free the string returned.  */
> >
> > Thanks,
> > Costas
> >
> > On Thu, 2 Mar 2023 at 07:32, Richard Biener <richard.guenther@gmail.com>
> wrote:
> >>
> >> On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches
> >> <gcc-patches@gcc.gnu.org> wrote:
> >> >
> >> > Hi
> >> >
> >> > It seems that the win32_spawn function in libiberty/pex-win32.c is
> leaking
> >> > the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).
>   The
> >> > problem here is that the cleanup code is written 3 times, one at each
> exit
> >> > scenario.
> >> >
> >> > The proposed attached refactoring has the cleanup code appearing just
> once
> >> > and is executed for all exit scenarios, reducing the likelihood of
> such
> >> > leaks in the future.
> >>
> >> One could imagine that CreateProcess in case of success takes ownership
> of
> >> the buffer pointed to by cmdline?  If you can confirm it is not then
> the patch
> >> looks OK to me.
> >>
> >> Thanks,
> >> Richard.
> >>
> >> > Thanks,
> >> > Costas
>

Re: [PATCH] libiberty: fix memory leak in pex-win32.c and refactor

[Richard Biener]

On Thu, Mar 2, 2023 at 3:02 PM Costas Argyris <costas.argyris@gmail.com> wrote:
>
> Thanks for the review.
>
> What is the next step please?

Somebody pushed the patch already.

Richard.

> Thanks,
> Costas
>
> On Thu, 2 Mar 2023 at 10:08, Richard Biener <richard.guenther@gmail.com> wrote:
>>
>> On Thu, Mar 2, 2023 at 10:21 AM Costas Argyris <costas.argyris@gmail.com> wrote:
>> >
>> > I forgot to mention that:
>> >
>> > 1) The CreateProcess documentation
>> >
>> > https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa
>> >
>> > doesn't mention anything about taking ownership of this or any other buffer passed to it.
>>
>> Thanks - thus the patch is OK.
>>
>> Thanks,
>> Richard.
>>
>> > 2) The cmdline buffer gets created by the argv_to_cmdline function
>> >
>> > https://github.com/gcc-mirror/gcc/blob/master/libiberty/pex-win32.c#L339
>> >
>> > which has this comment right above it:
>> >
>> > /* Return a Windows command-line from ARGV.  It is the caller's
>> >    responsibility to free the string returned.  */
>> >
>> > Thanks,
>> > Costas
>> >
>> > On Thu, 2 Mar 2023 at 07:32, Richard Biener <richard.guenther@gmail.com> wrote:
>> >>
>> >> On Wed, Mar 1, 2023 at 7:14 PM Costas Argyris via Gcc-patches
>> >> <gcc-patches@gcc.gnu.org> wrote:
>> >> >
>> >> > Hi
>> >> >
>> >> > It seems that the win32_spawn function in libiberty/pex-win32.c is leaking
>> >> > the cmdline buffer in 2/3 exit scenarios (it is only free'd in 1/3).    The
>> >> > problem here is that the cleanup code is written 3 times, one at each exit
>> >> > scenario.
>> >> >
>> >> > The proposed attached refactoring has the cleanup code appearing just once
>> >> > and is executed for all exit scenarios, reducing the likelihood of such
>> >> > leaks in the future.
>> >>
>> >> One could imagine that CreateProcess in case of success takes ownership of
>> >> the buffer pointed to by cmdline?  If you can confirm it is not then the patch
>> >> looks OK to me.
>> >>
>> >> Thanks,
>> >> Richard.
>> >>
>> >> > Thanks,
>> >> > Costas