Hi!
Ok, first let me list some needed follow-ups that don't need to be handled
right away:
- r237814-like changes for ASAN_MARK
- optimization to remove ASAN_MARK unpoisoning at the start 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
- try to improve the goto handling
On Mon, Oct 03, 2016 at 11:27:38AM +0200, Martin Liška wrote:
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + fprintf (dump_file, "Skipping stack emission for variable: %s "
> + "(%ldB)\n",
This message is weird. I would have thought you are informing the user
that you are unpoisoning the stack for the specified variable.
> + IDENTIFIER_POINTER (DECL_NAME (decl)),
Can't DECL_NAME be NULL?
> + var_end_offset - var_offset);
> + for (unsigned i = 0; i < size; ++i)
> + {
> + unsigned char shadow_c = c;
> + if (i == size- 1 && last_chunk_size && !is_clobber)
Wrong formatting, space before - missing.
> + g = gimple_build_assign (make_ssa_name (pointer_sized_int_node),
> + NOP_EXPR, base);
Incorrect indentation.
> +
> +/* 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
> + && ASAN_STACK);
> +}
This looks wrong. IMHO you really don't want to use ASAN_STACK in asan.h,
because it
requires params.h. I think what would be best is to prototype
asan_sanitize_stack_p in asan.h, remove static inline from its definition,
and have asan.h
static inline bool
asan_sanitize_use_after_scope (void)
{
return (flag_sanitize_use_after_scope && asan_sanitize_stack_p ();
}
That way, for the common case of no sanitization it would be inline and
fast.
> +static inline bool
> +asan_no_sanitize_address_p (void)
> +{
> + return lookup_attribute ("no_sanitize_address",
> + DECL_ATTRIBUTES (current_function_decl));
> +}
And you could avoid doing this.
> + if ((asan_sanitize_stack_p () || asan_sanitize_use_after_scope ())
And then just drop asan_sanitize_use_after_scope () from cases like this,
because really asan_sanitize_use_after_scope () must imply
asan_sanitize_stack_p ().
> @@ -1127,7 +1127,8 @@ expand_stack_vars (bool (*pred) (size_t), struct
> stack_vars_data *data)
> if (alignb * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT)
> {
> base = virtual_stack_vars_rtx;
> - if (asan_sanitize_stack_p () && pred)
> + if ((asan_sanitize_stack_p () || asan_sanitize_use_after_scope ())
Likewise.
> + && pred)
> @@ -1514,7 +1515,9 @@ 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 ()
> + || asan_sanitize_use_after_scope ())
And again.
> return true;
>
> unsigned int align = TREE_CODE (var) == SSA_NAME
> @@ -2212,7 +2215,7 @@ expand_used_vars (void)
> expand_stack_vars (stack_protect_decl_phase_2, &data);
> }
>
> - if (asan_sanitize_stack_p ())
> + if (asan_sanitize_stack_p () || asan_sanitize_use_after_scope ())
And again.
> --- a/gcc/flag-types.h
> +++ b/gcc/flag-types.h
> @@ -229,6 +229,7 @@ enum sanitize_code {
> SANITIZE_OBJECT_SIZE = 1UL << 20,
> SANITIZE_VPTR = 1UL << 21,
> SANITIZE_BOUNDS_STRICT = 1UL << 22,
> + SANITIZE_USE_AFTER_SCOPE = 1UL << 23,
> SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE |
> SANITIZE_UNREACHABLE
> | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN
> | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM
> @@ -237,7 +238,9 @@ enum sanitize_code {
> | SANITIZE_RETURNS_NONNULL_ATTRIBUTE
> | SANITIZE_OBJECT_SIZE | SANITIZE_VPTR,
> SANITIZE_NONDEFAULT = SANITIZE_FLOAT_DIVIDE | SANITIZE_FLOAT_CAST
> - | SANITIZE_BOUNDS_STRICT
> + | SANITIZE_BOUNDS_STRICT,
> + SANITIZE_ADDRESS_USE_AFTER_SCOPE = SANITIZE_ADDRESS
> + | SANITIZE_USE_AFTER_SCOPE
Looking at this, as -fsanitize-use-after-scope is a separate option, it
shouldn't mess up anything in the SANITIZE_* flags. So just use a
flag_sanitize_use_after_scope var for that. It isn't something where you'd
decide differently on it e.g. for -fno-sanitize= of -fsanitize-recover= etc.
> @@ -1228,6 +1306,14 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> }
> }
> }
> +
> + if (asan_sanitize_use_after_scope ()
> + && asan_poisoned_variables != NULL
> + && asan_poisoned_variables->contains (t))
Do you really need the asan_sanitize_use_after_scope () ? Won't the
hash_set not be populated at all if it isn't true?
> + if (asan_sanitize_use_after_scope ()
> + && !asan_no_sanitize_address_p ()
You wouldn't need to check !asan_no_sanitize_address_p ()
separately here.
Jakub
