[llvm-dev] HWASAN Exception handling question

[Peter Collingbourne via llvm-dev]

HI Matthew,

I think you may be correct. It makes sense that an unwinder would need to
visit the frame that called _Unwind_Resume again when unwinding. And when
it does, there will be no landing pad associated with the _Unwind_Resume
call, so the unwinder will be called with _URC_CONTINUE_UNWIND and the
stack frame will be cleared. I verified that with your change to the LLVM
pass and this change to the hwasan test suite:

diff --git a/compiler-rt/test/hwasan/TestCases/try-catch.cpp
b/compiler-rt/test/hwasan/TestCases/try-catch.cpp
index 8e35a9dd2a5..4d186bf17a6 100644
--- a/compiler-rt/test/hwasan/TestCases/try-catch.cpp
+++ b/compiler-rt/test/hwasan/TestCases/try-catch.cpp
@@ -23,6 +23,11 @@ void h() {

 __attribute__((noinline))
 void g() {
+  // Make sure that we need to resume when unwinding past this function.
+  struct S {
+    ~S() { optimization_barrier((void *)this); }
+  } s;
+
   char x[1000];
   optimization_barrier(x);
   h();

the hwasan tests continue to pass on Android (with both the libgcc unwinder
and LLVM libunwind).

There are a couple of things that I can think of that could make things go
wrong here:
(1) The function's landing pad adjusts SP such that it no longer covers the
local variables before calling _Unwind_Resume. This would mean that some
local variables won't get their tags cleared.
(2) The landing pad tail calls _Unwind_Resume (which would probably require
doing (1) first). That would mean that the unwinder wouldn't see the
_Unwind_Resume caller's stack frame at all.

I'm not sure why a compiler would do (1) since it would seem to result in
larger code and more unwind info. And I've been unable to provoke the
compiler into tail calling _Unwind_Resume either. Tim, do you know of any
circumstances where the AArch64 backend would do either of these things? If
not, I think we might consider going ahead with your proposed change.

Peter

On Tue, Nov 12, 2019 at 6:06 AM Matthew Malcomson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> Hello,
>
> When looking into the way that hwasan handles C++ exceptions I found
> myself wondering about the need for landing pad cleanups (as documented
> in the code
>
> https://github.com/llvm-mirror/compiler-rt/blob/69445f095c22aac2388f939bedebf224a6efcdaf/lib/hwasan/hwasan_exceptions.cpp#L51
> ).
>
> The comment above that code says:
> // We only untag frames without a landing pad because landing pads are
> // responsible for untagging the stack themselves if they resume.
>
> I think the code as implemented untags all frames as it goes past
> whether or not they have a landing pad, and am hoping people can check
> or correct my understanding.
>
> It seems that personality routine will eventually return
> _URC_CONTINUE_UNWIND if this stack frame is to be unwound past (and
> hence the frame will eventually get untagged).  If the frame has landing
> pads then that just means the personality routine will be called
> multiple times for this frame before returning that value.
>
>
> I took a look by testing code generated with a clang patched to avoid
> adding instrumentation to landing pads and it seems that the personality
> wrapper does indeed clear stack frames with a landing pad in the basic
> case.
>
>
> Is there an edge case where the personality wrapper is known to not be
> sufficient?  If not would removing that extra instrumentation sound
> reasonable?
>
>
> The test I ran was to generate code with a clang patched with the diff
> below, and run it in a debugger.
> I'm attaching the annotated gdb session to demonstrate my reasoning.  I
> checked that the shadow memory is not untagged in a landing pad and that
> after _Unwind_Resume the personality wrapper is run again, eventually
> returning _URC_CONTINUE_UNWIND and untagging the shadow memory.
>
>
> diff --git a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> index df7606d..ca97d72 100644
> --- a/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> +++ b/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
> @@ -1110,8 +1110,7 @@ bool HWAddressSanitizer::sanitizeFunction(Function
> &F) {
>              continue;
>            }
>
> -      if (isa<ReturnInst>(Inst) || isa<ResumeInst>(Inst) ||
> -          isa<CleanupReturnInst>(Inst))
> +      if (isa<ReturnInst>(Inst) || isa<CleanupReturnInst>(Inst))
>            RetVec.push_back(&INST);
>
>          if (auto *DDI = dyn_cast<DbgDeclareInst>(&Inst))
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>


-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191114/7e76be6b/attachment.html>