[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598
Julien Blanc changed:
What|Removed |Added
CC||julien.blanc at sprinte dot eu
--- Comment #9 from Julien Blanc ---
I recently ran into this while trying to use -fsanitize=enum for the codebase.
The situation is worse for C++ enum class : since you can only assign them with
static_cast, no load check can be done (clang also fails to detect such cases).
Here’s a sample code that should trigger but does not :
#include
enum class Foo
{
foo1 = 0,
foo2 = 1
};
std::ostream& operator<<(std::ostream& o, Foo foo)
{
switch(foo)
{
case Foo::foo1:
return o << "foo1";
case Foo::foo2:
return o << "foo2";
}
return o << "unknown";
}
int main()
{
Foo foo = static_cast(3);
std::cout << foo << std::endl;
return 0;
}
$ g++ -fsanitize=enum -fsanitize=undefined -fno-sanitize-recover enum.cpp
$ ./a.out
unknown
$
Note that clang++ is no better :
$ clang++-7 -fsanitize=enum -fsanitize=undefined -fno-sanitize-recover=all
enum.cpp
$ ./a.out
unknown
$
But i’d expect both checkers to detect such misuse. While it could be valid
code, there’s a high chance that its a bug. Ideally an attribute could be used
if it is expected behaviour.
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598
--- Comment #8 from Martin Liška ---
(In reply to Jakub Jelinek from comment #7)
> As for the switch, I think there shouldn't be any runtime error, there is no
> UB.
> You can have default: label even if you list all the possible in-range
> cases, you can have enum values in between min/max that aren't in the
> enumeration, and you can have case labels outside of the enum range too,
> that is something for a warning, but not a runtime error.
I've just noticed that in this example:
cat enum2.c
int main(int argc, char **argv)
{
char c = (char)argc;
bool x = argc;
switch (c)
{
case -128 ... 127:
break;
default:
return 2;
}
switch (x)
{
case true:
return 2;
case false:
return 3;
default:
return 4;
}
}
We are able to simplify the second switch in vrp:
[...]
Folding statement: switch (x_4) [33.33%], case 0: [33.33%],
case 1: [33.33%]>
removing unreachable case label
[...]
But we can't do it for the first switch. That's probably missing optimization.
I'm interested in the case where we've got call enum values covered and default
is provided: would it be valid to
simply remove the block (via __builtin_unreachable or similarly)?
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598 --- Comment #7 from Jakub Jelinek --- As for the switch, I think there shouldn't be any runtime error, there is no UB. You can have default: label even if you list all the possible in-range cases, you can have enum values in between min/max that aren't in the enumeration, and you can have case labels outside of the enum range too, that is something for a warning, but not a runtime error.
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598 --- Comment #6 from Jakub Jelinek --- The extra instrumention should be on NOP_EXPR/CONVERT_EXPR (dunno about VIEW_CONVERT_EXPR) from some type to enum type if the from type isn't the same enum type or some type with equal min/max values. But given that we consider such conversions useless: /* If both the inner and outer types are integral types, then the conversion is not necessary if they have the same mode and signedness and precision, and both or neither are boolean. */ we'd need to instrument that early (either during genericization, or gimplification at latest). It could be done by turning the check into some ifn and lowering it later (sanopt) of course.
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598
--- Comment #5 from Martin Liška ---
(In reply to Marek Polacek from comment #4)
> But you get compile-time warnings for that (-Wswitch), no?
For:
cat enum.c
enum values
{
A = 1000,
B = 30,
C = 100
};
int main(int argc, char **argv)
{
enum values x = (enum values)argc;
if (x == 12345)
return 1;
switch (x)
{
case A:
return 1;
case B:
return 1;
case C:
return 2;
case 123:
return 3;
default:
return 4;
}
}
I only have warning for 'case 123':
gcc -fsanitize=enum enum.c -Wall
enum.c: In function ‘main’:
enum.c:22:5: warning: case value ‘123’ not in enumerated type ‘enum values’
[-Wswitch]
case 123:
^~~~
But one could also produce warning for the if comparison and default bb in the
switch statement.
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598 --- Comment #4 from Marek Polacek --- But you get compile-time warnings for that (-Wswitch), no?
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598
Martin Liška changed:
What|Removed |Added
Status|UNCONFIRMED |NEW
Last reconfirmed||2017-08-03
CC||mpolacek at gcc dot gnu.org
Ever confirmed|0 |1
Severity|normal |enhancement
--- Comment #3 from Martin Liška ---
I believe we can instrument more in order to catch more situations:
enum values
{
A = 1000,
B = 30,
C = 100
};
enum values g;
int main(int argc, char **argv)
{
enum values x = (enum values)argc;
if (x == 12345)
return 1;
return g;
switch (x)
{
case A:
return 1;
case C:
return 2;
case 123:
return 3;
}
}
In this case, CFG is based on values that are undefined. As I've been working
on research of switch statement, I noticed that it's very common that switch
covers all possibly values of an enumeral type. Having that, we can instrument
default label with some UBSAN call.
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598
--- Comment #2 from Tim Ruehsen ---
(In reply to Jakub Jelinek from comment #1)
> This isn't a load, it is a cast, we sanitize just loads from memory.
Hmmm, seems ok if the compiler doesn't warn.
But the sanitizer IMO should trigger.
What if this cast has been done in a function returning flag_t ?
This could even be buggy code in an external library.
Then the sanitizer should definitely trigger.
And it doesn't with this code:
#include
typedef enum {
FLAG1 = (1 << 0),
FLAG2 = (1 << 1),
} flag_t;
static flag_t setter(int x)
{
return (flag_t) x;
}
int main(void)
{
int x = 5;
flag_t flags = setter(x);
printf("flags = %X\n", flags);
return 0;
}
$ g++-7 -O0 -fsanitize=undefined -fsanitize=enum -fno-sanitize-recover
enum_undef.cc
$ ./a.out
flags = 5
Hopefully, -O0 doesn't optimize into a single cast.
[Bug sanitizer/81598] -fsanitize=enum does not detect range violation
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81598 --- Comment #1 from Jakub Jelinek --- This isn't a load, it is a cast, we sanitize just loads from memory. >From the almost non-existent testsuite coverage in LLVM it is hard to find out what exactly they decided to implement (-fsanitize=enum has a single testcase with a single memory load there, -fsanitize=bool 2 testcases with a single memory load each).
