diff mbox

[asan] Patch - fix an ICE in asan.c

Message ID 509E53E2.5080908@net-b.de
State New
Headers show

Commit Message

Tobias Burnus Nov. 10, 2012, 1:17 p.m. UTC 
Jakub Jelinek wrote:
>> --- gcc/asan.c.orig	2012-11-09 21:26:26.000000000 +0100
>> +++ gcc/asan.c	2012-11-09 21:26:00.000000000 +0100
>> @@ -1362,6 +1362,8 @@ transform_statements (void)
>>   	    instrument_assignment (&i);
>>   	  else if (is_gimple_call (s))
>>   	    maybe_instrument_call (&i);
>> +	  if (gsi_end_p (i))
>> +	    break;
>>           }
>>       }
>>   }
>
> That looks a wrong place for this.

I already expected that it was not fully correct ;-)


> So untested:

Thanks for the patch! It fixed the problem half way: It fixes the second 
issue I had (fail10.ii, 
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00791.html ).

However, it didn't fix the original problem: As the call for strlen 
directly returns, it never reaches your patch. Hence, it doesn't fix 
fail31.ii of http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00786.html

If one uses the same line for strlen, it works.

Updated patches attached – one is on top of the trunk + Dodji's patches, 
the other is on top of the asan branch.

  * * *

The question is whether one also needs to do something for the atomics 
handling in maybe_instrument_builtin_call, which has:
         instrument_derefs (iter, dest, loc, is_store);
         return;

The instrument_derefs calls - in some cases - build_check_stmt, which in 
turn calls:
   *iter = gsi_start_bb (else_bb)

Tobias

Comments

Tobias Burnus Nov. 10, 2012, 3:18 p.m. UTC | #1 
Tobias Burnus wrote:
>> So untested:
>
> Thanks for the patch! It fixed the problem half way: It fixes the 
> second issue I had (fail10.ii, 
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00791.html ).
>
> However, it didn't fix the original problem: As the call for strlen 
> directly returns, it never reaches your patch. Hence, it doesn't fix 
> fail31.ii of http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00786.html
>
> If one uses the same line for strlen, it works.

I spoke too early. With the updated patch, there is no ICE, but one 
crashes for the following valid program with:

==27313== ERROR: AddressSanitizer crashed on unknown address 
0x13fffe8fc737 (pc 0x0000004008e3 sp 0x7fffa3f1c6d0 bp 0x7fffa3f1c700 T0)
AddressSanitizer can not provide additional info.


The crucial part is the "strlen" call.


#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[])
{
  int i;
  for (i = 0; i < argc; i++)
    {
      printf("%d: '%s':%zd\n", i, argv[i], strlen(argv[i]));
    }
  return 0;
}


Tobias
diff mbox

Patch

(This patch is for the "asan" branch.)

2012-11-10  Jakub Jelinek  <jakub@redhat.com>
	    Tobias Burnus  <burnus@net-b.de>

	* asan.c (maybe_instrument_builtin_call): Set *iter
	to gsi for the call at the end.

diff --git a/gcc/asan.c b/gcc/asan.c
index 155e84b..3297b52 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -952,6 +952,7 @@  maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
 
     case BUILT_IN_STRLEN:
       instrument_strlen_call (iter);
+      *iter = gsi_for_stmt (call);
       return true;
 
     /* And now the __atomic* and __sync builtins.
@@ -1191,6 +1192,7 @@  maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
+      *iter = gsi_for_stmt (call);
       return true;
     }
   return false;