* [PATCH] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
@ 2023-03-08 21:09 Marek Polacek
2023-03-09 8:12 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Marek Polacek @ 2023-03-08 21:09 UTC (permalink / raw)
To: GCC Patches; +Cc: Jakub Jelinek, Richard Biener
In this PR we are dealing with a missing .UBSAN_BOUNDS, so the
out-of-bounds access in the test makes the program crash before
a UBSan diagnostic was emitted. In C and C++, c_genericize gets
a[b] = a[b] | c;
but in C, both a[b] are one identical shared tree (not in C++ because
cp_fold/ARRAY_REF created two same but not identical trees). Since
ubsan_walk_array_refs_r keeps a pset, in C we produce
a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] = a[b] | c;
because the LHS is walked before the RHS.
Since r7-1900, we gimplify the RHS before the LHS. So the statement above
gets gimplified into
_1 = a[b];
c.0_2 = c;
b.1 = b;
.UBSAN_BOUNDS (0B, b.1, 8);
With this patch we produce:
a[b] = a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] | c;
which gets gimplified into:
b.0 = b;
.UBSAN_BOUNDS (0B, b.0, 8);
_1 = a[b.0];
therefore we emit a runtime error before making the bad array access.
I think it's OK that only the RHS gets a .UBSAN_BOUNDS, as in few lines
above: the instrumented array access dominates the array access on the
LHS, and I've verified that
b = 0;
a[b] = (a[b], b = -32768, a[0] | c);
works as expected: the inner a[b] is OK but we do emit an error for the
a[b] on the LHS.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/12?
PR sanitizer/108060
PR sanitizer/109050
gcc/c-family/ChangeLog:
* c-gimplify.cc (ubsan_walk_array_refs_r): For a MODIFY_EXPR, instrument
the RHS before the LHS.
gcc/testsuite/ChangeLog:
* c-c++-common/ubsan/bounds-17.c: New test.
* c-c++-common/ubsan/bounds-18.c: New test.
* c-c++-common/ubsan/bounds-19.c: New test.
* c-c++-common/ubsan/bounds-20.c: New test.
---
gcc/c-family/c-gimplify.cc | 12 ++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-17.c | 17 +++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-18.c | 17 +++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-19.c | 20 ++++++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-20.c | 16 ++++++++++++++++
5 files changed, 82 insertions(+)
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-17.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-18.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-19.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-20.c
diff --git a/gcc/c-family/c-gimplify.cc b/gcc/c-family/c-gimplify.cc
index 74b276b2b26..ef5c7d919fc 100644
--- a/gcc/c-family/c-gimplify.cc
+++ b/gcc/c-family/c-gimplify.cc
@@ -106,6 +106,18 @@ ubsan_walk_array_refs_r (tree *tp, int *walk_subtrees, void *data)
}
else if (TREE_CODE (*tp) == ARRAY_REF)
ubsan_maybe_instrument_array_ref (tp, false);
+ else if (TREE_CODE (*tp) == MODIFY_EXPR)
+ {
+ /* Since r7-1900, we gimplify RHS before LHS. Consider
+ a[b] |= c;
+ wherein we can have a single shared tree a[b] in both LHS and RHS.
+ If we only instrument the LHS and the access is invalid, the program
+ could crash before emitting a UBSan error. So instrument the RHS
+ first. */
+ *walk_subtrees = 0;
+ walk_tree (&TREE_OPERAND (*tp, 1), ubsan_walk_array_refs_r, pset, pset);
+ walk_tree (&TREE_OPERAND (*tp, 0), ubsan_walk_array_refs_r, pset, pset);
+ }
return NULL_TREE;
}
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-17.c b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c
new file mode 100644
index 00000000000..b727e3235b8
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c
@@ -0,0 +1,17 @@
+/* PR sanitizer/108060 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } "-flto" } */
+/* { dg-shouldfail "ubsan" } */
+
+int a[8];
+int c;
+
+int
+main ()
+{
+ int b = -32768;
+ a[b] |= c;
+}
+
+/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-18.c b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c
new file mode 100644
index 00000000000..556abc0e1c0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c
@@ -0,0 +1,17 @@
+/* PR sanitizer/108060 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } "-flto" } */
+/* { dg-shouldfail "ubsan" } */
+
+int a[8];
+int c;
+
+int
+main ()
+{
+ int b = -32768;
+ a[b] = a[b] | c;
+}
+
+/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-19.c b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c
new file mode 100644
index 00000000000..54217ae399f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c
@@ -0,0 +1,20 @@
+/* PR sanitizer/108060 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } "-flto" } */
+/* { dg-shouldfail "ubsan" } */
+
+int a[8];
+int a2[18];
+int c;
+
+int
+main ()
+{
+ int b = 0;
+ a[0] = (a2[b], b = -32768, a[0] | c);
+ b = 0;
+ a[b] = (a[b], b = -32768, a[0] | c);
+}
+
+/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-20.c b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c
new file mode 100644
index 00000000000..a78c67129e0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c
@@ -0,0 +1,16 @@
+/* PR sanitizer/109050 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds -fno-sanitize-recover=all" } */
+/* { dg-shouldfail "ubsan" } */
+
+long a;
+int b;
+int
+main ()
+{
+ int c[4] = {0, 1, 2, 3};
+ a = 0;
+ c[a - 9806816] |= b;
+}
+
+/* { dg-output "index -9806816 out of bounds for type 'int \\\[4\\\]'" } */
base-commit: 2e3dd14dd287ca94b72c36ed28a1ae30887f77ce
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
2023-03-08 21:09 [PATCH] ubsan: missed -fsanitize=bounds for compound ops [PR108060] Marek Polacek
@ 2023-03-09 8:12 ` Richard Biener
2023-03-09 8:44 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2023-03-09 8:12 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches, Jakub Jelinek
On Wed, 8 Mar 2023, Marek Polacek wrote:
> In this PR we are dealing with a missing .UBSAN_BOUNDS, so the
> out-of-bounds access in the test makes the program crash before
> a UBSan diagnostic was emitted. In C and C++, c_genericize gets
>
> a[b] = a[b] | c;
>
> but in C, both a[b] are one identical shared tree (not in C++ because
> cp_fold/ARRAY_REF created two same but not identical trees). Since
> ubsan_walk_array_refs_r keeps a pset, in C we produce
>
> a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] = a[b] | c;
>
> because the LHS is walked before the RHS.
>
> Since r7-1900, we gimplify the RHS before the LHS. So the statement above
> gets gimplified into
>
> _1 = a[b];
> c.0_2 = c;
> b.1 = b;
> .UBSAN_BOUNDS (0B, b.1, 8);
>
> With this patch we produce:
>
> a[b] = a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] | c;
>
> which gets gimplified into:
>
> b.0 = b;
> .UBSAN_BOUNDS (0B, b.0, 8);
> _1 = a[b.0];
>
> therefore we emit a runtime error before making the bad array access.
>
> I think it's OK that only the RHS gets a .UBSAN_BOUNDS, as in few lines
> above: the instrumented array access dominates the array access on the
> LHS, and I've verified that
>
> b = 0;
> a[b] = (a[b], b = -32768, a[0] | c);
>
> works as expected: the inner a[b] is OK but we do emit an error for the
> a[b] on the LHS.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/12?
I think this is a reasonable way to address the regression, so OK.
Thanks,
Richard.
> PR sanitizer/108060
> PR sanitizer/109050
>
> gcc/c-family/ChangeLog:
>
> * c-gimplify.cc (ubsan_walk_array_refs_r): For a MODIFY_EXPR, instrument
> the RHS before the LHS.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/ubsan/bounds-17.c: New test.
> * c-c++-common/ubsan/bounds-18.c: New test.
> * c-c++-common/ubsan/bounds-19.c: New test.
> * c-c++-common/ubsan/bounds-20.c: New test.
> ---
> gcc/c-family/c-gimplify.cc | 12 ++++++++++++
> gcc/testsuite/c-c++-common/ubsan/bounds-17.c | 17 +++++++++++++++++
> gcc/testsuite/c-c++-common/ubsan/bounds-18.c | 17 +++++++++++++++++
> gcc/testsuite/c-c++-common/ubsan/bounds-19.c | 20 ++++++++++++++++++++
> gcc/testsuite/c-c++-common/ubsan/bounds-20.c | 16 ++++++++++++++++
> 5 files changed, 82 insertions(+)
> create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-17.c
> create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-18.c
> create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-19.c
> create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-20.c
>
> diff --git a/gcc/c-family/c-gimplify.cc b/gcc/c-family/c-gimplify.cc
> index 74b276b2b26..ef5c7d919fc 100644
> --- a/gcc/c-family/c-gimplify.cc
> +++ b/gcc/c-family/c-gimplify.cc
> @@ -106,6 +106,18 @@ ubsan_walk_array_refs_r (tree *tp, int *walk_subtrees, void *data)
> }
> else if (TREE_CODE (*tp) == ARRAY_REF)
> ubsan_maybe_instrument_array_ref (tp, false);
> + else if (TREE_CODE (*tp) == MODIFY_EXPR)
> + {
> + /* Since r7-1900, we gimplify RHS before LHS. Consider
> + a[b] |= c;
> + wherein we can have a single shared tree a[b] in both LHS and RHS.
> + If we only instrument the LHS and the access is invalid, the program
> + could crash before emitting a UBSan error. So instrument the RHS
> + first. */
> + *walk_subtrees = 0;
> + walk_tree (&TREE_OPERAND (*tp, 1), ubsan_walk_array_refs_r, pset, pset);
> + walk_tree (&TREE_OPERAND (*tp, 0), ubsan_walk_array_refs_r, pset, pset);
> + }
> return NULL_TREE;
> }
>
> diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-17.c b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c
> new file mode 100644
> index 00000000000..b727e3235b8
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c
> @@ -0,0 +1,17 @@
> +/* PR sanitizer/108060 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=bounds" } */
> +/* { dg-skip-if "" { *-*-* } "-flto" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int a[8];
> +int c;
> +
> +int
> +main ()
> +{
> + int b = -32768;
> + a[b] |= c;
> +}
> +
> +/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
> diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-18.c b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c
> new file mode 100644
> index 00000000000..556abc0e1c0
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c
> @@ -0,0 +1,17 @@
> +/* PR sanitizer/108060 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=bounds" } */
> +/* { dg-skip-if "" { *-*-* } "-flto" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int a[8];
> +int c;
> +
> +int
> +main ()
> +{
> + int b = -32768;
> + a[b] = a[b] | c;
> +}
> +
> +/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
> diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-19.c b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c
> new file mode 100644
> index 00000000000..54217ae399f
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c
> @@ -0,0 +1,20 @@
> +/* PR sanitizer/108060 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=bounds" } */
> +/* { dg-skip-if "" { *-*-* } "-flto" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int a[8];
> +int a2[18];
> +int c;
> +
> +int
> +main ()
> +{
> + int b = 0;
> + a[0] = (a2[b], b = -32768, a[0] | c);
> + b = 0;
> + a[b] = (a[b], b = -32768, a[0] | c);
> +}
> +
> +/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
> diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-20.c b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c
> new file mode 100644
> index 00000000000..a78c67129e0
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c
> @@ -0,0 +1,16 @@
> +/* PR sanitizer/109050 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=bounds -fno-sanitize-recover=all" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +long a;
> +int b;
> +int
> +main ()
> +{
> + int c[4] = {0, 1, 2, 3};
> + a = 0;
> + c[a - 9806816] |= b;
> +}
> +
> +/* { dg-output "index -9806816 out of bounds for type 'int \\\[4\\\]'" } */
>
> base-commit: 2e3dd14dd287ca94b72c36ed28a1ae30887f77ce
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
2023-03-09 8:12 ` Richard Biener
@ 2023-03-09 8:44 ` Jakub Jelinek
2023-03-10 0:44 ` [PATCH v2] " Marek Polacek
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2023-03-09 8:44 UTC (permalink / raw)
To: Richard Biener; +Cc: Marek Polacek, GCC Patches
On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote:
> I think this is a reasonable way to address the regression, so OK.
It is true that both C and C++ (including c++14_down and c++17 and later
where the latter have different ordering rules) evaluate the lhs of
MODIFY_EXPR after rhs, so conceptually this patch makes sense.
But I wonder why we do in ubsan_maybe_instrument_array_ref:
if (e != NULL_TREE)
{
tree t = copy_node (*expr_p);
TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
e, op1);
*expr_p = t;
}
rather than modification of the ARRAY_REF's operand in place. If we
did that, we wouldn't really care about the order, shared tree would
be instrumented once, with SAVE_EXPR in there making sure we don't
compute that multiple times. Is that because the 2 copies could
have side-effects and we do want to evaluate those multiple times?
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
2023-03-09 8:44 ` Jakub Jelinek
@ 2023-03-10 0:44 ` Marek Polacek
2023-03-10 18:07 ` Jakub Jelinek
0 siblings, 1 reply; 6+ messages in thread
From: Marek Polacek @ 2023-03-10 0:44 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, GCC Patches
On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote:
> On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote:
> > I think this is a reasonable way to address the regression, so OK.
>
> It is true that both C and C++ (including c++14_down and c++17 and later
> where the latter have different ordering rules) evaluate the lhs of
> MODIFY_EXPR after rhs, so conceptually this patch makes sense.
Thank you both for taking a look.
> But I wonder why we do in ubsan_maybe_instrument_array_ref:
> if (e != NULL_TREE)
> {
> tree t = copy_node (*expr_p);
> TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
> e, op1);
> *expr_p = t;
> }
> rather than modification of the ARRAY_REF's operand in place. If we
> did that, we wouldn't really care about the order, shared tree would
> be instrumented once, with SAVE_EXPR in there making sure we don't
> compute that multiple times. Is that because the 2 copies could
> have side-effects and we do want to evaluate those multiple times?
I'd assumed that that was the point of the copy_node. But now that
I'm actually experimenting with it, I can't trigger any problems
without the copy_node. So maybe we can use the following patch, which
also adds a new test, bounds-21.c, to check that side-effects are
evaluated correctly. I didn't bother writing a description for this
patch yet because I sort of think we should apply both patches at the
same time.
Regtested on x86_64-pc-linux-gnu.
-- >8 --
PR sanitizer/108060
PR sanitizer/109050
gcc/c-family/ChangeLog:
* c-ubsan.cc (ubsan_maybe_instrument_array_ref): Don't copy_node.
gcc/testsuite/ChangeLog:
* c-c++-common/ubsan/bounds-17.c: New test.
* c-c++-common/ubsan/bounds-18.c: New test.
* c-c++-common/ubsan/bounds-19.c: New test.
* c-c++-common/ubsan/bounds-20.c: New test.
* c-c++-common/ubsan/bounds-21.c: New test.
---
gcc/c-family/c-ubsan.cc | 8 ++------
gcc/testsuite/c-c++-common/ubsan/bounds-17.c | 17 +++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-18.c | 17 +++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-19.c | 20 ++++++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-20.c | 16 ++++++++++++++++
gcc/testsuite/c-c++-common/ubsan/bounds-21.c | 18 ++++++++++++++++++
6 files changed, 90 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-17.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-18.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-19.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-20.c
create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-21.c
diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc
index 3e24198d7bb..8ce6421b61a 100644
--- a/gcc/c-family/c-ubsan.cc
+++ b/gcc/c-family/c-ubsan.cc
@@ -505,12 +505,8 @@ ubsan_maybe_instrument_array_ref (tree *expr_p, bool ignore_off_by_one)
tree e = ubsan_instrument_bounds (EXPR_LOCATION (*expr_p), op0, &op1,
ignore_off_by_one);
if (e != NULL_TREE)
- {
- tree t = copy_node (*expr_p);
- TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
- e, op1);
- *expr_p = t;
- }
+ TREE_OPERAND (*expr_p, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
+ e, op1);
}
}
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-17.c b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c
new file mode 100644
index 00000000000..b727e3235b8
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c
@@ -0,0 +1,17 @@
+/* PR sanitizer/108060 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } "-flto" } */
+/* { dg-shouldfail "ubsan" } */
+
+int a[8];
+int c;
+
+int
+main ()
+{
+ int b = -32768;
+ a[b] |= c;
+}
+
+/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-18.c b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c
new file mode 100644
index 00000000000..556abc0e1c0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c
@@ -0,0 +1,17 @@
+/* PR sanitizer/108060 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } "-flto" } */
+/* { dg-shouldfail "ubsan" } */
+
+int a[8];
+int c;
+
+int
+main ()
+{
+ int b = -32768;
+ a[b] = a[b] | c;
+}
+
+/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-19.c b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c
new file mode 100644
index 00000000000..54217ae399f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c
@@ -0,0 +1,20 @@
+/* PR sanitizer/108060 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds" } */
+/* { dg-skip-if "" { *-*-* } "-flto" } */
+/* { dg-shouldfail "ubsan" } */
+
+int a[8];
+int a2[18];
+int c;
+
+int
+main ()
+{
+ int b = 0;
+ a[0] = (a2[b], b = -32768, a[0] | c);
+ b = 0;
+ a[b] = (a[b], b = -32768, a[0] | c);
+}
+
+/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-20.c b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c
new file mode 100644
index 00000000000..a78c67129e0
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c
@@ -0,0 +1,16 @@
+/* PR sanitizer/109050 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds -fno-sanitize-recover=all" } */
+/* { dg-shouldfail "ubsan" } */
+
+long a;
+int b;
+int
+main ()
+{
+ int c[4] = {0, 1, 2, 3};
+ a = 0;
+ c[a - 9806816] |= b;
+}
+
+/* { dg-output "index -9806816 out of bounds for type 'int \\\[4\\\]'" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-21.c b/gcc/testsuite/c-c++-common/ubsan/bounds-21.c
new file mode 100644
index 00000000000..b9d9308849f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/bounds-21.c
@@ -0,0 +1,18 @@
+/* PR sanitizer/109050 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds -fno-sanitize-recover=all" } */
+
+int i;
+int foo (void) { return ++i; }
+
+int
+main ()
+{
+ char a[10] = { };
+ a[foo ()] = a[foo()] | 'a';
+ if (i != 2)
+ __builtin_abort ();
+ a[foo()] |= 'a';
+ if (i != 3)
+ __builtin_abort ();
+}
base-commit: f366fdfeec0af6cda716de913c32e48f9b1e3a0e
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
2023-03-10 0:44 ` [PATCH v2] " Marek Polacek
@ 2023-03-10 18:07 ` Jakub Jelinek
2023-03-10 18:09 ` Marek Polacek
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2023-03-10 18:07 UTC (permalink / raw)
To: Marek Polacek; +Cc: Richard Biener, GCC Patches
On Thu, Mar 09, 2023 at 07:44:53PM -0500, Marek Polacek wrote:
> On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote:
> > On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote:
> > > I think this is a reasonable way to address the regression, so OK.
> >
> > It is true that both C and C++ (including c++14_down and c++17 and later
> > where the latter have different ordering rules) evaluate the lhs of
> > MODIFY_EXPR after rhs, so conceptually this patch makes sense.
>
> Thank you both for taking a look.
>
> > But I wonder why we do in ubsan_maybe_instrument_array_ref:
> > if (e != NULL_TREE)
> > {
> > tree t = copy_node (*expr_p);
> > TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
> > e, op1);
> > *expr_p = t;
> > }
> > rather than modification of the ARRAY_REF's operand in place. If we
> > did that, we wouldn't really care about the order, shared tree would
> > be instrumented once, with SAVE_EXPR in there making sure we don't
> > compute that multiple times. Is that because the 2 copies could
> > have side-effects and we do want to evaluate those multiple times?
>
> I'd assumed that that was the point of the copy_node. But now that
> I'm actually experimenting with it, I can't trigger any problems
> without the copy_node. So maybe we can use the following patch, which
> also adds a new test, bounds-21.c, to check that side-effects are
> evaluated correctly. I didn't bother writing a description for this
> patch yet because I sort of think we should apply both patches at the
> same time.
Perhaps it would be safer to apply for GCC 13 just your first patch
and maybe the testsuite coverage from this one and defer this change
for GCC 14?
> Regtested on x86_64-pc-linux-gnu.
>
> -- >8 --
> PR sanitizer/108060
> PR sanitizer/109050
>
> gcc/c-family/ChangeLog:
>
> * c-ubsan.cc (ubsan_maybe_instrument_array_ref): Don't copy_node.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/ubsan/bounds-17.c: New test.
> * c-c++-common/ubsan/bounds-18.c: New test.
> * c-c++-common/ubsan/bounds-19.c: New test.
> * c-c++-common/ubsan/bounds-20.c: New test.
> * c-c++-common/ubsan/bounds-21.c: New test.
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
2023-03-10 18:07 ` Jakub Jelinek
@ 2023-03-10 18:09 ` Marek Polacek
0 siblings, 0 replies; 6+ messages in thread
From: Marek Polacek @ 2023-03-10 18:09 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, GCC Patches
On Fri, Mar 10, 2023 at 07:07:36PM +0100, Jakub Jelinek wrote:
> On Thu, Mar 09, 2023 at 07:44:53PM -0500, Marek Polacek wrote:
> > On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote:
> > > On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote:
> > > > I think this is a reasonable way to address the regression, so OK.
> > >
> > > It is true that both C and C++ (including c++14_down and c++17 and later
> > > where the latter have different ordering rules) evaluate the lhs of
> > > MODIFY_EXPR after rhs, so conceptually this patch makes sense.
> >
> > Thank you both for taking a look.
> >
> > > But I wonder why we do in ubsan_maybe_instrument_array_ref:
> > > if (e != NULL_TREE)
> > > {
> > > tree t = copy_node (*expr_p);
> > > TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1),
> > > e, op1);
> > > *expr_p = t;
> > > }
> > > rather than modification of the ARRAY_REF's operand in place. If we
> > > did that, we wouldn't really care about the order, shared tree would
> > > be instrumented once, with SAVE_EXPR in there making sure we don't
> > > compute that multiple times. Is that because the 2 copies could
> > > have side-effects and we do want to evaluate those multiple times?
> >
> > I'd assumed that that was the point of the copy_node. But now that
> > I'm actually experimenting with it, I can't trigger any problems
> > without the copy_node. So maybe we can use the following patch, which
> > also adds a new test, bounds-21.c, to check that side-effects are
> > evaluated correctly. I didn't bother writing a description for this
> > patch yet because I sort of think we should apply both patches at the
> > same time.
>
> Perhaps it would be safer to apply for GCC 13 just your first patch
> and maybe the testsuite coverage from this one and defer this change
> for GCC 14?
That sounds good, I'll push the original patch with the new test now.
Thanks.
> > Regtested on x86_64-pc-linux-gnu.
> >
> > -- >8 --
> > PR sanitizer/108060
> > PR sanitizer/109050
> >
> > gcc/c-family/ChangeLog:
> >
> > * c-ubsan.cc (ubsan_maybe_instrument_array_ref): Don't copy_node.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * c-c++-common/ubsan/bounds-17.c: New test.
> > * c-c++-common/ubsan/bounds-18.c: New test.
> > * c-c++-common/ubsan/bounds-19.c: New test.
> > * c-c++-common/ubsan/bounds-20.c: New test.
> > * c-c++-common/ubsan/bounds-21.c: New test.
>
> Jakub
>
Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-10 18:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-08 21:09 [PATCH] ubsan: missed -fsanitize=bounds for compound ops [PR108060] Marek Polacek
2023-03-09 8:12 ` Richard Biener
2023-03-09 8:44 ` Jakub Jelinek
2023-03-10 0:44 ` [PATCH v2] " Marek Polacek
2023-03-10 18:07 ` Jakub Jelinek
2023-03-10 18:09 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).