[Bug sanitizer/70541] unnoticed invalid dereference when using address sanitizer

[m.ostapenko at samsung dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70541

--- Comment #4 from Maxim Ostapenko <m.ostapenko at samsung dot com> ---
(In reply to Jakub Jelinek from comment #3)
> (In reply to Maxim Ostapenko from comment #1)
> > @@ -2060,7 +2067,20 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
> >        return true;
> >      }
> 
> If the function call returns a struct, then your patch wouldn't instrument
> it.
> You need the bool instrumented = false; already above
>   if (gimple_store_p (stmt))
> and set instrumented = true; there instead of gsi_next (iter); return true;
> 
> > -  return false;
> > +  bool instrumented = false;
> > +  HOST_WIDE_INT args_num = gimple_call_num_args (stmt);
> > +  for (int i = 0; i < args_num; ++i)
> > +    {
> > +      if (is_arg_deref_p (TREE_CODE (gimple_call_arg (stmt, i))))
> 
> I'm not aware of any is_arg_deref_p predicate.
> IMHO you should test:
>       if (!is_gimple_reg (gimple_call_arg (stmt, i)))
> 
> > +       {
> > +         instrument_derefs (iter, gimple_call_arg (stmt, i),
> > +                            gimple_location (stmt), false);
> > +         instrumented = true;
> > +       }
> > +    }
> > +  if (instrumented)
> > +    gsi_next (iter);
> > +  return instrumented;
> 

Thanks, looks better now?

diff --git a/gcc/asan.c b/gcc/asan.c
index 47bfdcd..c51e629 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1766,6 +1766,8 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t,

   tree type, base;
   HOST_WIDE_INT size_in_bytes;
+  if (location == UNKNOWN_LOCATION)
+    location = EXPR_LOCATION (t);

   type = TREE_TYPE (t);
   switch (TREE_CODE (t))
@@ -2049,6 +2051,7 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
       gsi_insert_before (iter, g, GSI_SAME_STMT);
     }

+  bool instrumented = false;
   if (gimple_store_p (stmt))
     {
       tree ref_expr = gimple_call_lhs (stmt);
@@ -2056,11 +2059,22 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
                         gimple_location (stmt),
                         /*is_store=*/true);

-      gsi_next (iter);
-      return true;
+      instrumented = true;
     }

-  return false;
+  HOST_WIDE_INT args_num = gimple_call_num_args (stmt);
+  for (int i = 0; i < args_num; ++i)
+    {
+      if (!is_gimple_reg (gimple_call_arg (stmt, i)))
+       {
+         instrument_derefs (iter, gimple_call_arg (stmt, i),
+                            gimple_location (stmt), false);
+         instrumented = true;
+       }
+    }
+  if (instrumented)
+    gsi_next (iter);
+  return instrumented;
 }

> As for the location_t thing, the fix would be to do in instrument_derefs
> something like:
>   if (location == UNKNOWN_LOCATION)
>     location = EXPR_LOCATION (t);
> after the early bail outs.

Hm, even with if (location == UNKNOWN_LOCATION) location = EXPR_LOCATION (t); I
don't see reasonable line in -O2 case:

  1 #include <stdio.h>
  2 #include <stdlib.h>
  3 
  4 struct Simple {
  5   int value;
  6 };
  7 
  8 int f(struct Simple simple) {
  9   return simple.value;
 10 }
 11 
 12 int g(int value) {
 13   return value;
 14 }
 15 
 16 int main() {
 17   struct Simple *psimple = (struct Simple *) malloc(sizeof(struct Simple));
 18   psimple->value = 42;
 19   free(psimple);
 20   printf("%d\n", f(*psimple));
 21   return 0;
 22 }

$ ./a.out

==29898==ERROR: AddressSanitizer: heap-use-after-free on address 0x60200000eff0
at pc 0x0000004055a6 bp 0x7ffc6b5632c0 sp 0x7ffc6b5632a0
READ of size 4 at 0x60200000eff0 thread T0
    #0 0x4055a5 in main /tmp/test2.c:22
    #1 0x7f8e6df32ec4 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #2 0x4055f9  (/tmp/a.out+0x4055f9)


$ cat test2.c.126t.asan1

main ()
{
  int simple$value;
  struct Simple * psimple;

  <bb 2>:
  [test2.c:17:18] psimple_3 = malloc (4);
  [test2.c:17:18] # DEBUG psimple => psimple_3
  [test2.c:19:3] free (psimple_3);
  ASAN_CHECK (6, psimple_3, 4, 8);
  simple$value_6 = MEM[(struct Simple *)psimple_3];
  # DEBUG simple$value => simple$value_6
  [test2.c:20:3] printf ([test2.c:20:10] "%d\n", simple$value_6);
  return 0;
}

Perhaps we indeed should look at the inliner.