On 05/12/2016 12:41 PM, Jakub Jelinek wrote:
> On Wed, May 11, 2016 at 02:54:01PM +0200, Martin Liška wrote:
>> On 05/06/2016 02:22 PM, Jakub Jelinek wrote:
>>> On Fri, May 06, 2016 at 01:04:30PM +0200, Martin Liška wrote:
>>>> I've started working on the patch couple of month go, basically after
>>>> a brief discussion with Jakub on IRC.
>>>>
>>>> I'm sending the initial version which can successfully run instrumented
>>>> tramp3d, postgresql server and Inkscape. It catches the basic set of
>>>> examples which are added in following patch.
>>>>
>>>> The implementation is quite straightforward as works in following steps:
>>>>
>>>> 1) Every local variable stack slot is poisoned at the very beginning of a function (RTL emission)
>>>> 2) In gimplifier, once we spot a DECL_EXPR, a variable is unpoisoned (by emitting ASAN_MARK builtin)
>>>> and the variable is marked as addressable
>>>
>>> Not all vars have DECL_EXPRs though.
> 
> Just random comments from quick skim, need to find enough spare time to
> actually try it and see how it works.
> 
>> Yeah, I've spotted one interesting example which is part of LLVM's testsuite:
>>
>> struct IntHolder {
>>   int val;
>> };
>>
>> const IntHolder *saved;
>>
>> void save(const IntHolder &holder) {
>>   saved = &holder;
>> }
>>
>> int main(int argc, char *argv[]) {
>>   save({10});
>>   int x = saved->val;  // BOOM
>>   return x;
>> }
>>
>> It would be also good to handle such temporaries. Any suggestions how to handle that in gimplifier?
> 
> Dunno, guess you need to do something in the FE for it already (talk to
> Jason?).  At least in *.original dump there is already:
>   <<cleanup_point <<< Unknown tree: expr_stmt
>   save ((const struct IntHolder &) &TARGET_EXPR <D.2263, {.val=10}>) >>>>>;
>     int x = (int) saved->val;
>   return <retval> = x;
> and the info on where the D.2263 temporary goes out of scope is lost.

Thanks for sample, I will ask Jason to help me with that.

> 
>> Apart from that, second version of the patch changes:
>> + fixed issues with missing stack unpoisoning; currently, I mark all VAR_DECLs that
>> are in ASAN_MARK internal fns and stack prologue/epilogue is emitted just for these vars
>> + removed unneeded hunks (tree-vect-patterns.c and asan_poisoning.cc)
>> + LABEL unpoisoning code makes stable sort for variables that were already used in the context
>> + stack poisoning hasn't worked for -O1+ due to following guard in asan.c
>>  /* Automatic vars in the current function will be always accessible.  */
>> + direct shadow memory poisoning/unpoisoning code is introduced - in both scenarios (RTL and GIMPLE),
>> I would appreciate feedback if storing multiple bytes is fine? What is the maximum memory wide
>> store mode supported by a target? How can I get such information?
>> + the maximum object size handled by a direct emission is guarded by use-after-scope-direct-emission-threshold
>> parameter; initial value (256B) should maximally emit store of 32B
> 
> Would be better if user visible param was in bytes rather than bits IMHO.
> 

Well, the size of an object is in bytes, but as we map every 8 (yeah, that's configurable, I'm quite curious about
real respecting of ASAN_SHADOW_SHIFT) bytes of real memory to
a single byte in shadow memory, thus the division by 8 is needed.

>> Yeah, depends because of:
>>
>> static inline bool
>> asan_sanitize_use_after_scope (void)
>> {
>>   return ((flag_sanitize & SANITIZE_ADDRESS_USE_AFTER_SCOPE)
>> 	  == SANITIZE_ADDRESS_USE_AFTER_SCOPE
>> 	  && flag_stack_reuse == SR_NONE
>> 	  && ASAN_STACK);
>> }
>>
>> Where ASAN_STACK comes from params.h.
> 
> I'd prefer just prototype the function in the header and define in asan.c
> or some other source file.  Or maybe split it, do the important case
> (flag_sanitize check) inline and call out of line function for the rest.
> Why do you check flag_stack_reuse?  I thought you'd arrange for it to be
> different when -fsanitize=use-after-scope?

Right, the sanitization does not relate to flag_stack, thus removing the dependency,
we can remove need of including params.h in various places.

> 
>> @@ -243,6 +243,11 @@ static unsigned HOST_WIDE_INT asan_shadow_offset_value;
>>  static bool asan_shadow_offset_computed;
>>  static vec<char *> sanitized_sections;
>>  
>> +/* Set of variable declarations that are going to be guarded by
>> +   use-after-scope sanitizer.  */
>> +
>> +static hash_set <tree> asan_handled_variables(13);
> 
> Not sure about the formatting here, don't we use xxx<arg> instead of xxx <arg>
> ?  And I'd expect space before (.

Yeah, done.

>> @@ -1020,6 +1020,91 @@ asan_function_start (void)
>>  			 current_function_funcdef_no);
>>  }
>>  
>> +/* Return number of shadow bytes that are occupied by a local variable
>> +   of SIZE bytes.  */
>> +
>> +static unsigned HOST_WIDE_INT
>> +get_shadow_memory_size (unsigned HOST_WIDE_INT size)
>> +{
>> +  /* Round up size of object.  */
>> +  unsigned HOST_WIDE_INT r;
>> +  if ((r = size % BITS_PER_UNIT) != 0)
>> +    size += BITS_PER_UNIT - r;
> 
> Isn't there a ROUND_UP macro?

Thanks. I was looking for the macro.

I'm sending v3 where all of aforementioned notes were handled.

Thanks,
Martin

> 
> 	Jakub
>