diff mbox series

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

Message ID CAHyHGCn4Gk3_pB8K1SsXNzCyWMVGGs4jOtO1_L=L+qBpkDtqHg@mail.gmail.com
State New
Headers
Series libiberty: fix memory leak in pex-win32.c and refactor |

Commit Message

Costas Argyris March 1, 2023, 6:07 p.m. UTC 
  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

Comments

Richard Biener March 2, 2023, 7:32 a.m. UTC | #1 
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
Costas Argyris March 2, 2023, 9:21 a.m. UTC | #2 
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
>
Richard Biener March 2, 2023, 10:08 a.m. UTC | #3 
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
Costas Argyris March 2, 2023, 2:02 p.m. UTC | #4 
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
>
Richard Biener March 3, 2023, 11:25 a.m. UTC | #5 
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
diff mbox series

Patch

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