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