On Fri, 18 Feb 2022, Jakub Jelinek wrote:
> Hi!
>
> We ICE on the following testcase, because the asan1 pass decides to
> instrument
> <retval>.x = 0;
> and does that by
> _13 = &<retval>.x;
> .ASAN_CHECK (7, _13, 4, 4);
> <retval>.x = 0;
> and later sanopt pass turns that into:
> _39 = (unsigned long) &<retval>.x;
> _40 = _39 >> 3;
> _41 = _40 + 2147450880;
> _42 = (signed char *) _41;
> _43 = *_42;
> _44 = _43 != 0;
> _45 = _39 & 7;
> _46 = (signed char) _45;
> _47 = _46 + 3;
> _48 = _47 >= _43;
> _49 = _44 & _48;
> if (_49 != 0)
> goto <bb 10>; [0.05%]
> else
> goto <bb 9>; [99.95%]
>
> <bb 10> [local count: 536864]:
> __builtin___asan_report_store4 (_39);
>
> <bb 9> [local count: 1073741824]:
> <retval>.x = 0;
> The problem is during expansion, <retval> isn't marked TREE_ADDRESSABLE,
> even when we take its address in (unsigned long) &<retval>.x.
>
> Now, instrument_derefs has code to avoid the instrumentation altogether
> if we can prove the access is within bounds of an automatic variable in the
> current function and the var isn't TREE_ADDRESSABLE (or we don't instrument
> use after scope), but we do it solely for VAR_DECLs.
>
> I think we should treat RESULT_DECLs exactly like that too, which is what
> the following patch does. I must say I'm unsure about PARM_DECLs, those can
> have different cases, either they are fully or partially passed in
> registers, then if we take parameter's address, they are in a local copy
> inside of a function and so work like those automatic vars. But if they
> are fully passed in memory, we typically just take address of the slot
> and in that case they live in the caller's frame. It is true we don't
> (can't) put any asan padding in between the arguments, so all asan could
> detect in that case is if caller passes fewer on stack arguments or smaller
> arguments than callee accepts. Anyway, as I'm unsure, I haven't added
> PARM_DECLs to that case.
>
> And another thing is, when we actually build_fold_addr_expr, we need to
> mark_addressable the inner if it isn't addressable already.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2022-02-18 Jakub Jelinek <[email protected]>
>
> PR sanitizer/102656
> * asan.cc (instrument_derefs): If inner is a RESULT_DECL and access is
> known to be within bounds, treat it like automatic variables.
> If instrumenting access and inner is {VAR,PARM,RESULT}_DECL from
> current function and !TREE_STATIC which is not TREE_ADDRESSABLE, mark
> it addressable.
>
> * g++.dg/asan/pr102656.C: New test.
>
> --- gcc/asan.cc.jj 2022-02-12 19:14:45.915168286 +0100
> +++ gcc/asan.cc 2022-02-17 17:13:09.346987047 +0100
> @@ -2688,13 +2688,13 @@ instrument_derefs (gimple_stmt_iterator
> return;
>
> poly_int64 decl_size;
> - if (VAR_P (inner)
> + if ((VAR_P (inner) || TREE_CODE (inner) == RESULT_DECL)
> && offset == NULL_TREE
> && DECL_SIZE (inner)
> && poly_int_tree_p (DECL_SIZE (inner), &decl_size)
> && known_subrange_p (bitpos, bitsize, 0, decl_size))
> {
> - if (DECL_THREAD_LOCAL_P (inner))
> + if (VAR_P (inner) && DECL_THREAD_LOCAL_P (inner))
> return;
> /* If we're not sanitizing globals and we can tell statically that this
> access is inside a global variable, then there's no point adding
> @@ -2724,6 +2726,14 @@ instrument_derefs (gimple_stmt_iterator
> }
> }
>
> + if ((VAR_P (inner)
> + || TREE_CODE (inner) == PARM_DECL
> + || TREE_CODE (inner) == RESULT_DECL)
> + && !TREE_STATIC (inner)
> + && decl_function_context (inner) == current_function_decl
> + && !TREE_ADDRESSABLE (inner))
There's no good reason to exclute TREE_STATIC or global vars. If
we take the address of something we should mark it addressable.
I think you can just use DECL_P (inner) as well here for simplicity.
OK with that change.
Thanks,
Richard.
> + mark_addressable (inner);
> +
> base = build_fold_addr_expr (t);
> if (!has_mem_ref_been_instrumented (base, size_in_bytes))
> {
> --- gcc/testsuite/g++.dg/asan/pr102656.C.jj 2022-02-17 17:09:42.758866731
> +0100
> +++ gcc/testsuite/g++.dg/asan/pr102656.C 2022-02-17 17:10:46.212981870
> +0100
> @@ -0,0 +1,27 @@
> +// PR sanitizer/102656
> +// { dg-do compile }
> +// { dg-options "-std=c++20 -fsanitize=address" }
> +
> +#include <coroutine>
> +
> +class promise;
> +
> +struct future {
> + using promise_type = promise;
> + future() = default;
> + int x = 0;
> +};
> +
> +struct promise {
> + future get_return_object() noexcept { return {}; }
> + auto initial_suspend() noexcept { return std::suspend_never{}; }
> + auto final_suspend() noexcept { return std::suspend_never{}; }
> + void return_void() noexcept {}
> + void unhandled_exception() {}
> +};
> +
> +future
> +func ()
> +{
> + co_return;
> +}
>
> Jakub
>
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
