[v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
Commit Message
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
base-commit: f366fdfeec0af6cda716de913c32e48f9b1e3a0e
Comments
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
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
@@ -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);
}
}
new file mode 100644
@@ -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\\\]'" } */
new file mode 100644
@@ -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\\\]'" } */
new file mode 100644
@@ -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\\\]'" } */
new file mode 100644
@@ -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\\\]'" } */
new file mode 100644
@@ -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 ();
+}