Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in backticks and system #13741

Closed
p5pRT opened this issue Apr 17, 2014 · 25 comments
Closed

Memory leak in backticks and system #13741

p5pRT opened this issue Apr 17, 2014 · 25 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 17, 2014

Migrated from rt.perl.org#121676 (status was 'resolved')

Searchable as RT121676$

@p5pRT
Copy link
Author

p5pRT commented Apr 17, 2014

From @bulk88

Created by @bulk88

There is a mem leak in system and `` on Win32 on 5.19 and 5.18 (tested
by me and others), see
https://rt.cpan.org/Public/Bug/Display.html?id=94138 and
http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214390.html
. This ticket is so the issue doesnt get lost.

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.19.10:

Configured by Owner at Sat Mar 15 22:30:42 2014.

Summary of my perl5 (revision 5 version 19 subversion 10) configuration:
  Derived from: 2179658b5e799a6e3c4e736ec7c84b0f50bf3473
  Ancestor: e9251c1a8f4944e6dceff5240d9e109ba075ff29
  Platform:
    osname=MSWin32, osvers=5.1, archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended, useposix=true, d_sigaction=undef
    useithreads=define, usemultiplicity=define
    use64bitint=undef, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cl', ccflags ='-nologo -GF -W3 -O1 -MD -Zi -DNDEBUG -G7 -GL 
-DWIN32 -D_CONSOLE -DNO_STRICT  -DPERL_TEXTMODE_SCRIPTS 
-DPERL_HASH_FUNC_ONE_AT_A_TIME -DPERL_IMPLICIT_CONTEXT 
-DPERL_IMPLICIT_SYS -DUSE_PERLIO -D_USE_32BIT_TIME_T',
    optimize='-O1 -MD -Zi -DNDEBUG -G7 -GL',
    cppflags='-DWIN32'
    ccversion='13.10.6030', gccversion='', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=undef, longlongsize=8, d_longdbl=define, longdblsize=8
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='__int64', 
lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='link', ldflags ='-nologo -nodefaultlib -debug -opt:ref,icf 
-ltcg  -libpath:"c:\perl519\lib\CORE"  -machine:x86'
    libpth="C:\Program Files\Microsoft Visual Studio .NET 2003\VC7\lib"
    libs=oldnames.lib kernel32.lib user32.lib gdi32.lib winspool.lib  
comdlg32.lib advapi32.lib shell32.lib ole32.lib oleaut32.lib  
netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  version.lib 
odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    perllibs=oldnames.lib kernel32.lib user32.lib gdi32.lib 
winspool.lib  comdlg32.lib advapi32.lib shell32.lib ole32.lib 
oleaut32.lib  netapi32.lib uuid.lib ws2_32.lib mpr.lib winmm.lib  
version.lib odbc32.lib odbccp32.lib comctl32.lib msvcrt.lib
    libc=msvcrt.lib, so=dll, useshrplib=true, libperl=perl519.lib
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
    cccdlflags=' ', lddlflags='-dll -nologo -nodefaultlib -debug 
-opt:ref,icf -ltcg  -libpath:"c:\perl519\lib\CORE"  -machine:x86'

Locally applied patches:
    uncommitted-changes
    2179658b5e799a6e3c4e736ec7c84b0f50bf3473


@INC for perl 5.19.10:
    C:/perl519/site/lib
    C:/perl519/lib
    .


Environment for perl 5.19.10:
    HOME (unset)
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=C:\perl519\bin;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\IDE;C:\Program Files\Microsoft Visual Studio .NET 
2003\VC7\BIN;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools;C:\Program Files\Microsoft Visual Studio .NET 
2003\Common7\Tools\bin\prerelease;C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\system32\wbem;
    PERL_BADLANG (unset)
    SHELL (unset)


@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2014

From @tonycoz

On Wed Apr 16 23​:37​:39 2014, bulk88 wrote​:

There is a mem leak in system and `` on Win32 on 5.19 and 5.18 (tested
by me and others), see
https://rt.cpan.org/Public/Bug/Display.html?id=94138 and
http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214390.html
. This ticket is so the issue doesnt get lost.

I tried to fudge the build (and added some extra code) to build perl with MSVCRT's debug version memory leak reporting[1], which reported only a single leak, which appears with perl -e0 or perl -e "`dir` for 1..100`".

I don't see a leak if I build perl with PERL_MALLOC defined (which requires disabling USE_IMP_SYS).

I don't see a leak with all of PERL_MALLOC, USE_MULTI, USE_ITHREADS, USE_IMP_SYS undefined.

No leak with just USE_MULTI re-enabled out of those.

Right now I suspect something in the USE_IMP_SYS layer.

Tony

[1] http​://msdn.microsoft.com/en-us/library/e5ewb1h3%28v=vs.90%29.aspx

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2014

From @bulk88

On Mon Apr 21 23​:33​:16 2014, tonyc wrote​:

I tried to fudge the build (and added some extra code) to build perl
with MSVCRT's debug version memory leak reporting[1], which reported
only a single leak, which appears with perl -e0 or perl -e "`dir` for
1..100`".

I've never used debugging CRT with Perl. Its on my very long todo list. Since you got it to compile, may I suggest you implement the feature in the makefiles?

I've always thought that PERL_DESTRUT_LEVEL and debugging CRT would conflict since perl doesn't free all memory on normal exists unless psuedo-fork has been called which sets PERL_DESTRUCT_LEVEL.

I don't see a leak if I build perl with PERL_MALLOC defined (which
requires disabling USE_IMP_SYS).

I don't see a leak with all of PERL_MALLOC, USE_MULTI, USE_ITHREADS,
USE_IMP_SYS undefined.

No leak with just USE_MULTI re-enabled out of those.

Right now I suspect something in the USE_IMP_SYS layer.

Did you see the leak in any config? I saw some strange reproducibility which had no identifyable pattern, since it wasn't tied to CRT dll or compiler, in http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214405.html . The leak can be clearly seen in Task Manager. Its about 200-300 KB a second/a screen refresh in task manager.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2014

From @tonycoz

On Tue, Apr 22, 2014 at 04​:41​:00AM -0700, bulk88 via RT wrote​:

On Mon Apr 21 23​:33​:16 2014, tonyc wrote​:

I tried to fudge the build (and added some extra code) to build perl
with MSVCRT's debug version memory leak reporting[1], which reported
only a single leak, which appears with perl -e0 or perl -e "`dir` for
1..100`".

I've never used debugging CRT with Perl. Its on my very long todo list. Since you got it to compile, may I suggest you implement the feature in the makefiles?

I had to fiddle with the source a bit, the macro in perl.h​:

#define CALLREGFREE_PVT(prog) \
  if(prog) RX_ENGINE(prog)->free(aTHX_ (prog))

fails pretty hard when the CRT defines a free() macro.

Also, I could only get the results to show in the debugger, not on the
console.

I've always thought that PERL_DESTRUT_LEVEL and debugging CRT would conflict since perl doesn't free all memory on normal exists unless psuedo-fork has been called which sets PERL_DESTRUCT_LEVEL.

You can build a -DDEBUGGING perl and set PERL_DESTRUCT_LEVEL=2 in the
environment.

I don't see a leak if I build perl with PERL_MALLOC defined (which
requires disabling USE_IMP_SYS).

I don't see a leak with all of PERL_MALLOC, USE_MULTI, USE_ITHREADS,
USE_IMP_SYS undefined.

No leak with just USE_MULTI re-enabled out of those.

Right now I suspect something in the USE_IMP_SYS layer.

Did you see the leak in any config? I saw some strange reproducibility which had no identifyable pattern, since it wasn't tied to CRT dll or compiler, in http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214405.html . The leak can be clearly seen in Task Manager. Its about 200-300 KB a second/a screen refresh in task manager.

A USE_IMP_SYS build did leak.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2014

From @steve-m-hay

On 22 April 2014 15​:01, Tony Cook <tony@​develop-help.com> wrote​:

