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.
Thus ok.
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
