| Message ID | 20141119135111.GB29446@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 19, 2014 at 02:51:11PM +0100, Marek Polacek wrote: > As the testcase shows, we should really check first that we > have a MEM_REF, otherwise subsequent TREE_OPERAND might ICE. > On an invalid testcase we might get e.g. STRING_CST. Well, addr_object_size should handle STRING_CSTs just fine, so why not handle STRING_CSTs in there too (similarly to decl_p = true case). Supposedly you want decl_p = true for them and need to tweak if (!POINTER_TYPE_P (TREE_TYPE (base)) && !DECL_P (base)) but otherwise it should work. On the other side, int foo (int x) { return "foobar"[x]; } is already handled by -fsanitize=bounds and for some reason gimple_assign_load_p doesn't consider handled components with STRING_CST as base as loads so for -fsanitize=object-size we aren't called for that. Jakub
On Wed, Nov 19, 2014 at 03:12:05PM +0100, Jakub Jelinek wrote: > On Wed, Nov 19, 2014 at 02:51:11PM +0100, Marek Polacek wrote: > > As the testcase shows, we should really check first that we > > have a MEM_REF, otherwise subsequent TREE_OPERAND might ICE. > > On an invalid testcase we might get e.g. STRING_CST. > > Well, addr_object_size should handle STRING_CSTs just fine, so > why not handle STRING_CSTs in there too (similarly to decl_p = true > case). > Supposedly you want decl_p = true for them and need to tweak > if (!POINTER_TYPE_P (TREE_TYPE (base)) && !DECL_P (base)) > but otherwise it should work. > On the other side, > int > foo (int x) > { > return "foobar"[x]; > } > is already handled by -fsanitize=bounds and for some reason > gimple_assign_load_p doesn't consider handled components with > STRING_CST as base as loads so for -fsanitize=object-size > we aren't called for that. The patch is ok as is. Jakub
On Wed, Nov 19, 2014 at 03:12:05PM +0100, Jakub Jelinek wrote: > On Wed, Nov 19, 2014 at 02:51:11PM +0100, Marek Polacek wrote: > > As the testcase shows, we should really check first that we > > have a MEM_REF, otherwise subsequent TREE_OPERAND might ICE. > > On an invalid testcase we might get e.g. STRING_CST. > > Well, addr_object_size should handle STRING_CSTs just fine, so > why not handle STRING_CSTs in there too (similarly to decl_p = true > case). > Supposedly you want decl_p = true for them and need to tweak > if (!POINTER_TYPE_P (TREE_TYPE (base)) && !DECL_P (base)) > but otherwise it should work. > On the other side, > int > foo (int x) > { > return "foobar"[x]; > } > is already handled by -fsanitize=bounds and for some reason Right - I've checked that this is the case. I could mention that in the original submission. > gimple_assign_load_p doesn't consider handled components with > STRING_CST as base as loads so for -fsanitize=object-size > we aren't called for that. Thanks for review, Marek
diff --git gcc/testsuite/gcc.dg/ubsan/pr63690.c gcc/testsuite/gcc.dg/ubsan/pr63690.c index e69de29..bcf520a 100644 --- gcc/testsuite/gcc.dg/ubsan/pr63690.c +++ gcc/testsuite/gcc.dg/ubsan/pr63690.c @@ -0,0 +1,9 @@ +/* PR sanitizer/63690 */ +/* { dg-do compile } */ +/* { dg-options "-fsanitize=undefined" } */ + +void +foo (void) +{ + (*"c")++; +} diff --git gcc/ubsan.c gcc/ubsan.c index 7d1e341..ad5665f 100644 --- gcc/ubsan.c +++ gcc/ubsan.c @@ -1539,7 +1539,13 @@ instrument_object_size (gimple_stmt_iterator *gsi, bool is_lhs) return; bool decl_p = DECL_P (inner); - tree base = decl_p ? inner : TREE_OPERAND (inner, 0); + tree base; + if (decl_p) + base = inner; + else if (TREE_CODE (inner) == MEM_REF) + base = TREE_OPERAND (inner, 0); + else + return; tree ptr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t); while (TREE_CODE (base) == SSA_NAME)