| Message ID | 20230314225026.163717-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | sanitizer: missing signed integer overflow errors [PR109107] | expand |
Ping. On Tue, Mar 14, 2023 at 06:50:26PM -0400, Marek Polacek via Gcc-patches wrote: > Here we're failing to detect a signed overflow with -O because match.pd, > since r8-1516, transforms > > c = (a + 1) - (int) (short int) b; > > into > > c = (int) ((unsigned int) a + 4294946117); > > wrongly eliding the overflow. This kind of problems is usually > avoided by using TYPE_OVERFLOW_SANITIZED in the appropriate place. > The first match.pd hunk in the patch fixes it. I've constructed > a testcase for each of the surrounding cases as well. Then I > noticed that fold_binary_loc/associate has the same problem, so I've > added a TYPE_OVERFLOW_SANITIZED there as well (it may be too coarse, > sorry). Then I found yet another problem, but instead of fixing it > now I've opened 109134. I could probably go on and find a dozen more. > > Is this worth doing? > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR sanitizer/109107 > > gcc/ChangeLog: > > * fold-const.cc (fold_binary_loc): Use TYPE_OVERFLOW_SANITIZED > when associating. > * match.pd: Use TYPE_OVERFLOW_SANITIZED. > > gcc/testsuite/ChangeLog: > > * c-c++-common/ubsan/pr109107-2.c: New test. > * c-c++-common/ubsan/pr109107-3.c: New test. > * c-c++-common/ubsan/pr109107-4.c: New test. > * c-c++-common/ubsan/pr109107.c: New test. > --- > gcc/fold-const.cc | 3 ++- > gcc/match.pd | 6 ++--- > gcc/testsuite/c-c++-common/ubsan/pr109107-2.c | 24 ++++++++++++++++++ > gcc/testsuite/c-c++-common/ubsan/pr109107-3.c | 25 +++++++++++++++++++ > gcc/testsuite/c-c++-common/ubsan/pr109107-4.c | 24 ++++++++++++++++++ > gcc/testsuite/c-c++-common/ubsan/pr109107.c | 23 +++++++++++++++++ > 6 files changed, 101 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/ubsan/pr109107-2.c > create mode 100644 gcc/testsuite/c-c++-common/ubsan/pr109107-3.c > create mode 100644 gcc/testsuite/c-c++-common/ubsan/pr109107-4.c > create mode 100644 gcc/testsuite/c-c++-common/ubsan/pr109107.c > > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc > index 02a24c5fe65..8d3308a34e9 100644 > --- a/gcc/fold-const.cc > +++ b/gcc/fold-const.cc > @@ -11319,7 +11319,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type, > And, we need to make sure type is not saturating. */ > > if ((! FLOAT_TYPE_P (type) || flag_associative_math) > - && !TYPE_SATURATING (type)) > + && !TYPE_SATURATING (type) > + && !TYPE_OVERFLOW_SANITIZED (type)) > { > tree var0, minus_var0, con0, minus_con0, lit0, minus_lit0; > tree var1, minus_var1, con1, minus_con1, lit1, minus_lit1; > diff --git a/gcc/match.pd b/gcc/match.pd > index e352bd422f5..98bca9ea388 100644 > --- a/gcc/match.pd > +++ b/gcc/match.pd > @@ -2933,7 +2933,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > /* If the constant operation overflows we cannot do the transform > directly as we would introduce undefined overflow, for example > with (a - 1) + INT_MIN. */ > - (if (types_match (type, @0)) > + (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type)) > (with { tree cst = const_binop (outer_op == inner_op > ? PLUS_EXPR : MINUS_EXPR, > type, @1, @2); } > @@ -2964,7 +2964,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) > || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) > (view_convert (minus (outer_op @1 (view_convert @2)) @0)) > - (if (types_match (type, @0)) > + (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type)) > (with { tree cst = const_binop (outer_op, type, @1, @2); } > (if (cst && !TREE_OVERFLOW (cst)) > (minus { cst; } @0)))))))) > @@ -2983,7 +2983,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) > || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) > (view_convert (plus @0 (minus (view_convert @1) @2))) > - (if (types_match (type, @0)) > + (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type)) > (with { tree cst = const_binop (MINUS_EXPR, type, @1, @2); } > (if (cst && !TREE_OVERFLOW (cst)) > (plus { cst; } @0))))))) > diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107-2.c b/gcc/testsuite/c-c++-common/ubsan/pr109107-2.c > new file mode 100644 > index 00000000000..eb440b58dd8 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107-2.c > @@ -0,0 +1,24 @@ > +/* PR sanitizer/109107 */ > +/* { dg-do run { target int32 } } */ > +/* { dg-options "-fsanitize=signed-integer-overflow" } */ > + > +#define INT_MIN (-__INT_MAX__ - 1) > +int a = INT_MIN; > +const int b = 676540; > + > +__attribute__((noipa)) int > +foo () > +{ > + int c = a - 1 + (int) (short) b; > + return c; > +} > + > +int > +main () > +{ > + foo (); > + return 0; > +} > + > +/* { dg-output "signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*signed integer overflow: 2147483647 \\+ 21180 cannot be represented in type 'int'" } */ > diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107-3.c b/gcc/testsuite/c-c++-common/ubsan/pr109107-3.c > new file mode 100644 > index 00000000000..fa074e7426a > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107-3.c > @@ -0,0 +1,25 @@ > +/* PR sanitizer/109107 */ > +/* { dg-do run { target int32 } } */ > +/* { dg-options "-fsanitize=signed-integer-overflow" } */ > + > +#define INT_MIN (-__INT_MAX__ - 1) > +const int a = INT_MIN; > +const int b = 40; > +int d = 1; > + > +__attribute__((noipa)) int > +foo () > +{ > + int c = a - d + (int) (short) b; > + return c; > +} > + > +int > +main () > +{ > + foo (); > + return 0; > +} > + > +/* { dg-output "signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*signed integer overflow: 2147483647 \\+ 40 cannot be represented in type 'int'" } */ > diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107-4.c b/gcc/testsuite/c-c++-common/ubsan/pr109107-4.c > new file mode 100644 > index 00000000000..b0ac987a15b > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107-4.c > @@ -0,0 +1,24 @@ > +/* PR sanitizer/109107 */ > +/* { dg-do run { target int32 } } */ > +/* { dg-options "-fsanitize=signed-integer-overflow" } */ > + > +#define INT_MIN (-__INT_MAX__ - 1) > +const int x = INT_MIN; > +const int y = -2; > +int a = -3; > + > +__attribute__((noipa)) int > +foo () > +{ > + int c = x - (y - a); > + return c; > +} > + > +int > +main () > +{ > + foo (); > + return 0; > +} > + > +/* { dg-output "signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'" } */ > diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107.c b/gcc/testsuite/c-c++-common/ubsan/pr109107.c > new file mode 100644 > index 00000000000..ca4dd0e3943 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107.c > @@ -0,0 +1,23 @@ > +/* PR sanitizer/109107 */ > +/* { dg-do run { target int32 } } */ > +/* { dg-options "-fsanitize=signed-integer-overflow" } */ > + > +#define INT_MIN (-__INT_MAX__ - 1) > +int a = INT_MIN; > +const int b = 676540; > + > +__attribute__((noipa)) int > +foo () > +{ > + int c = a + 1 - (int) (short) b; > + return c; > +} > + > +int > +main () > +{ > + foo (); > + return 0; > +} > + > +/* { dg-output "signed integer overflow: -2147483647 - 21180 cannot be represented in type 'int'" } */ > > base-commit: 9e44a9932c11f028269f3aa7e3031e703d151b0b > -- > 2.39.2 > Marek
On Tue, Mar 14, 2023 at 06:50:26PM -0400, Marek Polacek via Gcc-patches wrote: > Here we're failing to detect a signed overflow with -O because match.pd, > since r8-1516, transforms > > c = (a + 1) - (int) (short int) b; > > into > > c = (int) ((unsigned int) a + 4294946117); > > wrongly eliding the overflow. This kind of problems is usually > avoided by using TYPE_OVERFLOW_SANITIZED in the appropriate place. > The first match.pd hunk in the patch fixes it. I've constructed > a testcase for each of the surrounding cases as well. Then I > noticed that fold_binary_loc/associate has the same problem, so I've > added a TYPE_OVERFLOW_SANITIZED there as well (it may be too coarse, > sorry). Then I found yet another problem, but instead of fixing it > now I've opened 109134. I could probably go on and find a dozen more. > > Is this worth doing? > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR sanitizer/109107 > > gcc/ChangeLog: > > * fold-const.cc (fold_binary_loc): Use TYPE_OVERFLOW_SANITIZED > when associating. > * match.pd: Use TYPE_OVERFLOW_SANITIZED. > > gcc/testsuite/ChangeLog: > > * c-c++-common/ubsan/pr109107-2.c: New test. > * c-c++-common/ubsan/pr109107-3.c: New test. > * c-c++-common/ubsan/pr109107-4.c: New test. > * c-c++-common/ubsan/pr109107.c: New test. Please rename the last test to pr109107-1.c. Otherwise LGTM. Jakub
diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc index 02a24c5fe65..8d3308a34e9 100644 --- a/gcc/fold-const.cc +++ b/gcc/fold-const.cc @@ -11319,7 +11319,8 @@ fold_binary_loc (location_t loc, enum tree_code code, tree type, And, we need to make sure type is not saturating. */ if ((! FLOAT_TYPE_P (type) || flag_associative_math) - && !TYPE_SATURATING (type)) + && !TYPE_SATURATING (type) + && !TYPE_OVERFLOW_SANITIZED (type)) { tree var0, minus_var0, con0, minus_con0, lit0, minus_lit0; tree var1, minus_var1, con1, minus_con1, lit1, minus_lit1; diff --git a/gcc/match.pd b/gcc/match.pd index e352bd422f5..98bca9ea388 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -2933,7 +2933,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) /* If the constant operation overflows we cannot do the transform directly as we would introduce undefined overflow, for example with (a - 1) + INT_MIN. */ - (if (types_match (type, @0)) + (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type)) (with { tree cst = const_binop (outer_op == inner_op ? PLUS_EXPR : MINUS_EXPR, type, @1, @2); } @@ -2964,7 +2964,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) (view_convert (minus (outer_op @1 (view_convert @2)) @0)) - (if (types_match (type, @0)) + (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type)) (with { tree cst = const_binop (outer_op, type, @1, @2); } (if (cst && !TREE_OVERFLOW (cst)) (minus { cst; } @0)))))))) @@ -2983,7 +2983,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (if (!ANY_INTEGRAL_TYPE_P (TREE_TYPE (@0)) || TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0))) (view_convert (plus @0 (minus (view_convert @1) @2))) - (if (types_match (type, @0)) + (if (types_match (type, @0) && !TYPE_OVERFLOW_SANITIZED (type)) (with { tree cst = const_binop (MINUS_EXPR, type, @1, @2); } (if (cst && !TREE_OVERFLOW (cst)) (plus { cst; } @0))))))) diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107-2.c b/gcc/testsuite/c-c++-common/ubsan/pr109107-2.c new file mode 100644 index 00000000000..eb440b58dd8 --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107-2.c @@ -0,0 +1,24 @@ +/* PR sanitizer/109107 */ +/* { dg-do run { target int32 } } */ +/* { dg-options "-fsanitize=signed-integer-overflow" } */ + +#define INT_MIN (-__INT_MAX__ - 1) +int a = INT_MIN; +const int b = 676540; + +__attribute__((noipa)) int +foo () +{ + int c = a - 1 + (int) (short) b; + return c; +} + +int +main () +{ + foo (); + return 0; +} + +/* { dg-output "signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*signed integer overflow: 2147483647 \\+ 21180 cannot be represented in type 'int'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107-3.c b/gcc/testsuite/c-c++-common/ubsan/pr109107-3.c new file mode 100644 index 00000000000..fa074e7426a --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107-3.c @@ -0,0 +1,25 @@ +/* PR sanitizer/109107 */ +/* { dg-do run { target int32 } } */ +/* { dg-options "-fsanitize=signed-integer-overflow" } */ + +#define INT_MIN (-__INT_MAX__ - 1) +const int a = INT_MIN; +const int b = 40; +int d = 1; + +__attribute__((noipa)) int +foo () +{ + int c = a - d + (int) (short) b; + return c; +} + +int +main () +{ + foo (); + return 0; +} + +/* { dg-output "signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*signed integer overflow: 2147483647 \\+ 40 cannot be represented in type 'int'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107-4.c b/gcc/testsuite/c-c++-common/ubsan/pr109107-4.c new file mode 100644 index 00000000000..b0ac987a15b --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107-4.c @@ -0,0 +1,24 @@ +/* PR sanitizer/109107 */ +/* { dg-do run { target int32 } } */ +/* { dg-options "-fsanitize=signed-integer-overflow" } */ + +#define INT_MIN (-__INT_MAX__ - 1) +const int x = INT_MIN; +const int y = -2; +int a = -3; + +__attribute__((noipa)) int +foo () +{ + int c = x - (y - a); + return c; +} + +int +main () +{ + foo (); + return 0; +} + +/* { dg-output "signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/pr109107.c b/gcc/testsuite/c-c++-common/ubsan/pr109107.c new file mode 100644 index 00000000000..ca4dd0e3943 --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/pr109107.c @@ -0,0 +1,23 @@ +/* PR sanitizer/109107 */ +/* { dg-do run { target int32 } } */ +/* { dg-options "-fsanitize=signed-integer-overflow" } */ + +#define INT_MIN (-__INT_MAX__ - 1) +int a = INT_MIN; +const int b = 676540; + +__attribute__((noipa)) int +foo () +{ + int c = a + 1 - (int) (short) b; + return c; +} + +int +main () +{ + foo (); + return 0; +} + +/* { dg-output "signed integer overflow: -2147483647 - 21180 cannot be represented in type 'int'" } */