Re: [PATCH, RFC] Introduce -fsanitize=use-after-scope

[Jakub Jelinek <jakub@redhat.com>]

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.

> 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.

> 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?

> @@ -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 (.
> @@ -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?

	Jakub