Memory leak in backticks and system #13741
Comments
From @bulk88Created by @bulk88There is a mem leak in system and `` on Win32 on 5.19 and 5.18 (tested Perl Info |
From @tonycozOn Wed Apr 16 23:37:39 2014, bulk88 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 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 |
|
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Mon Apr 21 23:33:16 2014, tonyc wrote:
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.
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. -- |
From @tonycozOn Tue, Apr 22, 2014 at 04:41:00AM -0700, bulk88 via RT wrote:
I had to fiddle with the source a bit, the macro in perl.h: #define CALLREGFREE_PVT(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
You can build a -DDEBUGGING perl and set PERL_DESTRUCT_LEVEL=2 in the
A USE_IMP_SYS build did leak. Tony |
From @steve-m-hayOn 22 April 2014 15:01, Tony Cook <tony@develop-help.com> wrote:
I had a crack at using the debug CRT a year or more ago and ran into FWIW, here's the patch that I was playing with last. It's well over a |
From @steve-m-haydebug.patchdiff --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);
|
From @bulk88On Wed Apr 16 23:37:39 2014, bulk88 wrote:
Isolated to C:\Documents and Settings\Administrator\Desktop\lxs\Local-XS>perl -MLocal::XS - void eats 100 MB of mem a sec. -- |
From @steve-m-hay
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...) |
From @bulk88On Tue May 13 04:02:44 2014, bulk88 wrote:
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 . -- |
From @bulk88The reason it didn't leak on my winxp 32 bit machine is, for that machine, GetEnvironmentStringsW doesn't really allocate anything _GetEnvironmentStringsW@0: 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] 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. -- |
From @bulk88On Tue May 13 04:33:39 2014, bulk88 wrote:
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. -- |
From @bulk88On Tue May 13 04:36:45 2014, bulk88 wrote:
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 . -- |
From @steve-m-hay
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). |
From @steve-m-hay0001-perl-121676-Fix-memory-leak-in-backticks-and-system.patchFrom 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
|
From @bulk88On Tue May 13 04:57:08 2014, Steve.Hay@verosoftware.com wrote:
Works for me, no more leaking on the Server 2003. I also wrote a perldelta for this that is attached. -- |
From @bulk880002-perldelta-for-121676.patchFrom 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
|
From @steve-m-hayOn 14 May 2014 09:26, bulk88 via RT <perlbug-followup@perl.org> wrote:
Thanks. Ricardo: Are you happy for this patch (and the perldelta) to go into |
From @rjbs* Steve Hay <steve.m.hay@googlemail.com> [2014-05-14T08:09:28]
+1 -- |
From @steve-m-hayOn 14 May 2014 15:16, Ricardo Signes <perl.p5p@rjbs.manxome.org> wrote:
|
From @bulk88On Wed May 14 10:17:27 2014, shay 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. -- |
From @bulk88On Wed May 14 10:55:10 2014, bulk88 wrote:
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. -- |
From @steve-m-hayOn 14 May 2014 18:58, bulk88 via RT <perlbug-followup@perl.org> wrote:
Argh! My apologies and thanks for the spot. Now fixed in commit a6abe94. (Yes, FreeEnvironmentStringsA maps to FreeEnvironmentStrings unless |
From @steve-m-hayNow all fixed in blead as noted above, and also backported to maint-5.18 in these four commits: |
|
@steve-m-hay - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#121676 (status was 'resolved')
Searchable as RT121676$
The text was updated successfully, but these errors were encountered: