View Issue Details

IDProjectCategoryView StatusLast Update
0035603FPCCompilerpublic2019-05-22 20:47
ReporterVille KrumlindeAssigned To 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionduplicate 
Product Version3.3.1Product Build3.3.1 [2019/05/20] for x86_x64 
Target VersionFixed in Version 
Summary0035603: Case statement does not handle out of bounds value of enumerated type, crashes instead
DescriptionSee attached files.

Case statement has generated jump table for enumerated type but does not test that value is in the enumeration, so it reads random address and jumps to it, leading to crash.

Excepted result is that it should jump to "else" statement.

This is in latest trunk build. It was working in Fpc build 3-6 months ago.

In the attached generated asm the case is compiled to this:
# [20] case global1 of
    andl $255,%eax
    leaq Ld1(%rip),%rdx
    movslq (%rdx,%rax,4),%rax
    addq %rdx,%rax
    jmp *%rax

There is no check that the value is in range of the jump table.
Steps To ReproduceBuild attached file and run it. It will crash.
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget-
Attached Files
  • project1.lpr (1,236 bytes)
  • project1.s (6,794 bytes)
  • 35603_case_max_check.patch (1,323 bytes)
    Index: compiler/x86/nx86set.pas
    ===================================================================
    --- compiler/x86/nx86set.pas	(revision 42111)
    +++ compiler/x86/nx86set.pas	(working copy)
    @@ -157,6 +157,7 @@
             current_asmdata.getglobaldatalabel(table);
             { make it a 32bit register }
             indexreg:=cg.makeregsize(current_asmdata.CurrAsmList,hregister,OS_INT);
    +        cg.a_cmp_const_reg_label(current_asmdata.CurrAsmList,opcgsize,OC_A,aint(max_),hregister,elselabel);
             cg.a_load_reg_reg(current_asmdata.CurrAsmList,opcgsize,OS_INT,hregister,indexreg);
             { create reference }
             reference_reset_symbol(href,table,0,sizeof(pint),[]);
    Index: compiler/x86_64/nx64set.pas
    ===================================================================
    --- compiler/x86_64/nx64set.pas	(revision 42111)
    +++ compiler/x86_64/nx64set.pas	(working copy)
    @@ -156,6 +156,7 @@
             { local label in order to avoid using GOT }
             current_asmdata.getlabel(tablelabel,alt_data);
             indexreg:=cg.makeregsize(jtlist,hregister,OS_ADDR);
    +        cg.a_cmp_const_reg_label(jtlist,opcgsize,OC_A,aint(max_),hregister,elselabel);
             cg.a_load_reg_reg(jtlist,opcgsize,OS_ADDR,hregister,indexreg);
             { load table address }
             reference_reset_symbol(href,tablelabel,0,4,[]);
    
  • 35603_case_min_max_check.patch (1,520 bytes)
    Index: compiler/x86/nx86set.pas
    ===================================================================
    --- compiler/x86/nx86set.pas	(revision 42111)
    +++ compiler/x86/nx86set.pas	(working copy)
    @@ -157,6 +157,8 @@
             current_asmdata.getglobaldatalabel(table);
             { make it a 32bit register }
             indexreg:=cg.makeregsize(current_asmdata.CurrAsmList,hregister,OS_INT);
    +        cg.a_cmp_const_reg_label(current_asmdata.CurrAsmList,opcgsize,OC_B,aint(min_),hregister,elselabel);
    +        cg.a_cmp_const_reg_label(current_asmdata.CurrAsmList,opcgsize,OC_A,aint(max_),hregister,elselabel);
             cg.a_load_reg_reg(current_asmdata.CurrAsmList,opcgsize,OS_INT,hregister,indexreg);
             { create reference }
             reference_reset_symbol(href,table,0,sizeof(pint),[]);
    Index: compiler/x86_64/nx64set.pas
    ===================================================================
    --- compiler/x86_64/nx64set.pas	(revision 42111)
    +++ compiler/x86_64/nx64set.pas	(working copy)
    @@ -156,6 +156,8 @@
             { local label in order to avoid using GOT }
             current_asmdata.getlabel(tablelabel,alt_data);
             indexreg:=cg.makeregsize(jtlist,hregister,OS_ADDR);
    +        cg.a_cmp_const_reg_label(jtlist,opcgsize,OC_B,aint(min_),hregister,elselabel);
    +        cg.a_cmp_const_reg_label(jtlist,opcgsize,OC_A,aint(max_),hregister,elselabel);
             cg.a_load_reg_reg(jtlist,opcgsize,OS_ADDR,hregister,indexreg);
             { load table address }
             reference_reset_symbol(href,tablelabel,0,4,[]);
    

Relationships

duplicate of 0032079 resolvedJonas Maebe Jumptables for case..of may jump into invalid memory 

Activities

Ville Krumlinde

2019-05-20 10:17

reporter  

project1.lpr (1,236 bytes)
project1.s (6,794 bytes)

Marģers

2019-05-20 13:24

reporter   ~0116281

As in global1 is typecast none existent enumeration value, taking else statement also is wrong. Case statement covers all legal values, then range check is not necessary.
Some time ago I reported similar issue about arrays and resolution was "no changes required".
Program crash is bad, but only reasonable "solution". It is program error, not compiler error.

J. Gareth Moreton

2019-05-20 13:25

developer   ~0116282

When evaluating case blocks, the compiler assumes that the input value is within the type's domain - it makes no bounds check. This is a conscious decision on the part of the developers because for an enum to take on an out-of-bounds value is usually indictive of a bug elsewhere. The crash unfortunately occurs because the compiler, in this case, creates a linear jump table and ends up trying to branch to an invalid address.

If you think the input variable has a chance of being out of bounds (because it's being directly read from a file, for example), then you need to do the range check manually.

I really hate to say something so stereotypical here... "It's not a bug, it's a feature".

Ville Krumlinde

2019-05-20 14:48

reporter   ~0116284

Last edited: 2019-05-20 14:54

View 2 revisions

I understand what you are saying but:

1. I've been involved in many projects that contain code that has this behavior (relying on case-statement not crashing on invalid enum value) and this is the first time I've seen it crash. And recently it did not crash in Fpc either so something must have changed. And this change will break existing code. I've coded in Pascal since 1985, professionally every work day since 1990. At the very least it could keep the old behavior for Delphi-mode.

2. Setting a enum to a special magic value like $FF can be an indicator that this value is "unused", and is useful for size-sensitive packed records where adding another flag would waste memory.

3. Find crash bugs like this is difficult because the jump to address outside current function body messes up the callstack in debugger.

4. Both GCC and Clang C++ compilers test that enum value is not invalid before jumping into jump table, check the generated code here: https://godbolt.org/z/Rq00k2
In my opinion it should not be possible to crash a case-statement based on a invalid value.

Please consider fixing this.

Martok

2019-05-20 15:47

reporter   ~0116286

@Kit:
> If you think the input variable has a chance of being out of bounds (because it's being directly read from a file, for example), then you need to do the range check manually.
It's actually fairly hard to do this, since the compiler may remove the check at any time - after all, the condition tested for "can't" happen. Ondrej proposed a solution some time ago, that was also rejected.

@VK:
FPC was always the *only* compiler doing this. It only crashes a lot more user code since the weight for jumptables was reduced to fairly short types. I reported the same in 2017 as 0032079. Feel free to apply the patch posted there, which is well tested, safe, and (together with 0033093 v3) costs no cycles.
And yes, this broke about 30 years worth of code for me (or actually, it didn't, because we ended up staying with Delphi for that very reason).

J. Gareth Moreton

2019-05-20 22:10

developer   ~0116293

The nuances of case blocks have been a point of contention lately. I unfortunately can't promise anything will change, but I'll see what can be brought up, especially as the behaviour of FPC seems to stand in stark contrast to other dialects of Pascal when it comes to this.

And people rejecting the use of a tool because of a language characteristic is a worrying prospect that one should at least pay attention to.

Do-wan Kim

2019-05-21 02:51

reporter  

35603_case_max_check.patch (1,323 bytes)
Index: compiler/x86/nx86set.pas
===================================================================
--- compiler/x86/nx86set.pas	(revision 42111)
+++ compiler/x86/nx86set.pas	(working copy)
@@ -157,6 +157,7 @@
         current_asmdata.getglobaldatalabel(table);
         { make it a 32bit register }
         indexreg:=cg.makeregsize(current_asmdata.CurrAsmList,hregister,OS_INT);
+        cg.a_cmp_const_reg_label(current_asmdata.CurrAsmList,opcgsize,OC_A,aint(max_),hregister,elselabel);
         cg.a_load_reg_reg(current_asmdata.CurrAsmList,opcgsize,OS_INT,hregister,indexreg);
         { create reference }
         reference_reset_symbol(href,table,0,sizeof(pint),[]);
Index: compiler/x86_64/nx64set.pas
===================================================================
--- compiler/x86_64/nx64set.pas	(revision 42111)
+++ compiler/x86_64/nx64set.pas	(working copy)
@@ -156,6 +156,7 @@
         { local label in order to avoid using GOT }
         current_asmdata.getlabel(tablelabel,alt_data);
         indexreg:=cg.makeregsize(jtlist,hregister,OS_ADDR);
+        cg.a_cmp_const_reg_label(jtlist,opcgsize,OC_A,aint(max_),hregister,elselabel);
         cg.a_load_reg_reg(jtlist,opcgsize,OS_ADDR,hregister,indexreg);
         { load table address }
         reference_reset_symbol(href,tablelabel,0,4,[]);

Do-wan Kim

2019-05-21 03:53

reporter   ~0116298

min, max check on case patch. it looks good but I don't know about side effect.

35603_case_min_max_check.patch (1,520 bytes)
Index: compiler/x86/nx86set.pas
===================================================================
--- compiler/x86/nx86set.pas	(revision 42111)
+++ compiler/x86/nx86set.pas	(working copy)
@@ -157,6 +157,8 @@
         current_asmdata.getglobaldatalabel(table);
         { make it a 32bit register }
         indexreg:=cg.makeregsize(current_asmdata.CurrAsmList,hregister,OS_INT);
+        cg.a_cmp_const_reg_label(current_asmdata.CurrAsmList,opcgsize,OC_B,aint(min_),hregister,elselabel);
+        cg.a_cmp_const_reg_label(current_asmdata.CurrAsmList,opcgsize,OC_A,aint(max_),hregister,elselabel);
         cg.a_load_reg_reg(current_asmdata.CurrAsmList,opcgsize,OS_INT,hregister,indexreg);
         { create reference }
         reference_reset_symbol(href,table,0,sizeof(pint),[]);
Index: compiler/x86_64/nx64set.pas
===================================================================
--- compiler/x86_64/nx64set.pas	(revision 42111)
+++ compiler/x86_64/nx64set.pas	(working copy)
@@ -156,6 +156,8 @@
         { local label in order to avoid using GOT }
         current_asmdata.getlabel(tablelabel,alt_data);
         indexreg:=cg.makeregsize(jtlist,hregister,OS_ADDR);
+        cg.a_cmp_const_reg_label(jtlist,opcgsize,OC_B,aint(min_),hregister,elselabel);
+        cg.a_cmp_const_reg_label(jtlist,opcgsize,OC_A,aint(max_),hregister,elselabel);
         cg.a_load_reg_reg(jtlist,opcgsize,OS_ADDR,hregister,indexreg);
         { load table address }
         reference_reset_symbol(href,tablelabel,0,4,[]);

Ville Krumlinde

2019-05-21 10:37

reporter   ~0116300

Last edited: 2019-05-21 10:44

View 3 revisions

@Martok: Thanks for info and patch. I can see you fought for this issue before and I understand your frustration.

@Do-wan-kim: thanks for patch.

@J. Gareth Moreton: Yes any issue (no matter how minor) that stops code working on the first attempt when people try Fpc will be an obstacle for new users. Additionally this issue only happens when a jump-table is used (otherwise the code works) so the code will break depending on Fpcs internal logic when to use jump-table. Which means code will crash in such situation as
1) user adds additional case-labels
2) user increase Fpc optimization level
3) user upgrades Fpc.
And since reason of crash is difficult to find it is likely the user will continue using older solution.

As for when this started, Fpc trunk revision 38654 did not have this behavior. Same code compiles to this:
# [20] case global1 of
    subl $3,%eax
    cmpl $34,%eax
    ja Lj8
    andl $4294967295,%eax
    leaq Ld1(%rip),%rdx
    movslq (%rdx,%rax,4),%rax
    addq %rdx,%rax
    jmp *%rax

Lj8 is the "else" so any out of range value will go to else-label.

nanobit

2019-05-21 10:50

reporter   ~0116301

I agree, this should be fixed. There is no need for an arbitrary crash,
internally in the selector. It's easy to say, implement own domain checks
(because not implicit in "case"), but who can control all third party sources.
The docs essentially say: Else-branch means "outside" by negation-of-inside.
This can be seen as binary test, alike: if isNumber( NAN) then ... else ...
The programmer can examine the outlier (NAN) inside the else-branch,
if filtering-out was not the only intent.

Denis Golovan

2019-05-21 10:55

reporter   ~0116302

Last edited: 2019-05-21 13:42

View 2 revisions

It's a bit scary.
Please fix this, as whole lot of old 3rd party Delphi components rely on "else" triggering in this case.

Florian

2019-05-22 20:47

administrator   ~0116346

The issue was discussed already in the duplicate report.

About the patch: please note that it does not change all targets supported by FPC.

Issue History

Date Modified Username Field Change
2019-05-20 10:17 Ville Krumlinde New Issue
2019-05-20 10:17 Ville Krumlinde File Added: project1.lpr
2019-05-20 10:17 Ville Krumlinde File Added: project1.s
2019-05-20 13:24 Marģers Note Added: 0116281
2019-05-20 13:25 J. Gareth Moreton Note Added: 0116282
2019-05-20 14:48 Ville Krumlinde Note Added: 0116284
2019-05-20 14:54 Ville Krumlinde Note Edited: 0116284 View Revisions
2019-05-20 15:47 Martok Note Added: 0116286
2019-05-20 22:10 J. Gareth Moreton Note Added: 0116293
2019-05-21 02:51 Do-wan Kim File Added: 35603_case_max_check.patch
2019-05-21 03:53 Do-wan Kim File Added: 35603_case_min_max_check.patch
2019-05-21 03:53 Do-wan Kim Note Added: 0116298
2019-05-21 10:37 Ville Krumlinde Note Added: 0116300
2019-05-21 10:44 Ville Krumlinde Note Edited: 0116300 View Revisions
2019-05-21 10:44 Ville Krumlinde Note Edited: 0116300 View Revisions
2019-05-21 10:50 nanobit Note Added: 0116301
2019-05-21 10:55 Denis Golovan Note Added: 0116302
2019-05-21 13:42 J. Gareth Moreton Note Edited: 0116302 View Revisions
2019-05-22 20:46 Florian Relationship added duplicate of 0032079
2019-05-22 20:47 Florian Status new => resolved
2019-05-22 20:47 Florian Resolution open => duplicate
2019-05-22 20:47 Florian FPCTarget => -
2019-05-22 20:47 Florian Note Added: 0116346