[PATCH, RFC] Introduce -fsanitize=use-after-scope (v2)

Jakub Jelinek jakub@redhat.com
Fri Oct 21 14:26:00 GMT 2016


On Wed, Oct 12, 2016 at 04:07:53PM +0200, Martin Liška wrote:
> > Ok, first let me list some needed follow-ups that don't need to be handled
> > right away:
> > - r237814-like changes for ASAN_MARK
> 
> Yes, that's missing. I'm wondering whether the same approach would be viable as
> the {un}poisoning happens during gimplification. Thus it generates &var expressions
> which verifier won't be happy about?

Sure, it uses &var.  The trick is that the addressable sub-pass then ignores
the taking of the address just in ASAN_MARK, and if some var is determined
to be addressable solely because of ASAN_MARK &var uses and no other reason,
the ASAN_MARK would be dropped and variable rewritten into SSA form.

> > - optimization to remove ASAN_MARK unpoisoning at the start of the function
> 
> As current implementation does not poison variables at the very beginning of
> a functions (RTL stack frame emission), it won't be needed.

But you still ASAN_MARK unpoison the vars when they get into scope, right?
And those can be removed if the optimizers could prove that the area has not
been poisoned yet since the beginning of the function.

> > - optimization to remove ASAN_MARK poisoning at the end of function (and
> >   remove epilogue unpoisoning)
> > - optimization to remove ASAN_MARK unpoisoning followed by ASAN_MARK poisoning
> >   or vice versa if there are no memory accesses in between
> 
> Yep, both are not handled and are very similar from my perspective: pairing
> poisoning and unpoisoning pair which are not interfered by a memory operation
> in between (in dominator meaning of word).
> I'm wondering whether can be done effectively as we would need to visit all BBs
> in a dominance domain (get_all_dominated_blocks) and check for the memory operations.
> One improvement can be set of BBs that do not have any memory operations (excluding
> ASAN_CHECK, ASAN_MARK builtins), but it can be still expensive to simplify poisoning
> for all local variables. Or am I wrong?

My memory is weak, but isn't this something the sanopt pass
(sanopt_optimize) is already doing similarly for e.g. ASAN_CHECK, UBSAN_NULL
and UBSAN_VPTR checks?  For ASAN_MARK, you actually don't care about any
calls, those shouldn't unpoison or poison again the vars under compiler's
back.

> > - try to improve the goto handling
> 
> Works for me to be target for stage3.

Sure.

> 2016-09-27  Martin Liska  <mliska@suse.cz>

Likely newer date :)

> 	* c-common.c (warn_for_unused_label): Save all labels used
> 	in goto or in &label;

&label.
instead?

> +	      if (dump_file && (dump_flags & TDF_DETAILS))
> +		{
> +		  const char *n = DECL_NAME (decl)
> +		    ? IDENTIFIER_POINTER (DECL_NAME (decl)) : "<unknown>";

Bad formatting.

		  const char *n = (DECL_NAME (decl)
				   ? IDENTIFIER_POINTER (DECL_NAME (decl))
				   : "<unknown>");
or
		  const char *n
		    = (DECL_NAME (decl)
		       ? IDENTIFIER_POINTER (DECL_NAME (decl)) : "<unknown>");

>  /* Return true if DECL should be guarded on the stack.  */
>  
>  static inline bool
>  asan_protect_stack_decl (tree decl)
>  {
> -  return DECL_P (decl) && !DECL_ARTIFICIAL (decl);
> +  return DECL_P (decl) && TREE_ADDRESSABLE (decl);
>  }

Can you explain this change?  It won't affect just
-fsanitize-use-after-scope, and goes in both directions, adds some
decls that weren't previously protected and removes others that were
previously protected.

Is the removal of !DECL_ARTIFICIAL so that you can use-after-scope
verify the C++ TARGET_EXPR temporaries?  Do we need to stack protect
those even for -fno-sanitize-use-after-scope?
I'm not 100% sure if we keep TREE_ADDRESSABLE meaningful for variables that
need to live in memory for other reasons until expansion.

So, I wonder if we don't want && (TREE_ADDRESSABLE (decl) || !DECL_ARTIFICAL (decl))
or just DECL_P (decl); or conditionalize something on
-fsanitize-use-after-scope.  Perhaps the change is good as is, just want to
point out that it affects also other -fsanitize=address modes in both
ways (instruments something that hasn't been before, and stops instrumenting
something that has been before).

> @@ -1514,7 +1503,8 @@ defer_stack_allocation (tree var, bool toplevel)
>    /* If stack protection is enabled, *all* stack variables must be deferred,
>       so that we can re-order the strings to the top of the frame.
>       Similarly for Address Sanitizer.  */
> -  if (flag_stack_protect || asan_sanitize_stack_p ())
> +  if (flag_stack_protect
> +      || asan_sanitize_stack_p ())
>      return true;

This hunk isn't needed, if all the conditions fit on one line,
one line is better.

	Jakub



More information about the Gcc-patches mailing list