On Tue, Apr 22, 2014 at 04​:41​:00AM -0700, bulk88 via RT wrote​:

On Mon Apr 21 23​:33​:16 2014, tonyc wrote​:

I tried to fudge the build (and added some extra code) to build perl
with MSVCRT's debug version memory leak reporting[1], which reported
only a single leak, which appears with perl -e0 or perl -e "`dir` for
1..100`".

I've never used debugging CRT with Perl. Its on my very long todo list. Since you got it to compile, may I suggest you implement the feature in the makefiles?

I had to fiddle with the source a bit, the macro in perl.h​:

#define CALLREGFREE_PVT(prog) \
if(prog) RX_ENGINE(prog)->free(aTHX_ (prog))

fails pretty hard when the CRT defines a free() macro.

Also, I could only get the results to show in the debugger, not on the
console.

I had a crack at using the debug CRT a year or more ago and ran into
the free() problem too, and also trouble with the invalid parameter
handler choking on filehandles getting closed when they are already
closed. I have it on my TODO list to revisit this and get it working
too; maybe the three of us can finally get somewhere with it after
5.20 is out?!

FWIW, here's the patch that I was playing with last. It's well over a
year out of date, but I'm sure it could still be manually applied if
all else fails. (Sorry, it's also git diff style again. I've not
forgotten to avoid this in the future...)

@p5pRT
Copy link
Author

p5pRT commented Apr 22, 2014

From @steve-m-hay

debug.patch
diff --git a/miniperlmain.c b/miniperlmain.c
index 7f63a34..41e8f9f 100644
--- a/miniperlmain.c
+++ b/miniperlmain.c
@@ -75,6 +75,11 @@ main(int argc, char **argv, char **env)
     PL_use_safe_putenv = FALSE;
 #endif /* PERL_USE_SAFE_PUTENV */
 
+#ifdef _MSC_VER
+    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
+    _CrtSetBreakAlloc(-1L);
+#endif
+
     /* if user wants control of gprof profiling off by default */
     /* noop unless Configure is given -Accflags=-DPERL_GPROF_CONTROL */
     PERL_GPROF_MONCONTROL(0);
diff --git a/perl.h b/perl.h
index 2cc4e91..ab8b270 100644
--- a/perl.h
+++ b/perl.h
@@ -237,7 +237,7 @@
     Perl_pregfree(aTHX_ (prog))
 
 #define CALLREGFREE_PVT(prog) \
-    if(prog) RX_ENGINE(prog)->free(aTHX_ (prog))
+    if(prog) RX_ENGINE(prog)->freexxx(aTHX_ (prog))
 
 #define CALLREG_NUMBUF_FETCH(rx,paren,usesv)                                \
     RX_ENGINE(rx)->numbered_buff_FETCH(aTHX_ (rx),(paren),(usesv))
@@ -671,11 +671,15 @@ register struct op *Perl_op asm(stringify(OP_IN_REGISTER));
 #   include <sys/param.h>
 #endif
 
+#define _CRTDBG_MAP_ALLOC
+
 /* Use all the "standard" definitions? */
 #if defined(STANDARD_C) && defined(I_STDLIB)
 #   include <stdlib.h>
 #endif
 
+#include <crtdbg.h>
+
 /* If this causes problems, set i_unistd=undef in the hint file.  */
 #ifdef I_UNISTD
 #   include <unistd.h>
diff --git a/regcomp.c b/regcomp.c
index 921c0e9..e03a283 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -13896,7 +13896,7 @@ Perl_re_intuit_string(pTHX_ REGEXP * const r)
    
    handles refcounting and freeing the perl core regexp structure. When 
    it is necessary to actually free the structure the first thing it 
-   does is call the 'free' method of the regexp_engine associated to
+   does is call the 'freexxx' method of the regexp_engine associated to
    the regexp, allowing the handling of the void *pprivate; member 
    first. (This routine is not overridable by extensions, which is why 
    the extensions free is called first.)
diff --git a/regexp.h b/regexp.h
index db36edd..ea6cb63 100644
--- a/regexp.h
+++ b/regexp.h
@@ -155,7 +155,7 @@ typedef struct regexp_engine {
                        char *strend, const U32 flags,
                        re_scream_pos_data *data);
     SV*     (*checkstr) (pTHX_ REGEXP * const rx);
-    void    (*free) (pTHX_ REGEXP * const rx);
+    void    (*freexxx) (pTHX_ REGEXP * const rx);
     void    (*numbered_buff_FETCH) (pTHX_ REGEXP * const rx, const I32 paren,
                                     SV * const sv);
     void    (*numbered_buff_STORE) (pTHX_ REGEXP * const rx, const I32 paren,
diff --git a/win32/Makefile b/win32/Makefile
index 0edfd15..18a19dc 100644
--- a/win32/Makefile
+++ b/win32/Makefile
@@ -414,12 +414,12 @@ LOCDEFS		= -DPERLDLL -DPERL_CORE
 SUBSYS		= console
 CXX_FLAG	= -TP -EHsc
 
-LIBC	= msvcrt.lib
-
 !IF  "$(CFG)" == "Debug"
-OPTIMIZE	= -Od -MD -Zi -DDEBUGGING
+LIBC		= msvcrtd.lib
+OPTIMIZE	= -Od -MDd -Zi -D_DEBUG -DDEBUGGING
 LINK_DBG	= -debug
 !ELSE
+LIBC		= msvcrt.lib
 OPTIMIZE	= -MD -Zi -DNDEBUG
 # we enable debug symbols in release builds also
 LINK_DBG	= -debug -opt:ref,icf
diff --git a/win32/runperl.c b/win32/runperl.c
index b76f8ba..d8e017e 100644
--- a/win32/runperl.c
+++ b/win32/runperl.c
@@ -20,6 +20,10 @@ int _CRT_glob = 0;
 int
 main(int argc, char **argv, char **env)
 {
+#ifdef _MSC_VER
+    _CrtSetDbgFlag(_CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF);
+    _CrtSetBreakAlloc(95L);
+#endif
     return RunPerl(argc, argv, env);
 }
 
diff --git a/win32/win32.c b/win32/win32.c
index 78b55f6..259c1bb 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -150,7 +150,7 @@ void my_invalid_parameter_handler(const wchar_t* expression,
     unsigned int line, 
     uintptr_t pReserved)
 {
-#  ifdef _DEBUG
+#  ifdef _DEBUGxxx
     wprintf(L"Invalid parameter detected in function %s."
             L" File: %s Line: %d\n", function, file, line);
     wprintf(L"Expression: %s\n", expression);

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @bulk88

On Wed Apr 16 23​:37​:39 2014, bulk88 wrote​:

There is a mem leak in system and `` on Win32 on 5.19 and 5.18 (tested
by me and others), see
https://rt.cpan.org/Public/Bug/Display.html?id=94138 and
http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214390.html
. This ticket is so the issue doesnt get lost.

Isolated to


C​:\Documents and Settings\Administrator\Desktop\lxs\Local-XS>perl -MLocal​::XS -
e"while(1){Local​::XS​::runleak2()}"


void
runleak2()
CODE​:
  void* env;
  env = PerlEnv_get_childenv();
  PerlEnv_free_childenv(env);


eats 100 MB of mem a sec.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @steve-m-hay

-----Original Message-----
From​: bulk88 via RT [mailto​:perlbug-followup@​perl.org]
Sent​: 13 May 2014 12​:03
Cc​: perl5-porters@​perl.org
Subject​: [perl #121676] Memory leak in backticks and system

On Wed Apr 16 23​:37​:39 2014, bulk88 wrote​:

There is a mem leak in system and `` on Win32 on 5.19 and 5.18
(tested
by me and others), see
https://rt.cpan.org/Public/Bug/Display.html?id=94138 and

http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214390.ht

ml . This ticket is so the issue doesnt get lost.

Isolated to

---------------------------------
C​:\Documents and Settings\Administrator\Desktop\lxs\Local-XS>perl -
MLocal​::XS - e"while(1){Local​::XS​::runleak2()}"
---------------------------------
void
runleak2()
CODE​:
void* env;
env = PerlEnv_get_childenv();
PerlEnv_free_childenv(env);
---------------------------------

eats 100 MB of mem a sec.

At a quick look win32_freeenvironmentstrings appears to be missing a FreeEnvironmentStrings() for the GetEnvironmentStringsW() in win32_getenvironmentstrings()? (win32_freeenvironmentstrings() only win32_free()s what was win32_calloc()ed...)

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @bulk88

On Tue May 13 04​:02​:44 2014, bulk88 wrote​:

On Wed Apr 16 23​:37​:39 2014, bulk88 wrote​:

There is a mem leak in system and `` on Win32 on 5.19 and 5.18
(tested
by me and others), see
https://rt.cpan.org/Public/Bug/Display.html?id=94138 and
http​://www.nntp.perl.org/group/perl.perl5.porters/2014/04/msg214390.html
. This ticket is so the issue doesnt get lost.

Isolated to

---------------------------------
C​:\Documents and Settings\Administrator\Desktop\lxs\Local-XS>perl
-MLocal​::XS -
e"while(1){Local​::XS​::runleak2()}"
---------------------------------
void
runleak2()
CODE​:
void* env;
env = PerlEnv_get_childenv();
PerlEnv_free_childenv(env);
---------------------------------

eats 100 MB of mem a sec.

win32_getenvironmentstrings in win32.c is probably missing a call to FreeEnvironmentStrings. The leak probably started in http​://perl5.git.perl.org/perl.git/commit/4f46e52b008bf955ea3049ad2dc4bfe468842f06 from 5.17.0-1, so that means a 5.18 backport is needed, see also the ticket for that commit at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=113536 .

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @bulk88

The reason it didn't leak on my winxp 32 bit machine is, for that machine, GetEnvironmentStringsW doesn't really allocate anything


_GetEnvironmentStringsW@​0​:
7C812FA8 64 A1 18 00 00 00 mov eax,dword ptr fs​:[00000018h]
7C812FAE 8B 40 30 mov eax,dword ptr [eax+30h]
7C812FB1 8B 40 10 mov eax,dword ptr [eax+10h]
7C812FB4 8B 40 48 mov eax,dword ptr [eax+48h]
7C812FB7 C3 ret


on Server 2003 x64, with a 32 bit Perl, notice the call to RtlAllocateHeap


7D4DF986 64 A1 18 00 00 00 mov eax,dword ptr fs​:[00000018h]
7D4DF98C 8B 40 30 mov eax,dword ptr [eax+30h]
7D4DF98F 53 push ebx
7D4DF990 56 push esi
7D4DF991 8B 70 10 mov esi,dword ptr [eax+10h]
7D4DF994 57 push edi
7D4DF995 FF 15 C0 00 4D 7D call dword ptr [__imp__RtlAcquirePebLock@​0 (7D4D00C0h)]
7D4DF99B 8B 7E 48 mov edi,dword ptr [esi+48h]
7D4DF99E 8B F7 mov esi,edi
7D4DF9A0 57 push edi
7D4DF9A1 E8 C4 95 FF FF call _wcslen (7D4D8F6Ah)
7D4DF9A6 8D 7C 47 02 lea edi,[edi+eax*2+2]
7D4DF9AA 66 83 3F 00 cmp word ptr [edi],0
7D4DF9AE 59 pop ecx
7D4DF9AF 75 EF jne _GetEnvironmentStringsW@​0+1Ah (7D4DF9A0h)
7D4DF9B1 8B 0D 5C 06 56 7D mov ecx,dword ptr [_BaseDllTag (7D56065Ch)]
7D4DF9B7 64 A1 18 00 00 00 mov eax,dword ptr fs​:[00000018h]
7D4DF9BD 8B 40 30 mov eax,dword ptr [eax+30h]
7D4DF9C0 2B FE sub edi,esi
7D4DF9C2 47 inc edi
7D4DF9C3 47 inc edi
7D4DF9C4 57 push edi
7D4DF9C5 81 C1 00 00 18 00 add ecx,180000h
7D4DF9CB 51 push ecx
7D4DF9CC FF 70 18 push dword ptr [eax+18h]
7D4DF9CF FF 15 0C 00 4D 7D call dword ptr [__imp__RtlAllocateHeap@​12 (7D4D000Ch)]
7D4DF9D5 8B D8 mov ebx,eax
7D4DF9D7 85 DB test ebx,ebx
7D4DF9D9 0F 84 68 02 03 00 je _GetEnvironmentStringsW@​0+55h (7D50FC47h)
7D4DF9DF 8B CF mov ecx,edi
7D4DF9E1 8B C1 mov eax,ecx
7D4DF9E3 C1 E9 02 shr ecx,2
7D4DF9E6 8B FB mov edi,ebx
7D4DF9E8 F3 A5 rep movs dword ptr es​:[edi],dword ptr [esi]
7D4DF9EA 8B C8 mov ecx,eax
7D4DF9EC 83 E1 03 and ecx,3
7D4DF9EF F3 A4 rep movs byte ptr es​:[edi],byte ptr [esi]
7D4DF9F1 FF 15 B8 00 4D 7D call dword ptr [__imp__RtlReleasePebLock@​0 (7D4D00B8h)]
7D4DF9F7 8B C3 mov eax,ebx
7D4DF9F9 5F pop edi
7D4DF9FA 5E pop esi
7D4DF9FB 5B pop ebx
7D4DF9FC C3 ret


Notice in both, a deref at offset 48h happens, but on the Server 2003 its memcpy-ed to a new memory block. IDK if this is a 32 bit process on x64 thing, or a Server 2003 thing. On my WinXP FreeEnvironmentStringsW asm wise, just returns 1 always. It doesn't free anything. Which matches WinXP's GetEnvironmentStringsW's behavior.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @bulk88

On Tue May 13 04​:33​:39 2014, bulk88 wrote​:

IDK if this is a 32 bit process
on x64 thing, or a Server 2003 thing. On my WinXP
FreeEnvironmentStringsW asm wise, just returns 1 always. It doesn't
free anything. Which matches WinXP's GetEnvironmentStringsW's
behavior.

64 bit kernel32.dll from x64 Server 2003 also allocates memory identically to the 32 bit kernel32.dll code. IDK what a pure 32 bit Server 2003 would do, or a NT6 machine.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @bulk88

On Tue May 13 04​:36​:45 2014, bulk88 wrote​:

On Tue May 13 04​:33​:39 2014, bulk88 wrote​:

IDK if this is a 32 bit process
on x64 thing, or a Server 2003 thing. On my WinXP
FreeEnvironmentStringsW asm wise, just returns 1 always. It doesn't
free anything. Which matches WinXP's GetEnvironmentStringsW's
behavior.

64 bit kernel32.dll from x64 Server 2003 also allocates memory
identically to the 32 bit kernel32.dll code. IDK what a pure 32 bit
Server 2003 would do, or a NT6 machine.

I think I know what happened, MS decided to enforce the


Treat this memory as read-only; do not modify it directly. To add or change an environment variable, use the GetEnvironmentVariable and SetEnvironmentVariable functions.


sentence from MSDN's GetEnvironmentStrings docs. This article supports that guess http​://blogs.msdn.com/b/oldnewthing/archive/2013/01/18/10385718.aspx .

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @steve-m-hay

-----Original Message-----
From​: bulk88 via RT [mailto​:perlbug-followup@​perl.org]
Sent​: 13 May 2014 12​:42
Cc​: perl5-porters@​perl.org
Subject​: [perl #121676] Memory leak in backticks and system

On Tue May 13 04​:36​:45 2014, bulk88 wrote​:

On Tue May 13 04​:33​:39 2014, bulk88 wrote​:

IDK if this is a 32 bit process
on x64 thing, or a Server 2003 thing. On my WinXP
FreeEnvironmentStringsW asm wise, just returns 1 always. It doesn't
free anything. Which matches WinXP's GetEnvironmentStringsW's
behavior.

64 bit kernel32.dll from x64 Server 2003 also allocates memory
identically to the 32 bit kernel32.dll code. IDK what a pure 32 bit
Server 2003 would do, or a NT6 machine.

I think I know what happened, MS decided to enforce the

-----------------------
Treat this memory as read-only; do not modify it directly. To add or
change an environment variable, use the GetEnvironmentVariable and
SetEnvironmentVariable functions.
-----------------------

sentence from MSDN's GetEnvironmentStrings docs. This article supports
that guess
http​://blogs.msdn.com/b/oldnewthing/archive/2013/01/18/10385718.aspx .

Thanks for the details.

The attached patch fixes the leak for me. If this looks good to you then I think it should go in for 5.20 (and get backported to 5.18 too).

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @steve-m-hay

0001-perl-121676-Fix-memory-leak-in-backticks-and-system.patch
From 8203faa53157a34bf7342219a319bf7a3562c3ca Mon Sep 17 00:00:00 2001
From: Steve Hay <steve.m.hay@googlemail.com>
Date: Tue, 13 May 2014 12:53:10 +0100
Subject: [PATCH] [perl #121676] Fix memory leak in backticks and system

Introduced by perl #113536. It doesn't actually leak on Windows XP due to
differences in the implementation of GetEnvironmentStringsW() compared to
later OSes, but the missing FreeEnvironmentStrings() was technically wrong
anyway, and does now bite. Thanks for Daniel Dragan for finding this.
---
 win32/win32.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/win32/win32.c b/win32/win32.c
index bff5b88..41d10dd 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -1770,6 +1770,8 @@ win32_getenvironmentstrings(void)
     WideCharToMultiByte(CP_ACP, WC_NO_BEST_FIT_CHARS, lpWStr, wenvstrings_len, lpStr, 
                         aenvstrings_len, NULL, NULL);
 
+    FreeEnvironmentStrings(lpWStr);
+
     return(lpStr);
 }
 
-- 
1.8.4.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @bulk88

On Tue May 13 04​:57​:08 2014, Steve.Hay@​verosoftware.com wrote​:

Thanks for the details.

The attached patch fixes the leak for me. If this looks good to you
then I think it should go in for 5.20 (and get backported to 5.18
too).

Works for me, no more leaking on the Server 2003. I also wrote a perldelta for this that is attached.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @bulk88

0002-perldelta-for-121676.patch
From c2e7a6e3f72d8c6fb036c05c91b3b6b974503749 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Wed, 14 May 2014 04:09:39 -0400
Subject: [PATCH 2/2] perldelta for #121676

---
 pod/perldelta.pod |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/pod/perldelta.pod b/pod/perldelta.pod
index 3b4db7f..07e963f 100644
--- a/pod/perldelta.pod
+++ b/pod/perldelta.pod
@@ -1598,6 +1598,14 @@ would crash accessing parameter information for context stack entries
 that included no parameters, as with C<&foo;>.
 [L<perl #121721|https://rt.perl.org/Public/Bug/Display.html?id=121721>]
 
+=item *
+
+Introduced by [perl #113536], a memory leak on every call to C<system>
+and backticks (C< `` >), on most Win32 Perls starting from 5.18.0
+has been fixed.  The memory leak only occurred if you enabled psuedo-fork in
+your build of Win32 Perl, and were running that build on Server 2003 R2
+or newer OS.  The leak does not appear on WinXP SP3. [perl #121676]
+
 =back
 
 =item WinCE
-- 
1.7.9.msysgit.0

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @steve-m-hay

On 14 May 2014 09​:26, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

On Tue May 13 04​:57​:08 2014, Steve.Hay@​verosoftware.com wrote​:

Thanks for the details.

The attached patch fixes the leak for me. If this looks good to you
then I think it should go in for 5.20 (and get backported to 5.18
too).

Works for me, no more leaking on the Server 2003. I also wrote a perldelta for this that is attached.

Thanks.

Ricardo​: Are you happy for this patch (and the perldelta) to go into
blead for 5.20?

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @rjbs

* Steve Hay <steve.m.hay@​googlemail.com> [2014-05-14T08​:09​:28]

Ricardo​: Are you happy for this patch (and the perldelta) to go into
blead for 5.20?

+1

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @steve-m-hay

On 14 May 2014 15​:16, Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote​:

* Steve Hay <steve.m.hay@​googlemail.com> [2014-05-14T08​:09​:28]

Ricardo​: Are you happy for this patch (and the perldelta) to go into
blead for 5.20?

+1

Now in​: commits 90674ea and 58627cd.

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @bulk88

On Wed May 14 10​:17​:27 2014, shay wrote​:

On 14 May 2014 15​:16, Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote​:

* Steve Hay <steve.m.hay@​googlemail.com> [2014-05-14T08​:09​:28]

Ricardo​: Are you happy for this patch (and the perldelta) to go into
blead for 5.20?

+1

Now in​: commits 90674ea and 58627cd.

I noticed a problem, the patch uses FreeEnvironmentStrings, but above we used GetEnvironmentStringsW, with the "W". Therefore we have to use FreeEnvironmentStringsW, not FreeEnvironmentStrings which I bet is mapping to FreeEnvironmentStringsA. FreeEnvironmentStringsA and FreeEnvironmentStringsW have different implementations on my WinXP SP3 machine FreeEnvironmentStringsA calls RtlFreeHeap, FreeEnvironmentStringsW doesn't.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @bulk88

On Wed May 14 10​:55​:10 2014, bulk88 wrote​:

I noticed a problem, the patch uses FreeEnvironmentStrings, but above
we used GetEnvironmentStringsW, with the "W". Therefore we have to use
FreeEnvironmentStringsW, not FreeEnvironmentStrings which I bet is
mapping to FreeEnvironmentStringsA. FreeEnvironmentStringsA and
FreeEnvironmentStringsW have different implementations on my WinXP SP3
machine FreeEnvironmentStringsA calls RtlFreeHeap,
FreeEnvironmentStringsW doesn't.

On Server 2003 kernel32.dll, asm-wise FreeEnvironmentStringsA and FreeEnvironmentStringsW are the same function pointer (de-duping identical functions optimization by compiler), that is probably why nothing wrong happened when I tested the fix.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @steve-m-hay

On 14 May 2014 18​:58, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

On Wed May 14 10​:55​:10 2014, bulk88 wrote​:

I noticed a problem, the patch uses FreeEnvironmentStrings, but above
we used GetEnvironmentStringsW, with the "W". Therefore we have to use
FreeEnvironmentStringsW, not FreeEnvironmentStrings which I bet is
mapping to FreeEnvironmentStringsA. FreeEnvironmentStringsA and
FreeEnvironmentStringsW have different implementations on my WinXP SP3
machine FreeEnvironmentStringsA calls RtlFreeHeap,
FreeEnvironmentStringsW doesn't.

On Server 2003 kernel32.dll, asm-wise FreeEnvironmentStringsA and FreeEnvironmentStringsW are the same function pointer (de-duping identical functions optimization by compiler), that is probably why nothing wrong happened when I tested the fix.

Argh! My apologies and thanks for the spot. Now fixed in commit a6abe94.

(Yes, FreeEnvironmentStringsA maps to FreeEnvironmentStrings unless
UNICODE is #defined. Normally I would just write
FreeEnvironmentStrings and #define UNICODE (or not) as required, hence
I wasn't on the ball there with the need to explicitly call the *W()
version in this case.)

@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

From @steve-m-hay

Now all fixed in blead as noted above, and also backported to maint-5.18 in these four commits​:

2358a19
72a2b48
d4ade35
b54db2a

@p5pRT p5pRT closed this as completed May 14, 2014
@p5pRT
Copy link
Author

p5pRT commented May 14, 2014

@steve-m-hay - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant