Page MenuHomePhabricator

[libunwind] Handle G in personality string
Needs ReviewPublic

Authored by fmayer on Jul 1 2022, 9:34 AM.

Details

Reviewers
eugenis
MaskRay
Group Reviewers
Restricted Project
Summary

Tested with the following program:

static volatile int* x = nullptr;

void throws()  __attribute__((noinline)) {
  if (getpid() == 0)
    return;
  throw "error";
}

void maybe_throws()  __attribute__((noinline)) {
  volatile int y = 1;
  x = &y;
  throws();
  y = 2;
}

int main(int argc, char** argv) {
  int y;
  try {
    maybe_throws();
  } catch (const char* e) {
    //printf("Caught\n");
  }
  y = *x;
  printf("%d\n", y); // should be MTE failure.
  return 0;
}

Without this change, we crash on cxa_get_globals when trying to catch
the exception (because the stack frame hadn't been cleared up). With
this change, we crash on the y = *x; as expected, because the stack
frame has been untagged, but the pointer hasn't.

Diff Detail

Event Timeline

fmayer created this revision.Jul 1 2022, 9:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 1 2022, 9:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
fmayer updated this revision to Diff 441774.Jul 1 2022, 12:58 PM

fix unused argument error

fmayer updated this revision to Diff 441789.Jul 1 2022, 3:02 PM

fix mingw build

fmayer updated this revision to Diff 441811.Jul 1 2022, 4:21 PM

clang fmt

fmayer updated this revision to Diff 441825.Jul 1 2022, 5:30 PM

fix build

fmayer updated this revision to Diff 441826.Jul 1 2022, 5:32 PM

comment

fmayer updated this revision to Diff 449466.Tue, Aug 2, 3:52 PM

update comment

fmayer published this revision for review.Thu, Aug 4, 7:32 PM
fmayer added a reviewer: eugenis.

In addition to the test described in the message, I ran the libunwind test suites on FVP with SANITIZE_TARGET=memtag-stack and they still pass.

Herald added a project: Restricted Project. · View Herald TranscriptThu, Aug 4, 7:32 PM
eugenis added inline comments.Fri, Aug 5, 11:13 AM
libunwind/src/DwarfInstructions.hpp
221

Is this right? I'd expect to untag from current frame SP to previous frame SP, not to CFA (which could be at arbitrary offset in the frame AFAIK).

fmayer added inline comments.Fri, Aug 5, 12:42 PM
libunwind/src/DwarfInstructions.hpp
221

ARM64 ABI says this: "The CFA is the value of the stack pointer (sp) at the call site in the previous frame."

(see https://github.com/ARM-software/abi-aa/blob/main/aadwarf64/aadwarf64.rst#42canonical-frame-address)

fmayer updated this revision to Diff 450374.Fri, Aug 5, 1:24 PM

add comment

This seems ok in general, but I'd like someone with better knowledge of the code to review.

libunwind/src/DwarfInstructions.hpp
221

SGTM but change the comment - this is not about SP in the previous frame at all, this is about CFA being at the bottom of the current stack frame.

libunwind/src/DwarfParser.hpp
54

whitespace here and below

libunwind/src/UnwindCursor.hpp
1019

Do we need MTE support here?

fmayer updated this revision to Diff 450452.Fri, Aug 5, 5:14 PM
fmayer marked 4 inline comments as done.

address comments

libunwind/src/UnwindCursor.hpp
1019

I don't think so: This also doesn't handle the PAC BKey, and the plumbing through LLVM and the linker of that is the same as our 'G' flag.