Re: [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer.

[Maxim Ostapenko <m.ostapenko@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 3913 bytes --]

On 08/04/16 12:12, Jakub Jelinek wrote:
> On Fri, Apr 08, 2016 at 11:52:32AM +0300, Maxim Ostapenko wrote:
>> 2016-04-08  Maxim Ostapenko  <m.ostapenko@samsung.com>
>>
>> 	PR sanitizer/70541
>> 	* asan.c (instrument_derefs): If we get unknown location, extract it
>> 	with EXPR_LOCATION.
>> 	(maybe_instrument_call): Instrument gimple_call's arguments if needed.
>> 	* tree-inline.c (initialize_inlined_parameters): Set location for
>> 	inlined parameters initialization statements.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-04-08  Maxim Ostapenko  <m.ostapenko@samsung.com>
>>
>> 	PR sanitizer/70541
>> 	* c-c++-common/asan/pr70541-1.c: New test.
>> 	* c-c++-common/pr70541-2.c: Likewise.
>> +  /* Walk through gimple_call arguments and check them id needed.  */
>> +  unsigned args_num = gimple_call_num_args (stmt);
>> +  for (unsigned i = 0; i < args_num; ++i)
>> +    {
>> +      tree arg = gimple_call_arg (stmt, i);
>> +      gcc_assert (TREE_CODE (arg) != CALL_EXPR);
> Please remove this assert, of course in GIMPLE arguments can't be
> CALL_EXPRs, they must satisfy is_gimple_val or is_gimple_lvalue, but
> that is verified in verify_gimple_call.
>
>> +      /* If ARG is not a non-aggregate register variable, compiler in general
>> +	 creates temporary for it and pass it as argument to gimple call.
>> +	 But in some cases, e.g. when we pass by value a small structure that
>> +	 fits to register, compiler can avoid extra overhead by pulling out
>> +	 these temporaries.  In this case, we should check the argument.  */
>> +      if (!is_gimple_reg (arg))
> Actually, I believe this should be
>        if (!is_gimple_reg (arg) && !is_gimple_min_invariant (arg))
> because constants, or addresses etc. also aren't memory references.
> I know instrument_derefs exits early if the t isn't one of the selected
> few trees, but still, it looks unclean to call instrument_derefs on
> constants or addresses.
>
> The asan.c changes look good to me with those changes, feel free to commit
> them separately from the tree-inline.c change (perhaps with something in the
> testcase XFAILed for now).

Ok, thanks, landed ASan part as r234827. I'm not sure how to XFAIL 
output pattern tests only, so I enabled testcase only with -O0 for now.

>
>> --- a/gcc/tree-inline.c
>> +++ b/gcc/tree-inline.c
>> @@ -3247,7 +3247,8 @@ initialize_inlined_parameters (copy_body_data *id, gimple *stmt,
>>       {
>>         tree val;
>>         val = i < gimple_call_num_args (stmt) ? gimple_call_arg (stmt, i) : NULL;
>> -      setup_one_parameter (id, p, val, fn, bb, &vars);
>> +      if (gimple *new_stmt = setup_one_parameter (id, p, val, fn, bb, &vars))
>> +	gimple_set_location (new_stmt, gimple_location (stmt));
>>       }
>>     /* After remapping parameters remap their types.  This has to be done
>>        in a second loop over all parameters to appropriately remap
>> @@ -3285,7 +3286,9 @@ initialize_inlined_parameters (copy_body_data *id, gimple *stmt,
>>         /* No static chain?  Seems like a bug in tree-nested.c.  */
>>         gcc_assert (static_chain);
>>   
>> -      setup_one_parameter (id, p, static_chain, fn, bb, &vars);
>> +      if (gimple *new_stmt = setup_one_parameter (id, p, static_chain, fn, bb,
>> +						  &vars))
>> +	gimple_set_location (new_stmt, gimple_location (stmt));
>>       }
>>   
>>     declare_inline_vars (id->block, vars);
> The tree-inline.c change looks much more risky at this point, could we defer
> that for GCC 7?    Unlike the asan change this is going to affect debugging, etc.
> Maybe e.g. pass gimple_location (stmt) as argument to setup_one_parameter
> and set it in there instead?  Sometimes there are multiple statements
> created in there, not just one, etc.  In any case, I'd prefer second pair of
> eyes on the tree-inline.c changes, so I'm CCing Richi and Honza.

Perhaps we should move this issue to separate bug in Bugzilla?

>
> 	Jakub
>
>

-Maxim

[-- Attachment #2: pr70541-2.diff --]
[-- Type: text/x-diff, Size: 3899 bytes --]

gcc/ChangeLog:

2016-04-08  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/70541
	* asan.c (instrument_derefs): If we get unknown location, extract it
	with EXPR_LOCATION.
	(maybe_instrument_call): Instrument gimple_call's arguments if needed.

gcc/testsuite/ChangeLog:

2016-04-08  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/70541
	* c-c++-common/asan/pr70541.c: New test.

diff --git a/gcc/asan.c b/gcc/asan.c
index 47bfdcd..71095fb 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,30 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
 			 gimple_location (stmt),
 			 /*is_store=*/true);
 
-      gsi_next (iter);
-      return true;
+      instrumented = true;
     }
 
-  return false;
+  /* Walk through gimple_call arguments and check them id needed.  */
+  unsigned args_num = gimple_call_num_args (stmt);
+  for (unsigned i = 0; i < args_num; ++i)
+    {
+      tree arg = gimple_call_arg (stmt, i);
+      /* If ARG is not a non-aggregate register variable, compiler in general
+	 creates temporary for it and pass it as argument to gimple call.
+	 But in some cases, e.g. when we pass by value a small structure that
+	 fits to register, compiler can avoid extra overhead by pulling out
+	 these temporaries.  In this case, we should check the argument.  */
+      if (!is_gimple_reg (arg) && !is_gimple_min_invariant (arg))
+	{
+	  instrument_derefs (iter, arg,
+			     gimple_location (stmt),
+			     /*is_store=*/false);
+	  instrumented = true;
+	}
+    }
+  if (instrumented)
+    gsi_next (iter);
+  return instrumented;
 }
 
 /* Walk each instruction of all basic block and instrument those that
diff --git a/gcc/testsuite/c-c++-common/asan/pr70541.c b/gcc/testsuite/c-c++-common/asan/pr70541.c
new file mode 100644
index 0000000..b2a4bd5
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr70541.c
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-fno-builtin-malloc -fno-builtin-free" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+/* { dg-shouldfail "asan" } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+struct Simple {
+  int value;
+};
+
+int f(struct Simple simple) {
+  return simple.value;
+}
+
+int main() {
+  struct Simple *psimple = (struct Simple *) malloc(sizeof(struct Simple));
+  psimple->value = 42;
+  free(psimple);
+  printf("%d\n", f(*psimple));
+  return 0;
+}
+
+/* { dg-output "ERROR: AddressSanitizer:? heap-use-after-free on address\[^\n\r]*" } */
+/* { dg-output "0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "    #0 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*pr70541.c:21|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*freed by thread T0 here:\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "    #0 0x\[0-9a-f\]+ +(in _*(interceptor_|wrap_|)free|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "    #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*pr70541.c:20|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*previously allocated by thread T0 here:\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "    #0 0x\[0-9a-f\]+ +(in _*(interceptor_|wrap_|)malloc|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "    #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*pr70541.c:18|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */