| Message ID | 20170517141023.GH24582@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 17, 2017 at 4:10 PM, Marek Polacek <polacek@redhat.com> wrote: > We are failing to detect accessing a null pointer in &s->i because > v_3 = &s_2->i; > is not gimple_assign_load_p: > 1997 if (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT)) > 1998 { > 1999 if (gimple_store_p (stmt)) > 2000 instrument_null (gsi, true); > 2001 if (gimple_assign_load_p (stmt)) > 2002 instrument_null (gsi, false); > 2003 } > > So I think we can use gimple_assign_single_p instead of gimple_assign_load_p > and then strip the ADDR_EXPR in instrument_null before getting its base > address. (I've seen stripping ADDR_EXPR before get_base_address elsewhere in > the codebase, too.) > > Bootstrapped-ubsan/regtested on x86_64-linux, ok for trunk? The ADDR_EXPR check covers cases this cannot happen and generally not all single rhs are memory or addresses. But yes, it should work. Thus ok. Note that we miss to instrument all aggregate arguments to calls and asms as well. Richard. > 2017-05-17 Marek Polacek <polacek@redhat.com> > > PR sanitizer/80797 > * ubsan.c (instrument_null): Unwrap ADDR_EXPRs. > (pass_ubsan::execute): Call gimple_assign_single_p instead of > gimple_assign_load_p. > > * c-c++-common/ubsan/null-12.c: New test. > > diff --git gcc/testsuite/c-c++-common/ubsan/null-12.c gcc/testsuite/c-c++-common/ubsan/null-12.c > index e69de29..3478dc1 100644 > --- gcc/testsuite/c-c++-common/ubsan/null-12.c > +++ gcc/testsuite/c-c++-common/ubsan/null-12.c > @@ -0,0 +1,42 @@ > +/* PR sanitizer/80797 */ > +/* { dg-do run } */ > +/* { dg-options "-fsanitize=undefined" } */ > + > +struct S > +{ > + int i; > +}; > + > +struct R > +{ > + struct T { > + int i; > + } *t; > +} r; > + > +int > +main () > +{ > + struct S *s = 0; > + struct S *s2[1] = { }; > + > + int *v1 = &s->i; > + int *v2 = &(*s).i; > + int *v3 = &s2[0]->i; > + int *v4 = &s->i + 1; > + int *v5 = &r.t->i; > + > + asm ("" : : "r" (&v1) : "memory"); > + asm ("" : : "r" (&v2) : "memory"); > + asm ("" : : "r" (&v3) : "memory"); > + asm ("" : : "r" (&v4) : "memory"); > + asm ("" : : "r" (&v5) : "memory"); > + > + return 0; > +} > + > +/* { dg-output "member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct T'" } */ > diff --git gcc/ubsan.c gcc/ubsan.c > index 4159cc5..a4808d2 100644 > --- gcc/ubsan.c > +++ gcc/ubsan.c > @@ -1208,6 +1208,9 @@ instrument_null (gimple_stmt_iterator gsi, bool is_lhs) > { > gimple *stmt = gsi_stmt (gsi); > tree t = is_lhs ? gimple_get_lhs (stmt) : gimple_assign_rhs1 (stmt); > + /* Handle also e.g. &s->i. */ > + if (TREE_CODE (t) == ADDR_EXPR) > + t = TREE_OPERAND (t, 0); > tree base = get_base_address (t); > const enum tree_code code = TREE_CODE (base); > if (code == MEM_REF > @@ -1998,7 +2001,7 @@ pass_ubsan::execute (function *fun) > { > if (gimple_store_p (stmt)) > instrument_null (gsi, true); > - if (gimple_assign_load_p (stmt)) > + if (gimple_assign_single_p (stmt)) > instrument_null (gsi, false); > } > > > Marek
On Thu, May 18, 2017 at 09:03:31AM +0200, Richard Biener wrote: > On Wed, May 17, 2017 at 4:10 PM, Marek Polacek <polacek@redhat.com> wrote: > > We are failing to detect accessing a null pointer in &s->i because > > v_3 = &s_2->i; > > is not gimple_assign_load_p: > > 1997 if (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT)) > > 1998 { > > 1999 if (gimple_store_p (stmt)) > > 2000 instrument_null (gsi, true); > > 2001 if (gimple_assign_load_p (stmt)) > > 2002 instrument_null (gsi, false); > > 2003 } > > > > So I think we can use gimple_assign_single_p instead of gimple_assign_load_p > > and then strip the ADDR_EXPR in instrument_null before getting its base > > address. (I've seen stripping ADDR_EXPR before get_base_address elsewhere in > > the codebase, too.) > > > > Bootstrapped-ubsan/regtested on x86_64-linux, ok for trunk? > > The ADDR_EXPR check covers cases this cannot happen and generally not > all single rhs are memory or addresses. But yes, it should work. Right, I think that's why we check that the result of get_base_address is a MEM_REF in instrument_null. > Thus ok. Thanks. > Note that we miss to instrument all aggregate arguments to calls and > asms as well. :/ > Richard. > > > 2017-05-17 Marek Polacek <polacek@redhat.com> > > > > PR sanitizer/80797 > > * ubsan.c (instrument_null): Unwrap ADDR_EXPRs. > > (pass_ubsan::execute): Call gimple_assign_single_p instead of > > gimple_assign_load_p. > > > > * c-c++-common/ubsan/null-12.c: New test. > > > > diff --git gcc/testsuite/c-c++-common/ubsan/null-12.c gcc/testsuite/c-c++-common/ubsan/null-12.c > > index e69de29..3478dc1 100644 > > --- gcc/testsuite/c-c++-common/ubsan/null-12.c > > +++ gcc/testsuite/c-c++-common/ubsan/null-12.c > > @@ -0,0 +1,42 @@ > > +/* PR sanitizer/80797 */ > > +/* { dg-do run } */ > > +/* { dg-options "-fsanitize=undefined" } */ > > + > > +struct S > > +{ > > + int i; > > +}; > > + > > +struct R > > +{ > > + struct T { > > + int i; > > + } *t; > > +} r; > > + > > +int > > +main () > > +{ > > + struct S *s = 0; > > + struct S *s2[1] = { }; > > + > > + int *v1 = &s->i; > > + int *v2 = &(*s).i; > > + int *v3 = &s2[0]->i; > > + int *v4 = &s->i + 1; > > + int *v5 = &r.t->i; > > + > > + asm ("" : : "r" (&v1) : "memory"); > > + asm ("" : : "r" (&v2) : "memory"); > > + asm ("" : : "r" (&v3) : "memory"); > > + asm ("" : : "r" (&v4) : "memory"); > > + asm ("" : : "r" (&v5) : "memory"); > > + > > + return 0; > > +} > > + > > +/* { dg-output "member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ > > +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct T'" } */ > > diff --git gcc/ubsan.c gcc/ubsan.c > > index 4159cc5..a4808d2 100644 > > --- gcc/ubsan.c > > +++ gcc/ubsan.c > > @@ -1208,6 +1208,9 @@ instrument_null (gimple_stmt_iterator gsi, bool is_lhs) > > { > > gimple *stmt = gsi_stmt (gsi); > > tree t = is_lhs ? gimple_get_lhs (stmt) : gimple_assign_rhs1 (stmt); > > + /* Handle also e.g. &s->i. */ > > + if (TREE_CODE (t) == ADDR_EXPR) > > + t = TREE_OPERAND (t, 0); > > tree base = get_base_address (t); > > const enum tree_code code = TREE_CODE (base); > > if (code == MEM_REF > > @@ -1998,7 +2001,7 @@ pass_ubsan::execute (function *fun) > > { > > if (gimple_store_p (stmt)) > > instrument_null (gsi, true); > > - if (gimple_assign_load_p (stmt)) > > + if (gimple_assign_single_p (stmt)) > > instrument_null (gsi, false); > > } Marek
diff --git gcc/testsuite/c-c++-common/ubsan/null-12.c gcc/testsuite/c-c++-common/ubsan/null-12.c index e69de29..3478dc1 100644 --- gcc/testsuite/c-c++-common/ubsan/null-12.c +++ gcc/testsuite/c-c++-common/ubsan/null-12.c @@ -0,0 +1,42 @@ +/* PR sanitizer/80797 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=undefined" } */ + +struct S +{ + int i; +}; + +struct R +{ + struct T { + int i; + } *t; +} r; + +int +main () +{ + struct S *s = 0; + struct S *s2[1] = { }; + + int *v1 = &s->i; + int *v2 = &(*s).i; + int *v3 = &s2[0]->i; + int *v4 = &s->i + 1; + int *v5 = &r.t->i; + + asm ("" : : "r" (&v1) : "memory"); + asm ("" : : "r" (&v2) : "memory"); + asm ("" : : "r" (&v3) : "memory"); + asm ("" : : "r" (&v4) : "memory"); + asm ("" : : "r" (&v5) : "memory"); + + return 0; +} + +/* { dg-output "member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct S'\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*member access within null pointer of type 'struct T'" } */ diff --git gcc/ubsan.c gcc/ubsan.c index 4159cc5..a4808d2 100644 --- gcc/ubsan.c +++ gcc/ubsan.c @@ -1208,6 +1208,9 @@ instrument_null (gimple_stmt_iterator gsi, bool is_lhs) { gimple *stmt = gsi_stmt (gsi); tree t = is_lhs ? gimple_get_lhs (stmt) : gimple_assign_rhs1 (stmt); + /* Handle also e.g. &s->i. */ + if (TREE_CODE (t) == ADDR_EXPR) + t = TREE_OPERAND (t, 0); tree base = get_base_address (t); const enum tree_code code = TREE_CODE (base); if (code == MEM_REF @@ -1998,7 +2001,7 @@ pass_ubsan::execute (function *fun) { if (gimple_store_p (stmt)) instrument_null (gsi, true); - if (gimple_assign_load_p (stmt)) + if (gimple_assign_single_p (stmt)) instrument_null (gsi, false); }
We are failing to detect accessing a null pointer in &s->i because v_3 = &s_2->i; is not gimple_assign_load_p: 1997 if (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT)) 1998 { 1999 if (gimple_store_p (stmt)) 2000 instrument_null (gsi, true); 2001 if (gimple_assign_load_p (stmt)) 2002 instrument_null (gsi, false); 2003 } So I think we can use gimple_assign_single_p instead of gimple_assign_load_p and then strip the ADDR_EXPR in instrument_null before getting its base address. (I've seen stripping ADDR_EXPR before get_base_address elsewhere in the codebase, too.) Bootstrapped-ubsan/regtested on x86_64-linux, ok for trunk? 2017-05-17 Marek Polacek <polacek@redhat.com> PR sanitizer/80797 * ubsan.c (instrument_null): Unwrap ADDR_EXPRs. (pass_ubsan::execute): Call gimple_assign_single_p instead of gimple_assign_load_p. * c-c++-common/ubsan/null-12.c: New test. Marek