On Thu, May 18, 2017 at 09:03:31AM +0200, Richard Biener wrote:
> On Wed, May 17, 2017 at 4:10 PM, Marek Polacek <pola...@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 <pola...@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
