View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0034772 | FPC | Compiler | public | 2018-12-28 14:21 | 2019-05-15 16:30 |
| Reporter | Martin Friebe | Assigned To | Sergei Gorelkin | ||
| Priority | high | Severity | crash | Reproducibility | always |
| Status | assigned | Resolution | open | ||
| Platform | 64bit Intel | OS | win 10 | OS Version | 10 |
| Product Version | 3.3.1 | Product Build | 40680 | ||
| Target Version | Fixed in Version | ||||
| Summary | 0034772: Win64 (seh) compiled exe crashes, because implicit finally handler is entered twice. | ||||
| Description | This happens on Win64 only (seh specific) / *not* tested with the 32 bit seh... See code in "steps to reproduce" compiled with -gh (tested with -gh -gt -gw) crashes. The procedure allocates memory for a local copy of "a: array of integer" When "exit" is executed, _FPC_local_unwind is called. This calls the finally handlers (both the user one, and the implicit one). The following code is generated (-al) >>>>>>>>>>>>>>>>> .seh_handler __FPC_specific_handler,@unwind .seh_handlerdata .long 2 .long 0 .rva .Lj14 .rva .Lj15 .rva P$PROJECT1$_$FOO$array_of_LONGINT_$$_fin$0 .long 0 .rva .Lj9 .rva .Lj10 .rva P$PROJECT1$_$FOO$array_of_LONGINT_$$_fin$1 <<<<<<<<<<<<<<<< Lj10 is BEFORE the call to the implicit finally .Lj10: .Lj11: movq %rbp,%rcx call P$PROJECT1$_$FOO$array_of_LONGINT_$$_fin$1 So after unwind finishes, the implicit finally handler is called a 2nd time (setting a breakpoint in asm proves this). The memory for "a" is attempted to be freed a 2nd time, which crashes the app. | ||||
| Steps To Reproduce | program project1; {$mode objfpc} uses Classes; procedure Foo(a: array of integer); begin try writeln(a[0]); if a[0] = 1 then exit; writeln(a[0]); finally writeln(a[0]); end; end; begin foo([1]); end. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | |||||
| FPCOldBugId | 0 | ||||
| FPCTarget | |||||
| Attached Files |
| ||||
|
|
i386-win32 (SEH is enabled) works. x86_64-win64 doesn't and produces following exception and its stack trace:$ .\project1.exe 1 1 An unhandled exception occurred at $000000010000E358: EInvalidPointer: Invalid pointer operation $000000010000E358 SYSFREEMEM_FIXED, line 1162 of ../inc/heap.inc $000000010000E52B SYSFREEMEM, line 1227 of ../inc/heap.inc $000000010000CDAA FREEMEM, line 324 of ../inc/heap.inc $0000000100009CB2 TOBJECT__FREEINSTANCE, line 447 of ../inc/objpas.inc $0000000100019507 TFPLIST__DESTROY, line 113 of ../objpas/classes/lists.inc $0000000100009A8F TOBJECT__FREE, line 336 of ../inc/objpas.inc $000000010002FB3C FREEANDNIL, line 160 of ../objpas/sysutils/sysutils.inc $0000000100019C5D TLIST__DESTROY, line 663 of ../objpas/classes/lists.inc $0000000100009A8F TOBJECT__FREE, line 336 of ../inc/objpas.inc $000000010001A235 TTHREADLIST__DESTROY, line 979 of ../objpas/classes/lists.inc $0000000100009A8F TOBJECT__FREE, line 336 of ../inc/objpas.inc $000000010002FB3C FREEANDNIL, line 160 of ../objpas/sysutils/sysutils.inc $0000000100020FE6 COMMONCLEANUP, line 2543 of ../objpas/classes/classes.inc $0000000100029C9E CLASSES_$$_finalize$, line 84 of classes.pp $000000010000C341 FINALIZEUNITS, line 1013 of ../inc/system.inc $000000010000C651 INTERNALEXIT, line 1094 of ../inc/system.inc $000000010000C6FE fpc_do_exit, line 1137 of ../inc/system.inc |
|
|
project1.zip (1,195 bytes) |
|
|
x86_64-win64_project1.s.zip (4,620 bytes) |
|
|
i386-win32_project1.s.zip (4,106 bytes) |
|
|
Program received signal SIGSEGV, Segmentation fault. 0x000000010000a0ac in TOBJECT__INHERITSFROM (self=0x1195068, ACLASS=0x10003eb80) at ../inc/objpas.inc:621 621 ../inc/objpas.inc: No such file or directory. (gdb) bt #0 0x000000010000a0ac in TOBJECT__INHERITSFROM (self=0x1195068, ACLASS=0x10003eb80) at ../inc/objpas.inc:621 0000001 0x0000000100009813 in fpc_do_is (ACLASS=0x10003eb80, AOBJECT=0x11950b0) at ../inc/objpas.inc:42 0000002 0x0000000100030154 in CATCHUNHANDLEDEXCEPTION (OBJ=0x11950b0, ADDR=0x10000e358, FRAMECOUNT=9, FRAMES=0x11a50d0) at ../objpas/sysutils/sysutils.inc:310 0000003 0x0000000100013ec7 in __FPC_DEFAULT_HANDLER (REC=..., FRAME=0x102feb0, CONTEXT=..., DISPATCH=...) at seh64.inc:422 0000004 0x00000000779281ad in ntdll!RtlDecodePointer () from C:\Windows\SYSTEM32\ntdll.dll 0000005 0x0000000077917ddc in ntdll!RtlUnwindEx () from C:\Windows\SYSTEM32\ntdll.dll 0000006 0x000000007781052e in RtlUnwindEx () from C:\Windows\system32\kernel32.dll 0000007 0x0000000100013cc6 in __FPC_DEFAULT_HANDLER (REC=..., FRAME=0x102feb0, CONTEXT=..., DISPATCH=...) at seh64.inc:396 0000008 0x000000007792812d in ntdll!RtlDecodePointer () from C:\Windows\SYSTEM32\ntdll.dll 0000009 0x000000007791855f in ntdll!RtlUnwindEx () from C:\Windows\SYSTEM32\ntdll.dll 0000010 0x000000007794bcb8 in ntdll!KiUserExceptionDispatcher () from C:\Windows\SYSTEM32\ntdll.dll 0000011 0x000007fefd70a06d in RaiseException () from C:\Windows\system32\KernelBase.dll 0000012 0x000000010001397a in fpc_raiseexception (OBJ=0x11950b0, ANADDR=0x10000e358, AFRAME=0x102fc90) at seh64.inc:293 0000013 0x0000000100030661 in RUNERRORTOEXCEPT (ERRNO=204, ADDRESS=0x10000e358, FRAME=0x102fc90) at ../objpas/sysutils/sysutils.inc:440 0000014 0x000000010000c7c4 in HANDLEERRORADDRFRAME (ERRNO=204, ADDR=0x10000e358, FRAME=0x102fc90) at ../inc/system.inc:1205 0000015 0x000000010000c874 in HANDLEERRORADDRFRAMEIND (ERRNO=204, ADDR=0x10000e358, FRAME=0x102fc90) at ../inc/system.inc:1222 0000016 0x000000010000c89f in fpc_handleerror (ERRNO=204) at ../inc/system.inc:1242 0000017 0x000000010000e358 in SYSFREEMEM_FIXED (LOC_FREELISTS=0x117cc3a, PMC=0x118d038) at ../inc/heap.inc:1162 0000018 0x000000010000e52b in SYSFREEMEM (P=0x118d040) at ../inc/heap.inc:1227 0000019 0x000000010000c6aa in INTERNALEXIT () at ../inc/system.inc:1106 0000020 0x000000010000c6fe in fpc_do_exit () at ../inc/system.inc:1137 0000021 0x000000010000187d in main () at project1.lpr:25 |
|
|
Just as a note, the same issue would happen with any other data in any other implicit finally handler. (if user try finally blocks are "exit"ed.) Strings and arrays however are set to nil, when de-ref'ed. So they do not crash. Still they should only run once. |
|
|
The unwind data looks correct, .Lj10 designates the end of 'try' part of implicit try..finally statement. It appears that in procedures with only an implicit try..finally, 'exit' statement generates a plain jump to the end of 'try' part, which allows to entirely avoid unwind-related activities and considerably improves performance. When user try..finally statement is added, 'exit' changes its behavior and generates a call to fpc_local_unwind instead of jump, so that 'finally' part of the inner try..finally can be executed. However the exit label (which becomes the second argument of fpc_local_unwind) looks incorrect in this case, because it is recognized as being outside of outer 'try' statement and unwinding to it calls the finalizer. |
|
|
Not sure if this is relevant in anyway, but FWIW: While fpc 3.0.4 does not trigger the crash, heaptrace barfs about wrong memory signature: C:\Users\Bart\LazarusProjecten\bugs\Console\seh>seh 1 1 Marked memory at $00000000000EF130 invalid Wrong signature $AAAAAAAA instead of 26C33231 |
|
|
more simple: program Project1; procedure DoIt(ar: array of const); begin try exit; finally end end; begin DoIt([]); end. |
|
|
nx64flw.pas.patch (2,794 bytes)
Index: nx64flw.pas
===================================================================
--- nx64flw.pas (revision 41795)
+++ nx64flw.pas (working copy)
@@ -223,6 +223,7 @@
endtrylabel,
finallylabel,
endfinallylabel,
+ templabel,
oldexitlabel: tasmlabel;
oldflowcontrol: tflowcontrol;
catch_frame: boolean;
@@ -244,6 +245,7 @@
oldflowcontrol:=flowcontrol;
flowcontrol:=[fc_inflowcontrol];
+ templabel:=nil;
current_asmdata.getjumplabel(trylabel);
current_asmdata.getjumplabel(endtrylabel);
current_asmdata.getjumplabel(finallylabel);
@@ -284,20 +286,19 @@
exit;
end;
- { If the immediately preceding instruction is CALL,
- its return address must not end up outside the scope, so pad with NOP. }
+ { finallylabel is only used in implicit frames as an exit point from nested try..finally
+ statements, if any. To prevent finalizer from being executed twice, it must come before
+ endtrylabel (bug #34772) }
if catch_frame then
- cg.a_jmp_always(current_asmdata.CurrAsmList,finallylabel)
- else
- emit_nop;
-
- cg.a_label(current_asmdata.CurrAsmList,endtrylabel);
-
- { Handle the except block first, so endtrylabel serves both
- as end of scope and as unwind target. This way it is possible to
- encode everything into a single scope record. }
- if catch_frame then
begin
+ current_asmdata.getjumplabel(templabel);
+ cg.a_label(current_asmdata.CurrAsmList, finallylabel);
+ { jump over exception handler }
+ cg.a_jmp_always(current_asmdata.CurrAsmList,templabel);
+ { Handle the except block first, so endtrylabel serves both
+ as end of scope and as unwind target. This way it is possible to
+ encode everything into a single scope record. }
+ cg.a_label(current_asmdata.CurrAsmList,endtrylabel);
if (current_procinfo.procdef.proccalloption=pocall_safecall) then
begin
handle_safecall_exception;
@@ -305,10 +306,18 @@
end
else
InternalError(2014031601);
+ cg.a_label(current_asmdata.CurrAsmList,templabel);
+ end
+ else
+ begin
+ { same as emit_nop but using finallylabel instead of dummy }
+ cg.a_label(current_asmdata.CurrAsmList,finallylabel);
+ finallylabel.increfs;
+ current_asmdata.CurrAsmList.concat(Taicpu.op_none(A_NOP,S_NO));
+ cg.a_label(current_asmdata.CurrAsmList,endtrylabel);
end;
flowcontrol:=[fc_inflowcontrol];
- cg.a_label(current_asmdata.CurrAsmList,finallylabel);
{ generate finally code as a separate procedure }
if not implicitframe then
tcgprocinfo(current_procinfo).generate_exceptfilter(finalizepi);
|
|
|
34772.pas (847 bytes) |
|
|
I attached the proposed patch which I made some time time ago, and a test, extended to handle the 'safecall' calling convention. The patch solves the issue, but I didn't run the full test suite with it. Hopefully it saves some work. |
|
|
I too have a solution that is still being run through the regression tests, but is somewhat different to yours. We may have to make comparisons when all is done. |
|
|
i34772.patch (2,676 bytes)
Index: compiler/ncgflw.pas
===================================================================
--- compiler/ncgflw.pas (revision 41898)
+++ compiler/ncgflw.pas (working copy)
@@ -424,7 +424,10 @@
secondpass(left);
if (fc_unwind_exit in flowcontrol) then
- hlcg.g_local_unwind(current_asmdata.CurrAsmList,current_procinfo.CurrExitLabel)
+ { Use TrueExitLabel, not CurrExitLabel, because if there's an implicit finally
+ block, this will also be inside the unwinding information, and jumping to
+ CurrExitLabel might call the implicit finally block a second time. [Kit] [Issue #34772] }
+ hlcg.g_local_unwind(current_asmdata.CurrAsmList,current_procinfo.TrueExitLabel)
else
hlcg.a_jmp_always(current_asmdata.CurrAsmList,current_procinfo.CurrExitLabel);
end;
Index: compiler/procinfo.pas
===================================================================
--- compiler/procinfo.pas (revision 41898)
+++ compiler/procinfo.pas (working copy)
@@ -117,6 +117,9 @@
{ label to leave the sub routine }
CurrExitLabel : tasmlabel;
+ { True exit label (used as the exit point for stack unwinding on some platforms) }
+ TrueExitLabel : tasmlabel;
+
{ label for nested exits }
nestedexitlabel : tlabelsym;
@@ -215,6 +218,7 @@
reference_reset(save_regs_ref,sizeof(aint),[]);
{ labels }
current_asmdata.getjumplabel(CurrExitLabel);
+ TrueExitLabel := CurrExitLabel; { This label doesn't change }
current_asmdata.getjumplabel(CurrGOTLabel);
CurrBreakLabel:=nil;
CurrContinueLabel:=nil;
Index: compiler/x86_64/nx64flw.pas
===================================================================
--- compiler/x86_64/nx64flw.pas (revision 41898)
+++ compiler/x86_64/nx64flw.pas (working copy)
@@ -472,7 +472,10 @@
cg.a_label(current_asmdata.CurrAsmList,exitexceptlabel);
cg.g_call(current_asmdata.CurrAsmList,'FPC_DONEEXCEPTION');
if (fc_unwind_exit in oldflowcontrol) then
- cg.g_local_unwind(current_asmdata.CurrAsmList,oldCurrExitLabel)
+ { Use TrueExitLabel, not oldCurrExitLabel, because if there's an implicit finally
+ block, this will also be inside the unwinding information, and jumping to
+ oldCurrExitLabel might call the implicit finally block a second time. [Kit] [Issue #34772] }
+ cg.g_local_unwind(current_asmdata.CurrAsmList,current_procinfo.TrueExitLabel)
else
cg.a_jmp_always(current_asmdata.CurrAsmList,oldCurrExitLabel);
end;
|
|
|
I've written my own patch that also fixes the bug, along with a more extensive test to see how the implicit finally block interacts with explicit ones and also except blocks. This is a little awkward because your patch does fix the bug, so I'm interested to know why you didn't upload it before. Not to blow my own trumpet, but I want to say my patch is SLIGHTLY better because it's a little clearer as to what's going on, and it produces slightly more optimal code when it's the implicit finally block by itself: From the if-statement that calls exit: .Ll8: movq -8(%rbp),%rax cmpl $1,(%rax) je .Lj9 ... Your patch does the following: .Lj9: .Ll11: nop .Lj8: movq %rbp,%rcx call P$TW34772$_$TEST1$array_of_LONGINT_$$_fin$00000001 While mine (and the trunk) does this: .Lj17: .Ll11: nop .Lj8: .Lj9: movq %rbp,%rcx call P$TW34772$_$TEST1$array_of_LONGINT_$$_fin$00000001 The jump is after the "nop" not before, so your code makes the final EXE very slightly more inefficient. I hope you don't take it personally. tw34772.pp (12,098 bytes) |
|
|
I believe the patch "i34772.patch" now fixes the issue. Can you confirm, Martin Friebe? |
|
|
I didn't upload the patch immediately after making it, because I was intending to fully test it and then apply it myself, without uploading here. Since I'm the author of Windows SEH support, fixing this bug was my responsibility :) As for efficiency, putting the exitlabel before 'nop' (or, more precisely, at address before .Lj8) was exactly the goal of my patch. The exitlabel (.Lj9) residing at the same address as end of scope (.Lj8) was the reason of wrong behavior, because it considered the outer finalizer a part of inner 'try' part and executed it once during local_unwind, and then the second time during normal execution. Another possible approach is placing the exitlabel after call to implicit finalizer (that probably is what your patch does, from the looks of it), so that both inner and outer finalizers are executed as part of unwind, but that will somewhat slow down execution of 'exit' statement, because runtime will have to interpret two unwind scopes instead of just one. |
|
|
One thought that sprung to mind as a future optimisation is to not call the unwind routine at all with Exit, but instead just call the finally blocks as regular procedures, since it's not in an error situation. Will that work or am I missing something? |
|
|
You may miss that 'finally' code is allowed to raise its own exceptions. Correctly handling them while executing 'exit' path outside of unwind may complicate code generation too much, if possible at all. Back when I was writing the SEH support, other compilers, namely Delphi and MSVC did not have such optimization. |
|
|
Fair enough. Thanks! Regarding my patch, the logic is that TrueExitLabel is set to the procedure's original exit label (since currExitLabel gets changed when it enters a new try block), and it is this that's passed into the unwind procedure as the parameter, whereas before, it was currExitLabel, which was set to the implicit finally block (or otherwise wasn't the true end point), which is also inside the unwind information, hence got called twice. (Also, my patch is slightly more general-purpose in that if another platform introduces stack unwinding, additional plaform-specific code hopefully won't need to be written because of the TrueExitLabel introduction) |
|
|
Confirmed with Jonas that since Sergei is the maintainer of SEH functionality, the final call as to which patch should be applied is up to him. Sergei, it's in your hands now! |
|
|
Any feedback on this? Either of the two patches fixes the problem, but until one of them is applied, the trunk compiler will generate bad code in the aforementioned instance. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2018-12-28 14:21 | Martin Friebe | New Issue | |
| 2018-12-28 16:40 | J. Gareth Moreton | Priority | normal => high |
| 2018-12-28 16:40 | J. Gareth Moreton | Severity | minor => crash |
| 2018-12-28 17:11 | Cyrax | Note Added: 0112953 | |
| 2018-12-28 17:13 | Cyrax | File Added: project1.zip | |
| 2018-12-28 17:15 | Cyrax | File Added: x86_64-win64_project1.s.zip | |
| 2018-12-28 17:16 | Cyrax | File Added: i386-win32_project1.s.zip | |
| 2018-12-28 17:56 | Cyrax | Note Added: 0112955 | |
| 2018-12-28 18:06 | Martin Friebe | Note Added: 0112957 | |
| 2018-12-28 22:54 | Sergei Gorelkin | Note Added: 0112966 | |
| 2018-12-28 23:08 | Sergei Gorelkin | Note Edited: 0112966 | View Revisions |
| 2018-12-29 18:36 | Bart Broersma | Note Added: 0112985 | |
| 2019-02-28 12:19 | Snus | Note Added: 0114500 | |
| 2019-04-19 08:43 | J. Gareth Moreton | Assigned To | => J. Gareth Moreton |
| 2019-04-19 08:43 | J. Gareth Moreton | Status | new => assigned |
| 2019-04-19 14:37 | Sergei Gorelkin | File Added: nx64flw.pas.patch | |
| 2019-04-19 14:38 | Sergei Gorelkin | File Added: 34772.pas | |
| 2019-04-19 14:41 | Sergei Gorelkin | Note Added: 0115675 | |
| 2019-04-19 14:51 | J. Gareth Moreton | Note Added: 0115676 | |
| 2019-04-19 14:52 | J. Gareth Moreton | Note Edited: 0115676 | View Revisions |
| 2019-04-19 14:52 | J. Gareth Moreton | Note Edited: 0115676 | View Revisions |
| 2019-04-21 04:31 | J. Gareth Moreton | File Added: i34772.patch | |
| 2019-04-21 04:31 | J. Gareth Moreton | File Added: tw34772.pp | |
| 2019-04-21 04:31 | J. Gareth Moreton | Note Added: 0115699 | |
| 2019-04-21 04:33 | J. Gareth Moreton | Assigned To | J. Gareth Moreton => Martin Friebe |
| 2019-04-21 04:33 | J. Gareth Moreton | Status | assigned => feedback |
| 2019-04-21 04:33 | J. Gareth Moreton | Note Added: 0115700 | |
| 2019-04-21 11:00 | Sergei Gorelkin | Note Added: 0115702 | |
| 2019-04-21 13:40 | J. Gareth Moreton | Note Added: 0115704 | |
| 2019-04-21 14:20 | Sergei Gorelkin | Note Added: 0115706 | |
| 2019-04-21 16:04 | J. Gareth Moreton | Note Added: 0115707 | |
| 2019-04-21 18:31 | J. Gareth Moreton | Note Edited: 0115707 | View Revisions |
| 2019-04-22 14:10 | J. Gareth Moreton | Assigned To | Martin Friebe => Sergei Gorelkin |
| 2019-04-22 14:12 | J. Gareth Moreton | Status | feedback => assigned |
| 2019-04-22 14:12 | J. Gareth Moreton | Note Added: 0115726 | |
| 2019-04-22 14:13 | J. Gareth Moreton | Note Edited: 0115707 | View Revisions |
| 2019-05-15 14:45 | J. Gareth Moreton | Note Added: 0116206 |
