[libcxx-commits] [PATCH] D90898: [libunwind] Unwind through aarch64/Linux sigreturn frame

Tue Nov 10 00:14:34 PST 2020

rprichard added a comment.

In D90898#2379387 <https://reviews.llvm.org/D90898#2379387>, @compnerd wrote:

> Could you add a bit of a comment explaining why the XOM case is not protected against?

I think I can expand on the comment.

> libunwind can be used for generating backtraces during regular execution (e.g. profiling) and crashing if the XOM is in use seems unfortunate.

For profiling, I assume the unwind info needs to be accurate at every instruction? I know that's definitely not the case for AArch64 <https://bugs.llvm.org/show_bug.cgi?id=47142>, but I thought it's still only mostly-accurate on other archs? I don't *really* know that unwinding through a signal handler is important for Android, but I don't know how to rule it out either, and we do have a test for it <https://android.googlesource.com/platform/bionic/+/6fc751cca894fb496fb5d854260151bf443229ac/tests/stack_unwinding_test.cpp#114>. I think the test predated Android's libunwindstack unwinder. Maybe someone could be using `_Unwind_Backtrace` in a segfault handler, and that probably avoids the async-unwind-info problem most of the time. But I also don't think that's how third-party Android crash dumpers work.

It would only crash on an XOM function if that function also doesn't have unwind info.

> We could check the memory mapping and just give up in the XOM case.

That could work. Some similar approaches:

- process_vm_readv
- pipe+write
- open /proc/self/mem

They all have the drawback that /proc might be unavailable or that SELinux/seccomp/etc might restrict some system calls.

================
Comment at: libunwind/include/__libunwind_config.h:31
+# if defined(__linux__)
+#  define _LIBUNWIND_TARGET_LINUX 1
+# endif
----------------
compnerd wrote:
> What do you think of having an else clause which sets this to 0 to allow using the macro as a condition rather than checking the definedness of it?
D31078 was a previous attempt to establish a different macro convention for booleans (each macro is either undefined or defined to empty), but it seems to have left a lot of the macros defined to `1` anyway. e.g. `_LIBUNWIND_TARGET_I386` was changed from `1` to empty, but none of the other target macros were affected.

I don't currently have a opinion on what the convention should be, so I was trying to match what I thought the convention was (boolean macros are undefined or defined to `1`, then checked with `defined(...)`).

================
Comment at: libunwind/src/UnwindCursor.hpp:940
+  template <typename R2> bool setInfoForSigReturn(R2 &) { return false; }
+  template <typename R2> int stepThroughSigReturn(R2 &) { return UNW_STEP_END; }
+#endif
----------------
compnerd wrote:
> What do you think of `Registers` instead of `R2`?
Both `R` and `Registers` would be in scope and would be the names of `Registers_*` classes, but that seems OK to me.

================
Comment at: libunwind/src/UnwindCursor.hpp:2016
+  // Look for instructions: mov x8, #0x8b; svc #0x0
+  if (_addressSpace.get32(pc) == 0xd2801168 &&
+      _addressSpace.get32(pc + 4) == 0xd4000001) {
----------------
compnerd wrote:
> It really would be amazing to have a link to the VDSO code here to make it easier to understand the check - I figured it was that this is the implementation, but had to chase through the files.
I think [the vDSO](https://github.com/torvalds/linux/blob/659caaf65dc9c7150aa3e80225ec6e66b25ab3ce/arch/arm64/kernel/vdso/sigreturn.S) is the most canonical implementation -- Bionic uses it, and it looks like glibc also does. It's not universal, though. e.g.:

 * musl provides its own [restore function](https://github.com/ifduyue/musl/blob/d91a6cf6e369a79587c5665fce9635e5634ca201/src/signal/aarch64/restore.s) on all architectures (and doesn't provide DWARF/EHABI unwind info for any of them).

 * The user-mode qemu trampoline doesn't come from a vDSO. When I skimming through the code, I see [something that writes the trampoline onto the stack](https://github.com/qemu/qemu/blob/3c8c36c9087da957f580a9bb5ebf7814a753d1c6/linux-user/aarch64/signal.c#L498-L506).

FWIW, Bionic provides its own restorer function on arm/x86/x86_64 and only uses the kernel's restorer for arm64. This patch is sufficient to fix unwinding through a signal handler for Bionic, but not other targets. e.g. The situations above, but I think x86(-32) with glibc is also affected if there is no vDSO.

I think I can add a link to the vDSO, probably to:
https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/vdso/sigreturn.S

================
Comment at: libunwind/src/UnwindCursor.hpp:2016
+  // Look for instructions: mov x8, #0x8b; svc #0x0
+  if (_addressSpace.get32(pc) == 0xd2801168 &&
+      _addressSpace.get32(pc + 4) == 0xd4000001) {
----------------
danielkiss wrote:
> rprichard wrote:
> > compnerd wrote:
> > > It really would be amazing to have a link to the VDSO code here to make it easier to understand the check - I figured it was that this is the implementation, but had to chase through the files.
> > I think [the vDSO](https://github.com/torvalds/linux/blob/659caaf65dc9c7150aa3e80225ec6e66b25ab3ce/arch/arm64/kernel/vdso/sigreturn.S) is the most canonical implementation -- Bionic uses it, and it looks like glibc also does. It's not universal, though. e.g.:
> > 
> >  * musl provides its own [restore function](https://github.com/ifduyue/musl/blob/d91a6cf6e369a79587c5665fce9635e5634ca201/src/signal/aarch64/restore.s) on all architectures (and doesn't provide DWARF/EHABI unwind info for any of them).
> > 
> >  * The user-mode qemu trampoline doesn't come from a vDSO. When I skimming through the code, I see [something that writes the trampoline onto the stack](https://github.com/qemu/qemu/blob/3c8c36c9087da957f580a9bb5ebf7814a753d1c6/linux-user/aarch64/signal.c#L498-L506).
> > 
> > FWIW, Bionic provides its own restorer function on arm/x86/x86_64 and only uses the kernel's restorer for arm64. This patch is sufficient to fix unwinding through a signal handler for Bionic, but not other targets. e.g. The situations above, but I think x86(-32) with glibc is also affected if there is no vDSO.
> > 
> > I think I can add a link to the vDSO, probably to:
> > https://github.com/torvalds/linux/blob/master/arch/arm64/kernel/vdso/sigreturn.S
> > 
> VDSO is an elf, so we could do something like this:
> 
> ```
>   void* handle = dlopen("linux-vdso.so.1", RTLD_LAZY);
>   const pint_t ptr = (const pint_t)dlsym(handle, "__kernel_rt_sigreturn");
>   if( pc == ptr ) ... 
> 
> ```
> This could help with the XOM issue too. 
It looks like dlopen("linux-vdso.so.1", ...) does find the vDSO with Bionic and glibc, but not with musl. (I wondered about Bionic, because Bionic's dl_iterate_phdr reports the name of the vDSO as `[vdso]` rather than `linux-vdso.so.1`. However, when I tested, it looks like Bionic's dlopen can open `linux-vdso.so` but not `[vdso]`.)

This approach might have trouble with static linking? For statically-linked Bionic programs, a special limited `dl_iterate_phdr` is provided in libc.a, but there is no `dlopen` linked in by default. There is a stub `dlopen` in libdl.a, but [the stub just returns nullptr](https://android.googlesource.com/platform/bionic/+/22a6a058c7c785eff58d6c366bb85082da154080/libdl/libdl_static.cpp#21). For glibc, dlopen also requires linking with -ldl, and then ld.bfd/ld.gold apparently print a warning for static executables:
```
/tmp/test-f61321.o:test.c:function main: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
```

One approach I thought of: change `findUnwindSections` so that it reports a readable range of memory containing the lookup PC. I think this would break with qemu user-mode emulation but would work in most other configurations. (e.g. A static musl executable doesn't look like it shows the vDSO via dl_iterate_phdr, but that's OK if musl provides its own sigreturn function.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90898/new/

https://reviews.llvm.org/D90898