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
older MS CRTs/libc putenv'es/Time::Piece cause memory corruption #14782
Comments
From @bulk88Created by @bulk88TLDR: Time::Piece causes random memory corruption due to an interaction Win32 Perl minimally uses MS CRT's _putenv function. It is only used in Perl sets ENV vars like this. Time::Piece directly calls _putenv to update the CRT's copy of the env ---------------------------------------------------------------------- running cpan/Time-Piece/t/02core.t under the C debugger eventually ---------------------------------------------------------------------- and if execution continues, an assert fail (DEBUGGING) or crash (no ---------------------------------------------------------------------- This application has requested the Runtime to terminate it in an unusual C:\perl521\srcnewb4opt> The assert fail comes from. ---------------------------------------------------------------------- The assert fail specifically occurs because malloc/win32 heap returns The bug in the CRT is, in _putenv_lk, there is if (remove) { return retval; If SetEnvironmentVariable call fails, __crtsetenv frees var option, and Time::Piece's testing makes SetEnviromentVariable fail with ERROR_ENVVAR_NOT_FOUND That triggers the double free bug. A different block/branch in __crtsetenv shows NULL being assigned to the -------------------------------------------------------------- /* we now freed the pointer, so NULL out the incoming return 0; I've written a test case of this double free bug which I attached. You Some failing callstacks from the test case. First-chance exception at 0x00a0086f (msvcr80d.dll) in putenvsegv.exe: msvcr80d.dll!_free_dbg_nolock(void * pUserData=0x003e3a48, int How does the CRT get env var info from the OS? We know it supposedly So where does it initially do the import from Win32 env to CRT env? This is from studying msvcr71.dll GetEnvironmentVariableA, only used to lookup __MSVCRT_HEAP_SELECT in GetEnvironmentStringsW/GetEnvironmentStrings, unexported _setenvp called Another idea is, in Time::Piece always registering empty string env var Perl Info
|
From @bulk88<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"> |
From @bulk88// putenvsegv.cpp : Defines the entry point for the console application. #define _WIN32_WINNT 0x0500 #include <stdio.h> void trycrt(const char * libname) { // Using Microsoft's intrinsics instead of inline assembly int _tmain(int argc, _TCHAR* argv[]) hActCtx = CreateActCtx(&ActCtx); for(int i = 0; i< (sizeof(crts)/sizeof(void*)); i++) { return 0; |
From @bulk88 |
From @bulk88 |
From @bulk88<?xml version="1.0" encoding="Windows-1252"?> |
From @dolmenThe right place to report this issue is probably on GitHub: Le Jeu 02 Jui 2015 02:17:12, bulk88 a écrit :
|
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Thu Jul 02 10:22:01 2015, dolmen wrote:
Steve Hay/Tony C/JDB will never see the ticket if I file it there on T::P's github. I don't see RJBS or Sam Smith (T::P's maints) being able to comment on Win32 C issues. Time::Piece's Win32 putenv code was developed on P5P ML in http://www.nntp.perl.org/group/perl.perl5.porters/2009/03/msg145324.html and committed to P5P repo before published on CPAN ( http://perl5.git.perl.org/perl.git/commitdiff/12016aadb5ccd03002d026d37636471225cf9aa5 and http://perl5.git.perl.org/perl.git/commitdiff/6e0733998eff7a098d2d21d5602f3eb2a7521e1f Dual-Life/Time-Piece@5229c9b ). This bug also wouldn't happen if Perl core used putenv instead of SetEnvironmentVariable (but I am not advocating this change). This ticket asks if Perl core needs to a C function to resync the Win32 env that Perl uses with the libc CRT env. I think P5P is the most appropriate venue for the ticket. -- |
From @tonycozOn Thu Jul 02 11:33:09 2015, bulk88 wrote:
Can you see a solution that isn't "use a newer compiler"? Tony |
From @bulk88On Mon Jul 13 18:32:12 2015, tonyc wrote:
1. P5P offers a "sync win32 env with crt env using _environ and _wenviron data vars" function to XS code. Time::Piece uses that function on older VCs. 2. Time::Piece applies the patch in Dual-Life/Time-Piece#16 . I've attached that patch to the Perl RT ticket (this ticket) for ease of viewing (on github its, eghhh, steganography stored). 3. Live patch machine code old CRTs at runtime. Similar in spirit to this win32/win32.c | 23 +++++++++++++++++++++++ Inline Patchdiff --git a/win32/win32.c b/win32/win32.c
index 3bb6e06..f2d01e5 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -4491,6 +4491,29 @@ Perl_win32_init(int *argcp, char ***argvp)
#endif
ansify_path();
+ {
+ char * f = (char*)GetProcAddress(GetModuleHandle("kernel32.dll"), "BaseQueryModuleData");
+ if(f) {
+ if(memcmp(f, "\x8B\xFF\x55\x8B\xEC\x51\xE8", sizeof("\x8B\xFF\x55\x8B\xEC\x51\xE8")-1) == 0){
+ f += sizeof("\x8B\xFF\x55\x8B\xEC\x51\xE8")-1;
+ f = f + 4 + *(U32*)f;
+ if(memcmp(f, "\x8B\xFF\x55\x8B\xEC\x83\xEC\x20\xA1\xCC\x56\x88\x7C\x57\x33\xFF\x89\x45\xFC\xA1",
+ sizeof("\x8B\xFF\x55\x8B\xEC\x83\xEC\x20\xA1\xCC\x56\x88\x7C\x57\x33\xFF\x89\x45\xFC\xA1")-1)
+ == 0){
+ U32 * flag;
+ f+= sizeof("\x8B\xFF\x55\x8B\xEC\x83\xEC\x20\xA1\xCC\x56\x88\x7C\x57\x33\xFF\x89\x45\xFC\xA1")-1;
+ flag = *(U32**)f;
+ /* now we have address of the static var */
+ *flag = 1;
+ return;
+ }
+ }
+ DebugBreak(); /* failed, time to analyze */
+ }
+ }
}
void
--
-- |
From @bulk880001-fix-perl-125529-putenv-mem-corruption-on-older-ms-cr.patchFrom 6b5432abccf86bb4fdb11c47b4eaebbb767eee22 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Sat, 4 Jul 2015 22:04:31 -0400
Subject: [PATCH] fix [perl #125529] putenv mem corruption on older MS
CRTs/libs
---
cpan/Time-Piece/Piece.pm | 2 +-
cpan/Time-Piece/Piece.xs | 16 +++++++++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/cpan/Time-Piece/Piece.pm b/cpan/Time-Piece/Piece.pm
index a8b80fc..6833529 100644
--- a/cpan/Time-Piece/Piece.pm
+++ b/cpan/Time-Piece/Piece.pm
@@ -17,7 +17,7 @@ our %EXPORT_TAGS = (
':override' => 'internal',
);
-our $VERSION = '1.30';
+our $VERSION = '1.31';
require DynaLoader;
{
diff --git a/cpan/Time-Piece/Piece.xs b/cpan/Time-Piece/Piece.xs
index eafb790..221376f 100644
--- a/cpan/Time-Piece/Piece.xs
+++ b/cpan/Time-Piece/Piece.xs
@@ -153,8 +153,22 @@ fix_win32_tzenv(void)
if (crt_tz_env == NULL)
crt_tz_env = "";
if (strcmp(perl_tz_env, crt_tz_env) != 0) {
- newenv = (char*)malloc((strlen(perl_tz_env) + 4) * sizeof(char));
+ STRLEN perl_tz_env_len = strlen(perl_tz_env);
+ newenv = (char*)malloc((perl_tz_env_len + 4) * sizeof(char));
if (newenv != NULL) {
+/* putenv with old MS CRTs will cause a double free internally if you delete
+ an env var with the CRT env that doesn't exist in Win32 env (perl %ENV only
+ modifies the Win32 env, not CRT env), so always create the env var in Win32
+ env before deleting it with CRT env api, so the error branch never executes
+ in __crtsetenv after SetEnvironmentVariableA executes inside __crtsetenv.
+
+ VC 9/2008 and up dont have this bug, older VC (msvcrt80.dll and older) and
+ mingw (msvcrt.dll) have it see [perl #125529]
+*/
+#if !(_MSC_VER >= 1500)
+ if(!perl_tz_env_len)
+ SetEnvironmentVariableA("TZ", "");
+#endif
sprintf(newenv, "TZ=%s", perl_tz_env);
putenv(newenv);
if (oldenv != NULL)
--
1.7.9.msysgit.0
|
From @bulk88On Thu Jul 02 02:17:12 2015, bulk88 wrote:
For the record, PHP found this bug in 2005 https://bugs.php.net/bug.php?id=32957 , they decided to fix it by always creating the env var in Win32 env before calling CRT putenv. -- |
From @bulk88On Thu Jul 02 02:17:12 2015, bulk88 wrote:
So should perl core have a win32_sync_crt_env function or not? If not, then the Time::Piece fix has to go into Time::Piece and get shipped. cpan/Time-Piece/t/02core.t randomly SEGVing/panic errors/test failing on unthreaded perl is annoying me. -- |
From @bulk88On Sun Aug 02 21:47:47 2015, bulk88 wrote:
Bump. -- |
From @tonycozOn Sun Aug 02 21:47:47 2015, bulk88 wrote:
I think the answer is #2. I don't see a ticket for this at https://rt.cpan.org//Dist/Display.html?Queue=Time-Piece which is supposed to be the dist's bug tracker. Tony |
From @bulk88On Sun Aug 16 18:45:22 2015, tonyc wrote:
Dual-Life/Time-Piece#16 I didn't want to do a PR to prevent a hasty merge before the CRT sync question is answered, since Time-Piece probably isn't the only XS module in the world to do "#undef getenv" or NO_XSLOCKS and use the real libc putenv instead of the perl psuedofork one. Since it doesn't look anyone else will answer, I am slightly learning towards #2 since it is the quicker solution than trying to manipulate the env copy inside the CRT which is not that friendly to do (see also the VC 2015 threads/tickets where the undocumented _pioinfo data array was dropped from the CRT lib in the 2015 edition). You could also argue that only old CRTs need to have their ENVs synced, and those old CRTs are out of support for MS and won't ever change. there are other open issues on github so I am not the first to use GH issue tracker by accident Dual-Life/Time-Piece#15 , maybe rjbs can disable the issue tracker if he chooses -- |
From p5p@net153.netOn 08/16/2015 10:02 PM, bulk88 via RT wrote:
Added patch and pushed out as version 1.30_01. Did not test on windows --Sam |
From @bulk88On Thu Jul 02 02:17:12 2015, bulk88 wrote:
I found the same code as in Time::Piece in POSIX::, http://perl5.git.perl.org/perl.git/blob/2efb8b4b644d5f3c28974a8f577081b4142decd2:/ext/POSIX/POSIX.xs#l1738 so POSIX will need fixing whatever way Time::Piece will be fixed. -- |
Given Time::Piece fixed this in 1.32 and 1.33 is in blead, I'm closing this case. |
Migrated from rt.perl.org#125529 (status was 'open')
Searchable as RT125529$
The text was updated successfully, but these errors were encountered: