Page MenuHomePhabricator

StackProtector: add unwind cleanup paths for instrumentation.

Authored by t.p.northover on Feb 9 2023, 3:35 AM.



This is a mitigation patch for, where existing stack protection is skipped if a function is returned through by an unwinder rather than the normal call/return path. The recent patch D139254 added the ability to instrument a visible unwind path, at least in the IR case (I'm working on the SelectionDAG instrumentation too) but there are still invisible unwinds it can't reach.

So this patch adds logic to DwarfEHPrepare that goes through a function, converting any call that might throw into an invoke to a simple resume cleanup, and adding cleanup clauses to existing landingpads that lack them. Obviously we don't really want to do this if it's wasted effort, so I also exposed requiresStackProtector from the actual StackProtector code to skip the extra paths if they won't be used.

Diff Detail

Unit TestsFailed

60,030 msx64 debian > libFuzzer.libFuzzer::minimize_crash.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/NullDerefTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/minimize_crash.test.tmp-NullDerefTest

Event Timeline

t.p.northover created this revision.Feb 9 2023, 3:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 3:35 AM
t.p.northover requested review of this revision.Feb 9 2023, 3:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 3:35 AM

Realised I hadn't updated the DomTree correctly first time round.

xiangzhangllvm added a comment.EditedFeb 21 2023, 10:19 PM

I took a quick look, Seems we can't reproduce the report for test in in latest llvm.

*** stack smashing detected ***: terminated
Aborted (core dumped)
$./a.out test
*** stack smashing detected ***: terminated
Aborted (core dumped)

or we mark "noreturn" for all the throw cases.
I'll take a deeper look on this patch. Anyway thanks first.


I just header the func __gxx_personality_v0 in libc++ will handle the exception.
Does all the callees in the caller (with personality attribute) may do invisible unwinds for exception ? So you visibly generate the cleanup resume (to let current stack protect can see it then generate protection code. )

t.p.northover added inline comments.Thu, Mar 9, 1:40 AM

That's pretty much it.

Actually, even without a personality function callees might unwind through this function (that just needs uwtables or similar) but without a personality function we have no way to intercept the exception and check the stack is OK on the way back through.

xiangzhangllvm accepted this revision.Thu, Mar 9, 5:42 PM
xiangzhangllvm added inline comments.

LGTM, just a question.
Do we will really generate a Landing Pad for try catch pairs without cleanup in real world ?
Should it report error earlier if without cleanup?
Anyway this patch enhance for them.

This revision is now accepted and ready to land.Thu, Mar 9, 5:42 PM
xiangzhangllvm added inline comments.Thu, Mar 9, 6:15 PM

Sorry, here only for personality function not visible try catch.

t.p.northover closed this revision.Thu, Mar 16, 4:34 AM

Thanks, committed as 203b6f31bb71.


Do we will really generate a Landing Pad for try catch pairs without cleanup in real world ?

Yes, cleanup is only used if there's some code that needs to be run regardless of whether an exception is caught (e.g. C++ destructors). So for example this leads to a landingpad without cleanup:

void func();
void foo() {
  try {
  } catch (int i) {
thakis added a subscriber: thakis.Sat, Mar 18, 2:17 PM

It looks like this broke building libunwind. Building it now fails with (when building clang and lld and so on with this patch, and then building libunwind with just-built clang and lld and so on):

ld64.lld: error: undefined symbol: __gxx_personality_v0

I suppose this needs some flag to turn it off when building libunwind?

This is in a GN build. I didn't check if it's also broken in a cmake build as it's difficult (for me, at least) to do a build of libunwind that builds with just-build clang and lld. The GN build doesn't build libunwind with any -fstack-protector- or _FORTIFY_SOURCE things, but maybe it does something else silly that causes stack protection to fire when it shouldn't (…any ideas what this might be?). Or maybe stack protection is just enabled by default on darwin? But I don't see a fno-stack-protector flag in the libunwind cmake build either.

…hm, adding "-fno-stack-protector" to libunwind's cflags seems to unbreak the libunwind build. So I figure I'll add that in the GN build. The CMake build doesn't seem to explicitly add that, so I'm guessing libunwind is broken there too. Can you look at that?

(clang/cmake/caches/Apple-stage2.cmake globally adds -fno-stack-protector as far as I can tell, so the Apple bots probably don't show this if they use that cache file. But if you don't use that cache file, it's probably broken now?)

I filed for this.