[libcxx-commits] [PATCH] D123428: [libunwind] Add configuration to disable sigreturn frame check

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai created this revision.
smeenai added reviewers: compnerd, danielkiss, rprichard.
Herald added subscribers: libcxx-commits, kristof.beyls, mgorny.
Herald added projects: libunwind, All.
Herald added a reviewer: libunwind.
smeenai requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

https://reviews.llvm.org/D90898 added a check to libunwind for a
sigreturn frame on Linux aarch64. The check operates by reading from
memory, and the patch notes the possibility of segfaults occurring as a
result. We're observing such segfaults internally (possibly due to
invalid unwind info in libraries which are out of our control). Add a
configuration option to omit this sigreturn check, for users who are
willing to give up the ability to unwind through sigreturn frames in
return for avoiding potential segfaults.

The alternative would be to check memory before attempting to access it,
and that's the approach taken by nongnu libunwind [1]. There were
concerns raised on the original review about the syscalls needed for
this (e.g. `pipe`) not being avaiable because of OS security
configurations, and there's also the overhead incurred by the memory
checking, so I'm inclined to go with the current approach for now.

[1] https://github.com/libunwind/libunwind/blob/e07b43c02d5cf1ea060c018fdf2e2ad34b7c7d80/src/aarch64/Ginit.c#L187


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123428

Files:
  libunwind/CMakeLists.txt
  libunwind/src/UnwindCursor.hpp


Index: libunwind/src/UnwindCursor.hpp
===================================================================
--- libunwind/src/UnwindCursor.hpp
+++ libunwind/src/UnwindCursor.hpp
@@ -25,6 +25,11 @@
   #include <mach-o/dyld.h>
 #endif
 
+#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64) &&  \
+    !defined(_LIBUNWIND_OMIT_LINUX_AARCH64_SIGRETURN_CHECK)
+#define _LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN
+#endif
+
 #if defined(_LIBUNWIND_SUPPORT_SEH_UNWIND)
 // Provide a definition for the DISPATCHER_CONTEXT struct for old (Win7 and
 // earlier) SDKs.
@@ -937,7 +942,7 @@
   }
 #endif
 
-#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#if defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
   bool setInfoForSigReturn() {
     R dummy;
     return setInfoForSigReturn(dummy);
@@ -1226,7 +1231,7 @@
   unw_proc_info_t  _info;
   bool             _unwindInfoMissing;
   bool             _isSignalFrame;
-#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#if defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
   bool             _isSigReturn = false;
 #endif
 };
@@ -1925,7 +1930,7 @@
 
 template <typename A, typename R>
 void UnwindCursor<A, R>::setInfoBasedOnIPRegister(bool isReturnAddress) {
-#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#if defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
   _isSigReturn = false;
 #endif
 
@@ -2027,7 +2032,7 @@
   }
 #endif // #if defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
 
-#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#if defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
   if (setInfoForSigReturn())
     return;
 #endif
@@ -2036,7 +2041,7 @@
   _unwindInfoMissing = true;
 }
 
-#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#if defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
 template <typename A, typename R>
 bool UnwindCursor<A, R>::setInfoForSigReturn(Registers_arm64 &) {
   // Look for the sigreturn trampoline. The trampoline's body is two
@@ -2096,7 +2101,7 @@
   _isSignalFrame = true;
   return UNW_STEP_SUCCESS;
 }
-#endif // defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#endif // defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
 
 template <typename A, typename R>
 int UnwindCursor<A, R>::step() {
@@ -2106,7 +2111,7 @@
 
   // Use unwinding info to modify register set as if function returned.
   int result;
-#if defined(_LIBUNWIND_TARGET_LINUX) && defined(_LIBUNWIND_TARGET_AARCH64)
+#if defined(_LIBUNWIND_CHECK_LINUX_AARCH64_SIGRETURN)
   if (_isSigReturn) {
     result = this->stepThroughSigReturn();
   } else
Index: libunwind/CMakeLists.txt
===================================================================
--- libunwind/CMakeLists.txt
+++ libunwind/CMakeLists.txt
@@ -344,6 +344,10 @@
   add_compile_definitions(_LIBUNWIND_REMEMBER_HEAP_ALLOC)
 endif()
 
+if(LIBUNWIND_OMIT_LINUX_AARCH64_SIGRETURN_CHECK)
+  add_compile_definitions(_LIBUNWIND_OMIT_LINUX_AARCH64_SIGRETURN_CHECK)
+endif()
+
 # This is the _ONLY_ place where add_definitions is called.
 if (MSVC)
   add_definitions(-D_CRT_SECURE_NO_WARNINGS)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D123428.421654.patch
Type: text/x-patch
Size: 3180 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220409/c2a16da1/attachment.bin>