Re: [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
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 +, 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
Re: [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
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 +, 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
[PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060]
On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote: > On Thu, Mar 09, 2023 at 08:12:47AM +, 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, , 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 000..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 000..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 000..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