StackProtector: add unwind cleanup paths for instrumentation.

[t.p.northover]

Summary

This is a mitigation patch for https://bugs.chromium.org/p/llvm/issues/detail?id=30, 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.

[t.p.northover]

Ping.

[t.p.northover]

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

[xiangzhangllvm]

I took a quick look, Seems we can't reproduce the report for test in https://bugs.chromium.org/p/llvm/issues/detail?id=30 in latest llvm.

$./a.out
*** 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.

[xiangzhangllvm]

llvm/lib/CodeGen/DwarfEHPrepare.cpp

287

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]

llvm/lib/CodeGen/DwarfEHPrepare.cpp

287

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]

llvm/lib/CodeGen/DwarfEHPrepare.cpp

176

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.

[xiangzhangllvm]

llvm/lib/CodeGen/DwarfEHPrepare.cpp

176

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

[t.p.northover]

Thanks, committed as 203b6f31bb71.

llvm/lib/CodeGen/DwarfEHPrepare.cpp

176

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 {
    func();
  } catch (int i) {
  }
}

[thakis]

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 https://github.com/llvm/llvm-project/issues/61501 for this.

[hans]

Heads up that we bisected test failures in Chromium (targeting iOS/x86_64 simulator) to this revision: https://bugs.chromium.org/p/chromium/issues/detail?id=1428624 Hopefully we'll be able to figure out what's going on next week, but if anyone else has seen problems too it would be interesting to know.

[hans]

In D143637#4237166, @hans wrote:

Heads up that we bisected test failures in Chromium (targeting iOS/x86_64 simulator) to this revision: https://bugs.chromium.org/p/chromium/issues/detail?id=1428624 Hopefully we'll be able to figure out what's going on next week, but if anyone else has seen problems too it would be interesting to know.

I still don't have a root cause or stand-alone repro, but I did notice that this kicks in even with clang's -fno-exceptions, which seems like it might be wrong?

[hans]

Finally, here's a stand-alone reproducer for desktop: https://bugs.chromium.org/p/chromium/issues/detail?id=1428624#c11

Can we revert this until that's addressed?

[t.p.northover]

Oh dear, thanks for investigating this, I didn't notice until I got an e-mail about the revert.

ObjC never fails to disappoint it seems; I'll see what I can do about accommodating it.