Page MenuHomePhabricator

[Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls
ClosedPublic

Authored by yln on Jan 25 2019, 6:17 PM.

Details

Summary

UBSan wants to detect when unreachable code is actually reached, so it
adds instrumentation before every unreachable instruction. However, the
optimizer will remove code after calls to functions marked with
noreturn. To avoid this UBSan removes noreturn from both the call
instruction as well as from the function itself. Unfortunately, ASan
relies on this annotation to unpoison the stack by inserting calls to
_asan_handle_no_return before noreturn functions. This is important for
functions that do not return but access the the stack memory, e.g.,
unwinder functions *like* longjmp (longjmp itself is actually
"double-proofed" via its interceptor). The result is that when ASan and
UBSan are combined, the noreturn attributes are missing and ASan cannot
unpoison the stack, so it has false positives when stack unwinding is
used.

Changes:
Clang-CodeGen now directly insert calls to __asan_handle_no_return
when a call to a noreturn function is encountered and both
UBsan-unreachable and ASan are enabled. This allows UBSan to continue
removing the noreturn attribute from functions without any changes to
the ASan pass.

Previously generated code:

call void @longjmp
call void @__asan_handle_no_return
call void @__ubsan_handle_builtin_unreachable

Generated code (for now):

call void @__asan_handle_no_return
call void @longjmp
call void @__asan_handle_no_return
call void @__ubsan_handle_builtin_unreachable

rdar://problem/40723397

Diff Detail

Repository
rC Clang

Event Timeline

yln created this revision.Jan 25 2019, 6:17 PM
yln set the repository for this revision to rL LLVM.
yln added a project: Restricted Project.
yln added subscribers: llvm-commits, cfe-commits.
eugenis accepted this revision.Jan 28 2019, 5:31 PM

LGTM
Since the previous iteration of this was controversial, please wait for at least one more review.

This revision is now accepted and ready to land.Jan 28 2019, 5:31 PM
vsk added a comment.Jan 28 2019, 5:41 PM

Is it necessary to remove visitation of 'noreturn' call sites from ASan? I'd expect that to break non-C frontends which emit noreturn calls (rust/swift?).

yln added a comment.Jan 29 2019, 11:40 AM
In D57278#1374852, @vsk wrote:

Is it necessary to remove visitation of 'noreturn' call sites from ASan? I'd expect that to break non-C frontends which emit noreturn calls (rust/swift?).

Good point! I didn't think about other frontends.

Currently we insert all noreturn handler calls in the frontend. Since the ASan pass still needs to insert handler calls to support other frontends, I would like to change it so that clang only inserts calls when it is necessary to prevent UBSan incompatibilities. This means that the ASan pass can remain completely unchanged.
(Orthogonal issue exposed by the above: In a follow-up I will change the ASan pass to not intrument !nosanitize calls to get rid of superfluous handler calls.)

@eugenis
We were hoping to only insert noreturn handler calls in one place, but I am not sure this is feasible when we consider alternative frontends. Are you still fine with this?

Sounds good.

yln updated this revision to Diff 184168.Jan 29 2019, 1:59 PM

Directly insert calls to __asan_handle_no_return

Clang-CodeGen now directly insert calls to __asan_handle_no_return
when a call to a noreturn function is encountered and both UBsan
unreachable and ASan are enabled. This allows UBSan to continue
removing the noreturn attribute from functions without any changes to
the ASan pass.

yln edited the summary of this revision. (Show Details)Jan 29 2019, 2:00 PM
vsk accepted this revision.Jan 29 2019, 2:02 PM

LGTM, thanks!

LGTM assuming the plan is to add !nosanitize where !noreturn has been, at the original call site, in the follow-up change.

This revision was automatically updated to reflect the committed changes.