[RFC] Introduce -fsanitize=use-after-scope

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.

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?

> 
>> 3) Similarly, BIND_EXPR is the place where we poison the variable (scope exit)
>> 4) At the very end of the function, we clean up the poisoned memory
>> 5) The builtins are expanded to call to libsanitizer run-time library (__asan_poison_stack_memory, __asan_unpoison_stack_memory)
>> 6) As the use-after-scope stuff is already included in libsanitizer, no change is needed for the library
> 
>> As mentioned, it's request for comment as it still has couple of limitations:
>> a) VLA are not supported, which should make sense as we are unable to allocate a stack slot for that
>> b) we can possibly strip some instrumentation in situations where a variable is introduced in a very first BB (RTL poisoning is superfluous).
>> Similarly for a very last BB of a function, we can strip end of scope poisoning (and RTL unpoisoning). I'll do that incrementally.
> 
> Yeah.
> 
>> c) We require -fstack-reuse=none option, maybe it worth to warn a user if -fsanitize=use-after-scope is provided without the option?
> 
> This should be implicitly set by -fsanitize=use-after-scope.  Only if some
> other -fstack-reuse= option is explicitly set together with
> -fsanitize=use-after-scope, we should warn and reset it anyway.

Handled in v2 of the patch.

> 
>> d) An instrumented binary is quite slow (~20x for tramp3d) as every function call produces many memory read/writes. I'm wondering whether
>> we should provide a faster alternative (like instrument just variables that have address taken) ?
> 
> I don't see any point in instrumenting !needs_to_live_in_memory vars,
> at least not those that don't need to live in memory at gimplification time.
> How could one use those after scope?  They can't be accessed by
> dereferencing some pointer, and the symbol itself should be unavailable for
> symbol lookup after the symbol goes out of scope.
> Plus obviously ~20x slowdown isn't acceptable.
> 
> Another thing is what to do with variables that are addressable at
> gimplification time, but generally are made non-addressable afterwards,
> such as due to optimizing away the taking of their address, inlining, etc.
> 
> Perhaps depending on how big slowdown you get after just instrumenting
> needs_to_live_in_memory vars from ~ gimplification time and/or with the
> possible inlining of the poisoning/unpoisoning (again, should be another
> knob), at least with small sized vars, there might be another knob,
> which would tell if vars that are made non-addressable during optimizations
> later on should be instrumented or not.
> E.g. if you ASAN_MARK all needs_to_live_in_memory vars early, you could
> during the addressable determination when the knob says stuff should be
> faster, but less precise, ignore the vars that are addressable just because
> of the ASAN_MARK calls, and if they'd then turn to be non-addressable,
> remove the corresponding ASAN_MARK calls.

Following the aforementioned instrumentation and utilizing direct shadow memory
instruction emission, I was able to reduce tramp3d slowdown to 3x and
postgresql server test-suite runs 2x slower.

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

> 
>> 2016-05-04  Martin Liska  <mliska@suse.cz>
>>
>> 	* asan/asan_poisoning.cc: Do not call PoisonShadow in case
>> 	of zero size of aligned size.
> 
> Generally, libsanitizer changes would need to go through upstream.

Sure, in fact the hunk looks unneeded.

> 
>> --- a/gcc/asan.c
>> +++ b/gcc/asan.c
>> @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "varasm.h"
>>  #include "stor-layout.h"
>>  #include "tree-iterator.h"
>> +#include "params.h"
>>  #include "asan.h"
>>  #include "dojump.h"
>>  #include "explow.h"
>> @@ -54,7 +55,6 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "cfgloop.h"
>>  #include "gimple-builder.h"
>>  #include "ubsan.h"
>> -#include "params.h"
>>  #include "builtins.h"
>>  #include "fnmatch.h"
> 
> Why do you need to move params.h around?  Does asan.h now depend on
> params.h?

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.

> 
>> +  gimplify_seq_add_stmt
>> +    (seq_p, gimple_build_call_internal (IFN_ASAN_MARK, 3,
>> +					build_int_cst (integer_type_node,
>> +						       flags),
>> +					base, unit_size));
> 
> Formatting, better introduce some temporary variables, like
>   gimple *g = gimple_build_call_internal (...);
>   gimplify_seq_add_stmt (seq_p, g);
> 

Done.

>> --- a/gcc/tree-vect-patterns.c
>> +++ b/gcc/tree-vect-patterns.c
>> @@ -3570,7 +3570,8 @@ vect_recog_mask_conversion_pattern (vec<gimple *> *stmts, tree *type_in,
>>  {
>>    gimple *last_stmt = stmts->pop ();
>>    enum tree_code rhs_code;
>> -  tree lhs, rhs1, rhs2, tmp, rhs1_type, rhs2_type, vectype1, vectype2;
>> +  tree lhs = NULL_TREE, rhs1, rhs2, tmp, rhs1_type, rhs2_type;
>> +  tree vectype1, vectype2;
>>    stmt_vec_info stmt_vinfo = vinfo_for_stmt (last_stmt);
>>    stmt_vec_info pattern_stmt_info;
>>    vec_info *vinfo = stmt_vinfo->vinfo;
> 
> How is the above related to this patch?

Done.

> 
>> +/* Return TRUE if we should instrument for use-after-scope sanity checking.  */                                                                   
>> +                                                                                                                                                  
>> +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;                                                                                                                                
>> +}                                                                                                                                                 
> 
> Formatting, there should be ()s around the whole return expression as it
> spans multiple lines, and it should be indented properly.
> Plus IMHO flag_stack_reuse should be dealt with during option handling.
> 
> 	Jakub
> 

Also done.

Thanks,
Martin

[RFC] Introduce -fsanitize=use-after-scope

Commit Message

Comments

Patch