uninit mem read in win32_fseek() randomly creates multi GB files on Win64 #13677
Comments
From @bulk88Created by @bulk88Using "Microsoft (R) C/C++ Optimizing Compiler Version 15.00.30729.01 Specifically a file is extended to 4 GB file is randomly (over the last ------------------------------------ Happens. The sum of $offset+4+20+68 should be, and on PP level is ~ ------------------------------------- Note the first pos is 340 bytes (~0x150) above 2^32. This really is the Asm dump of the broken win32_fseek from the binary Sending win32_fseek through preprocessor shows everything is correct. after preprocessor, as win32 miniperl fpos_t pos; #line 2843 "win32.c" In the attached asm output from Visual C (NOT the asm from the final Perl Info |
From @bulk88; COMDAT pdata ; 2819 : { $LN12: ; 2820 : #if defined(WIN64) || defined(USE_LARGE_FILES) test r8d, r8d ; 2833 : case SEEK_SET: call QWORD PTR __imp__errno ; 2837 : return -1; jmp SHORT $LN11@win32_fsee ; 2827 : break; xor edx, edx ; 2830 : pos = _telli64(fileno(pf)); mov rcx, rbx ; 2831 : offset += pos; jmp SHORT $LN10@win32_fsee ; 2823 : case SEEK_CUR: lea rdx, QWORD PTR pos$[rsp] ; 2825 : return -1; or eax, -1 ; 2826 : offset += pos; mov rax, QWORD PTR pos$[rsp] ; 2838 : } lea rdx, QWORD PTR offset$[rsp] ; 2840 : #else add rsp, 32 ; 00000020H |
From @steve-m-hayI wish you had mentioned in the subject that this affects the creation of wperl.exe! Lost in the detail of your report you mention the win32/bin/exetype.pl command-line, but I hadn't clocked that it was what built wperl.exe. I've just stumbled across this bug when testing a 64-bit build of blead and found (when trying to copy the installation somewhere and wondering why it was taking so long!) that my wperl.exe is 4GB. Then I recalled seeing your report recently... This seems like a serious bug to me -- surely bad news for anyone that uses wperl.exe in a 64-bit build. I wonder if this should be marked a blocker for 5.20 since (after a quick test to double-check) 5.18.2 does not have this problem? |
From [Unknown Contact. See original ticket]I wish you had mentioned in the subject that this affects the creation of wperl.exe! Lost in the detail of your report you mention the win32/bin/exetype.pl command-line, but I hadn't clocked that it was what built wperl.exe. I've just stumbled across this bug when testing a 64-bit build of blead and found (when trying to copy the installation somewhere and wondering why it was taking so long!) that my wperl.exe is 4GB. Then I recalled seeing your report recently... This seems like a serious bug to me -- surely bad news for anyone that uses wperl.exe in a 64-bit build. I wonder if this should be marked a blocker for 5.20 since (after a quick test to double-check) 5.18.2 does not have this problem? |
From @steve-m-hayOn 28 March 2014 23:30, Steve Hay via RT <perlbug-comment@perl.org> wrote:
I've bisected this to a change made by Ruslan Zakirov and committed by This commit is ok: but the very next commit breaks the 64-bit build on Windows, causing I do not see anything obviously wrong with the change or why it has |
From @bulk88On Sat Mar 29 11:58:21 2014, shay wrote:
IDK about it being a blocker, since I have no estimate of time to fix this by me. I'm a bit busy atm. I think its a compiler bug since the preprocessor C is fine (feel free to verify on your side the .i file, that "Off_t offset" really is a 8 byte type according to the rules of C and I'm not crazy), and the asm (not machine) code outputed shows a 8 byte mov, but C compiler bug is a heavy accusation to make. It might be related to the -GL flag, since VC will not produce an asm file if you pass -GL (WPO/IPO/LTO). And when you remove -GL, the asm code is fine.
It is an uninit memory bug. If the C stack happens to have 0x00000000 on it, in the area which later becomes "Off_t offset", no failure will be observed. Looking at the machine code from the binary is the only way to figure it out (is the source for the x64 mov rdx or edx?), or put into the runloop something like void * getStack(int param) { // in runloop before each pp_ void * freeCstack = getStack(param); Looking at ActivePerl Build 1402 [295342] 5.14.2 x64, win32_fseek uses rdx (correct). mov [rsp+arg_8], rdx My VC 2008 x64 -Od perl519.dll, win32_fseek uses rdx (correct). mov [rsp+arg_10], r8d Same build as above, miniperl.exe has mov [rsp+arg_10], r8d In addition to -GL hypothesis, lack of win32_fseek being PE exported is another thing that can be affecting this. Exporting a C function or using it as a fnc ptr on x86-32 with VC kills the random calling convention optimization of -GL or "static". Then again, with VC x64, I've never ever seen the random calling convention feature since that would probably (not 100% sure) break MS's highly limited SEH on x64 exception unwind table syntax. miniperl.exe rightfully has no exports nowadays after some optimizations I did a long time ago http://perl5.git.perl.org/perl.git/commitdiff/a19baa613c9769701790afd2f9ebfc422a1e97e7 . -- |
From @steve-m-hayOn 29 March 2014 22:13, bulk88 via RT <perlbug-followup@perl.org> wrote:
I don't think whether any individual has time to fix it should have Fotunately, I've now fixed it anyway after staring at things for [...]
I see the same thing too (using dumpbin /disasm on perl519.dll and I then tried to generate the asm output from the compiler which you When building miniperl.exe we use a canned configuration file copied But by the time we build perl519.dll we have rewritten config.h using The latter contains some tweaks for large file options (the Thus, when building miniperl.exe Off_t is actually the canned config.h The attached patch adds the correct setting of Off_t et al to As for whether this is a blocker... It could be argued not since the I would like to see this patch go in for 5.20, but it's Ricardo's call. |
From @steve-m-hayOff_t.patchdiff --git a/win32/Makefile b/win32/Makefile
index 5991a00..9a6df99 100644
--- a/win32/Makefile
+++ b/win32/Makefile
@@ -887,6 +887,9 @@ config.w32 : $(CFGSH_TMPL)
@echo.>>$@
@echo #ifndef _config_h_footer_>>$@
@echo #define _config_h_footer_>>$@
+ @echo #undef Off_t>>$@
+ @echo #undef LSEEKSIZE>>$@
+ @echo #undef Off_t_size>>$@
@echo #undef PTRSIZE>>$@
@echo #undef SSize_t>>$@
@echo #undef HAS_ATOLL>>$@
@@ -905,6 +908,15 @@ config.w32 : $(CFGSH_TMPL)
@echo #undef UVXf>>$@
@echo #undef USE_64_BIT_INT>>$@
@echo #undef Size_t_size>>$@
+!IF "$(USE_LARGE_FILES)"=="define"
+ @echo #define Off_t __int64>>$@
+ @echo #define LSEEKSIZE ^8>>$@
+ @echo #define Off_t_size ^8>>$@
+!ELSE
+ @echo #define Off_t long>>$@
+ @echo #define LSEEKSIZE ^4>>$@
+ @echo #define Off_t_size ^4>>$@
+!ENDIF
!IF "$(WIN64)"=="define"
@echo #define PTRSIZE ^8>>$@
@echo #define SSize_t __int64>>$@
diff --git a/win32/makefile.mk b/win32/makefile.mk
index 269f4b7..0c89340 100644
--- a/win32/makefile.mk
+++ b/win32/makefile.mk
@@ -1015,6 +1015,9 @@ config.w32 : $(CFGSH_TMPL)
@echo.>>$@
@echo #ifndef _config_h_footer_>>$@
@echo #define _config_h_footer_>>$@
+ @echo #undef Off_t>>$@
+ @echo #undef LSEEKSIZE>>$@
+ @echo #undef Off_t_size>>$@
@echo #undef PTRSIZE>>$@
@echo #undef SSize_t>>$@
@echo #undef HAS_ATOLL>>$@
@@ -1033,6 +1036,15 @@ config.w32 : $(CFGSH_TMPL)
@echo #undef UVXf>>$@
@echo #undef USE_64_BIT_INT>>$@
@echo #undef Size_t_size>>$@
+.IF "$(USE_LARGE_FILES)"=="define"
+ @echo #define Off $(INT64)>>$@
+ @echo #define LSEEKSIZE ^8>>$@
+ @echo #define Off_t_size ^8>>$@
+.ELSE
+ @echo #define Off long>>$@
+ @echo #define LSEEKSIZE ^4>>$@
+ @echo #define Off_t_size ^4>>$@
+.ENDIF
.IF "$(WIN64)"=="define"
@echo #define PTRSIZE ^8>>$@
@echo #define SSize_t $(INT64)>>$@
|
|
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Sat Mar 29 17:37:38 2014, shay wrote:
Remove -GL from the command line. If you use /FAs and -GL, no assembly file is made. Probably because with -GL the .objs dont contain machine code but instead MS internal bytecode, so there is no assembly code to write to a file, so no file is written. MS needs to add a better error.
That commit is 5.17.3 but also Off_t is "long" in old config_H.vc64 file. I'm surprised AS never reported it.
Maybe the rewritten config.h is why my .i files showed 8 byte types even though I passed -DPERL_IS_MINIPERL . The config.h wasn't the one used to compile miniperl anymore. This does raise another bug problem, if you delete win32/mini, then "nmake ../miniperl.exe", it will be partially a full perl then inside that miniperl.
Uninit memory, if I really wanted to I'd step the pp opcodes until I figured out the C statment that did it but that wouldn't help in solving this bug. Plus the sea of 64 bit trunction warnings that a VC win64 build has that nobody (me?) pays attention to contributed to this bug.
Building perl and getting a 4GB file of nulls written out to your disk isnt a friendly thing to do. It should go in. I haven't personally tested this patch. -- |
From @steve-m-hayOn 30 Mar 2014 04:23, "bulk88 via RT" <perlbug-followup@perl.org> wrote:
Thanks, I will try this later.
Indeed. If the old config_H.vc64 file had had Off_t being '__int64' then
Yes, you should really delete config.h too if you delete mini/ Not a problem that will affect end-users, though.
I've had fixing them on my TODO list for too long... :-/ I will try harder
I agree. Even if wperl.exe isn't widely used (and the fact that it's I think it should go in. Ricardo? |
From @steve-m-hayOn Sat Mar 29 20:23:26 2014, bulk88 wrote:
Do you have time to test the patch? It would be good to gets a head-sup from the OP that it fixes the problem. |
From @bulk88On Sun Apr 06 11:34:38 2014, shay wrote:
Its fixed. rdx used in miniperl asm. wperl.exe is normal size, not 4 gb. Next time can you post a git patch instead of a diff patch? Git patches are more convenient for me than diff patches. -- |
From @steve-m-hayOn Sun Apr 06 15:57:18 2014, bulk88 wrote:
Thanks for reporting/analyzing this problem and testing the fix, which is now applied in commit 200b4fd. |
|
@steve-m-hay - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#121471 (status was 'resolved')
Searchable as RT121471$
The text was updated successfully, but these errors were encountered: