Re: [PATCH] sanitizer: missing signed integer overflow errors [PR109107]
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
Re: [PATCH] sanitizer: missing signed integer overflow errors [PR109107]
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 000..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
[PATCH] sanitizer: missing signed integer overflow errors [PR109107]
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 000..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'" } */
