[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 --- Comment #10 from GCC Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:6586359e8e4c611dd96129b5d4f24023949ac3fc commit r14-9445-g6586359e8e4c611dd96129b5d4f24023949ac3fc Author: Jakub Jelinek Date: Wed Mar 13 09:19:05 2024 +0100 asan: Fix ICE during instrumentation of returns_twice calls [PR112709] The following patch on top of the previously posted ubsan/gimple-iterator one handles asan the same. While the case of returning by hidden reference is handled differently because of the first recently posted asan patch, this deals with instrumentation of the aggregates returned in registers case as well as instrumentation of loads from aggregate memory in the function arguments of returns_twice calls. 2024-03-13 Jakub Jelinek PR sanitizer/112709 * asan.cc (maybe_create_ssa_name, maybe_cast_to_ptrmode, build_check_stmt, maybe_instrument_call, asan_expand_mark_ifn): Use gsi_safe_insert_before instead of gsi_insert_before. * gcc.dg/asan/pr112709-2.c: New test.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 --- Comment #9 from GCC Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:364c684c474841e3c9c04e025a5c1bca49705c86 commit r14-9444-g364c684c474841e3c9c04e025a5c1bca49705c86 Author: Jakub Jelinek Date: Wed Mar 13 09:16:45 2024 +0100 gimple-iterator, ubsan: Fix ICE during instrumentation of returns_twice calls [PR112709] ubsan, asan (both PR112709) and _BitInt lowering (PR113466) want to insert some instrumentation or adjustment statements before some statement. This unfortunately creates invalid IL if inserting before a returns_twice call, because we require that such calls are the first statement in a basic block and that we have an edge from the .ABNORMAL_DISPATCHER block to the block containing the returns_twice call (in addition to other edge(s)). The following patch adds helper functions for such insertions and uses it for now in ubsan (I'll post a follow up which uses it in asan and will work later on the _BitInt lowering PR). In particular, if the bb with returns_twice call at the start has just 2 edges, one EDGE_ABNORMAL from .ABNORMAL_DISPATCHER and another (non-EDGE_ABNORMAL/EDGE_EH) from some other bb, it just inserts the statement or sequence on that other edge. If the bb has more predecessor edges or the one not from .ABNORMAL_DISPATCHER is e.g. an EH edge (this latter case likely shouldn't happen, one would need labels or something like that), the patch splits the block with returns_twice call such that there is just one edge next to .ABNORMAL_DISPATCHER edge and adjusts PHIs as needed to make it happen. The functions also replace uses of PHIs from the returns_twice bb with the corresponding PHI arguments, because otherwise it would be invalid IL. E.g. in ubsan/pr112709-2.c (qux) we have before the ubsan pass : # .MEM_5(ab) = PHI <.MEM_4(9), .MEM_25(ab)(11)> # _7(ab) = PHI <_20(9), _8(ab)(11)> # .MEM_21(ab) = VDEF <.MEM_5(ab)> _22 = bar (*_7(ab)); where bar is returns_twice call and bb 11 has .ABNORMAL_DISPATCHER call, this patch instruments it like: : # .MEM_4 = PHI <.MEM_17(ab)(4), .MEM_10(D)(5), .MEM_14(ab)(8)> # DEBUG BEGIN_STMT # VUSE <.MEM_4> _20 = p; # .MEM_27 = VDEF <.MEM_4> .UBSAN_NULL (_20, 0B, 0); # VUSE <.MEM_27> _2 = __builtin_dynamic_object_size (_20, 0); # .MEM_28 = VDEF <.MEM_27> .UBSAN_OBJECT_SIZE (_20, 1024, _2, 0); : # .MEM_5(ab) = PHI <.MEM_28(9), .MEM_25(ab)(11)> # _7(ab) = PHI <_20(9), _8(ab)(11)> # .MEM_21(ab) = VDEF <.MEM_5(ab)> _22 = bar (*_7(ab)); The edge from .ABNORMAL_DISPATCHER is there just to represent the returning for 2nd and later times, the instrumentation can't be done at that point as there is no code executed during that point. The ubsan/pr112709-1.c testcase includes non-virtual PHIs to cover the handling of those as well. 2024-03-13 Jakub Jelinek PR sanitizer/112709 * gimple-iterator.h (gsi_safe_insert_before, gsi_safe_insert_seq_before): Declare. * gimple-iterator.cc: Include gimplify.h. (edge_before_returns_twice_call, adjust_before_returns_twice_call, gsi_safe_insert_before, gsi_safe_insert_seq_before): New functions. * ubsan.cc (instrument_mem_ref, instrument_pointer_overflow, instrument_nonnull_arg, instrument_nonnull_return): Use gsi_safe_insert_before instead of gsi_insert_before. (maybe_instrument_pointer_overflow): Use force_gimple_operand, gimple_seq_add_seq_without_update and gsi_safe_insert_seq_before instead of force_gimple_operand_gsi. (instrument_object_size): Likewise. Use gsi_safe_insert_before instead of gsi_insert_before. * gcc.dg/ubsan/pr112709-1.c: New test. * gcc.dg/ubsan/pr112709-2.c: New test.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709
Jakub Jelinek changed:
What|Removed |Added
Priority|P1 |P2
--- Comment #8 from Jakub Jelinek ---
GCC 13.{1,2} has been released with this bug, so P2.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709
--- Comment #7 from GCC Commits ---
The master branch has been updated by Jakub Jelinek :
https://gcc.gnu.org/g:ad860cc27b3312f9119c7fecb8638a7c1f6d77c9
commit r14-9438-gad860cc27b3312f9119c7fecb8638a7c1f6d77c9
Author: Jakub Jelinek
Date: Tue Mar 12 11:34:50 2024 +0100
asan: Instrument stores in callees rather than callers [PR112709]
asan currently instruments since PR69276 r6-6758 fix calls which store
the return value into memory on the caller side, before the call it
verifies the memory is writable.
Now PR112709 where we ICE on trying to instrument such calls made me
think about whether that is what we want to do.
There are 3 different cases.
One is when a function returns an aggregate which is passed e.g. in
registers, say like struct S { int a[4]; }; returning on x86_64.
That would be ideally instrumented in between the actual call and
storing of the aggregate into memory, but asan currently mostly
works as a GIMPLE pass and arranging for the instrumentation to happen
at that spot would be really hard. We could diagnose after the call
but generally asan attempts to diagnose stuff before something is
overwritten rather than after, or keep the current behavior (that is
what this patch does, which has the disadvantage that it can complain
about UB even for functions which never return and so never actually store,
and doesn't check whether the memory wasn't e.g. poisoned during the call)
or could e.g. instrument both before and after the call (that would have
the disadvantage the current state has but at least would check post-factum
the store again afterwards).
Another case is when a function returns an aggregate through a hidden
reference, struct T { int a[128]; }; on x86_64 or even the above struct S
on ia32 as example. In the actual program such stores happen when storing
something to or its parts in the callee, because there
expands to *hidden_retval. So, IMHO we should instrument those in the
callee rather than caller, that is where the writes are and we can do that
easily. This is what the patch below does.
And the last case is for builtins/internal functions. Usually those don't
return aggregates, but in case they'd do and can be expanded inline, it is
better to instrument them in the caller (as before) rather than not
instrumenting the return stores at all.
I had to tweak the expected output on the PR69276 testcase, because
with the patch it keeps previous behavior on x86_64 (structure returned
in registers, stored in the caller, so reported as UB in A::A()),
while on i686 it changed the behavior and is reported as UB in the
vnull::operator vec which stores the structure, A::A() is then a frame
above it in the backtrace.
2024-03-12 Jakub Jelinek
PR sanitizer/112709
* asan.cc (has_stmt_been_instrumented_p): Don't instrument call
stores on the caller side unless it is a call to a builtin or
internal function or function doesn't return by hidden reference.
(maybe_instrument_call): Likewise.
(instrument_derefs): Instrument stores to RESULT_DECL if
returning by hidden reference.
* gcc.dg/asan/pr112709-1.c: New test.
* g++.dg/asan/pr69276.C: Adjust expected output for some targets.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709
--- Comment #6 from Jakub Jelinek ---
struct S { char c[1024]; };
int foo (int);
__attribute__((returns_twice, noipa)) struct S
bar (int x)
{
(void) x;
struct S s = {};
s.c[42] = 42;
return s;
}
void
baz (struct S *p)
{
foo (1);
*p = bar (0);
}
void
qux (int x, struct S *p)
{
if (x == 25)
x = foo (2);
else if (x == 42)
x = foo (foo (3));
*p = bar (x);
}
void
corge (int x, struct S *p)
{
void *q[] = { &, &, &, & };
if (x == 25)
{
l1:
x = foo (2);
}
else if (x == 42)
{
l2:
x = foo (foo (3));
}
l3:
*p = bar (x);
if (x < 4)
goto *q[x & 3];
}
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 --- Comment #5 from Jakub Jelinek --- Adjusted testcase which shows more cases, like multiple edges into the returns_twice bb in addition to the edge from .ABNORMAL_DISPATCHER.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 --- Comment #4 from Jakub Jelinek --- Thinking about it, I'd say this should be instrumented differently between asan and ubsan. ubsan, which ought to just check whether the pointer is non-NULL and properly aligned, should instrument it in the caller, so for returns_twice on all the edges but the abnormal from .ABNORMAL_DISPATCHER, if there is just one such edge, emit it on that edge, if there are multiple, split the block, add PHIs and move the .ABNORMAL_DISPATCHER edge. Because the function isn't called for the second time actually, the argument where to store it to will be the same in both cases. For asan this is different, while the address to which the result is stored will be the same, the memory might be poisoned in between, so I think we want to instrument that on the callee side when storing into RESULT_DECL.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 Jeffrey A. Law changed: What|Removed |Added CC||law at gcc dot gnu.org Priority|P3 |P1
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 --- Comment #3 from Jakub Jelinek --- The .ASAN_CHECK call before the returns_twice fn call was added in r6-6758-g7db337c247a6f34708b502016d58c2ef9991b2a8 and with .UBSAN_NULL call before it with -fsanitize=undefined since r0-126632-gb9a55b135e5482e2484c27b6233ebf9132347ee5 when -fsanitize=null support has been introduced.
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 --- Comment #2 from Jakub Jelinek --- This isn't specific to asan, -fsanitize=undefined ICEs on it the same. In both cases, we want to add instrumentation for the store of the call lhs. So, either we move the instrumentation on the non-abnormal predecessor edge of the calls, but then we actually don't perform the checking when reaching the call through the abnormal edge (i.e. returning for second and following time from the call, in the caller perhaps the pointer might have changed). Or we stop doing such instrumentations on the call lhs and instead do such instrumentation in the callee (on = whatever; stores).
[Bug sanitizer/112709] [13/14 Regression] address sanitize and returns_twice causes an ICE
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112709 Andrew Pinski changed: What|Removed |Added Status|UNCONFIRMED |NEW Summary|address sanitize and|[13/14 Regression] address |returns_twice causes an ICE |sanitize and returns_twice ||causes an ICE Ever confirmed|0 |1 Last reconfirmed||2023-11-25 Target Milestone|--- |13.3 --- Comment #1 from Andrew Pinski --- Confirmed, the checking is new in GCC 13. Though it seems like GCC 5 produced IR that was ok. I am not 100% sure how we should handle this case either.
