Safe fetch 64-bit value and pointer (#247)

zhengyu123 · web-flow · commit fec8a131bedf · 2025-07-22T08:46:52.000-04:00
* v0

* v1

* Fix

* v2

* Remove old safe access handler
diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp
@@ -886,28 +886,6 @@ bool Profiler::crashHandler(int signo, siginfo_t *siginfo, void *ucontext) {
     return false;
   }
 
-  uintptr_t length = SafeAccess::skipLoad(pc);
-  if (length > 0) {
-    // Skip the fault instruction, as if it successfully loaded NULL
-    frame.pc() += length;
-    frame.retval() = 0;
-    if (thrd != nullptr) {
-      thrd->exitCrashHandler();
-    }
-    return true;
-  }
-
-  length = SafeAccess::skipLoadArg(pc);
-  if (length > 0) {
-    // Act as if the load returned default_value argument
-    frame.pc() += length;
-    frame.retval() = frame.arg1();
-    if (thrd != nullptr) {
-      thrd->exitCrashHandler();
-    }
-    return true;
-  }
-
   if (WX_MEMORY && Trap::isFaultInstruction(pc)) {
     if (thrd != nullptr) {
       thrd->exitCrashHandler();
diff --git a/ddprof-lib/src/main/cpp/safeAccess.cpp b/ddprof-lib/src/main/cpp/safeAccess.cpp
@@ -20,6 +20,7 @@
 #include <ucontext.h>
 
 extern "C" int safefetch32_cont(int* adr, int errValue);
+extern "C" int64_t safefetch64_cont(int64_t* adr, int64_t errValue);
 
 #ifdef __APPLE__
     #if defined(__x86_64__)
@@ -37,12 +38,12 @@ extern "C" int safefetch32_cont(int* adr, int errValue);
 #endif
 
 /**
- Loading a 32-bit value from specific address, the errValue will be returned
+ Loading a 32-bit/64-bit value from specific address, the errValue will be returned
  if the address is invalid.
  The load is protected by the 'handle_safefetch` signal handler, who sets next `pc`
- to `safefetch32_cont`, upon returning from signal handler, `safefetch32_cont` returns `errValue`
+ to `safefetch32_cont/safefetch64_cont`, upon returning from signal handler,
+ `safefetch32_cont/safefetch64_cont` returns `errValue`
  **/
-
 #if defined(__x86_64__)
   #ifdef __APPLE__
     asm(R"(
@@ -56,6 +57,16 @@ extern "C" int safefetch32_cont(int* adr, int errValue);
         _safefetch32_cont:
             movl %esi, %eax
             ret
+        .globl _safefetch64_impl
+        .private_extern _safefetch64_impl
+        _safefetch64_impl:
+             movq (%rdi), %rax
+             ret
+        .globl _safefetch64_cont
+        .private_extern _safefetch64_cont
+        _safefetch64_cont:
+            movq %rsi, %rax
+            ret
     )");
   #else
     asm(R"(
@@ -72,6 +83,18 @@ extern "C" int safefetch32_cont(int* adr, int errValue);
         safefetch32_cont:
             movl %esi, %eax
             ret
+        .globl safefetch64_impl
+        .hidden safefetch64_impl
+        .type safefetch64_imp, %function
+        safefetch64_impl:
+            movq (%rdi), %rax
+            ret
+        .globl safefetch64_cont
+        .hidden safefetch64_cont
+        .type safefetch64_cont, %function
+        safefetch64_cont:
+            movq %rsi, %rax
+            ret
     )");
   #endif // __APPLE__
 #elif defined(__aarch64__)
@@ -85,6 +108,16 @@ extern "C" int safefetch32_cont(int* adr, int errValue);
         .globl _safefetch32_cont
         .private_extern _safefetch32_cont
         _safefetch32_cont:
+            mov      w0, w1
+            ret
+        .globl _safefetch64_impl
+        .private_extern _safefetch64_impl
+        _safefetch64_impl:
+            ldr      x0, [x0]
+            ret
+        .globl _safefetch64_cont
+        .private_extern _safefetch64_cont
+        _safefetch64_cont:
             mov      x0, x1
             ret
     )");
@@ -101,6 +134,18 @@ extern "C" int safefetch32_cont(int* adr, int errValue);
         .hidden safefetch32_cont
         .type safefetch32_cont, %function
         safefetch32_cont:
+            mov      w0, w1
+            ret
+        .globl safefetch64_impl
+        .hidden safefetch64_impl
+        .type safefetch64_impl, %function
+        safefetch64_impl:
+            ldr      x0, [x0]
+            ret
+        .globl safefetch64_cont
+        .hidden safefetch64_cont
+        .type safefetch64_cont, %function
+        safefetch64_cont:
             mov      x0, x1
             ret
     )");
@@ -114,6 +159,9 @@ bool SafeAccess::handle_safefetch(int sig, void* context) {
     if (pc == (uintptr_t)safefetch32_impl) {
       uc->current_pc = (uintptr_t)safefetch32_cont;
       return true;
+    } else if (pc == (uintptr_t)safefetch64_impl) {
+      uc->current_pc = (uintptr_t)safefetch64_cont;
+      return true;
     }
   }
   return false;
diff --git a/ddprof-lib/src/main/cpp/safeAccess.h b/ddprof-lib/src/main/cpp/safeAccess.h
@@ -23,6 +23,7 @@
 #include <stdint.h>
 
 extern "C" int safefetch32_impl(int* adr, int errValue);
+extern "C" int64_t safefetch64_impl(int64_t* adr, int64_t errValue);
 
 #ifdef __clang__
 #define NOINLINE __attribute__((noinline))
@@ -38,62 +39,31 @@ class SafeAccess {
     return safefetch32_impl(ptr, errorValue);
   }
 
-  static bool handle_safefetch(int sig, void* context);
-
-  NOINLINE NOADDRSANITIZE __attribute__((aligned(16))) static void *load(void **ptr) {
-    return *ptr;
+  static inline int64_t safeFetch64(int64_t* ptr, int64_t errorValue) {
+    return safefetch64_impl(ptr, errorValue);
   }
 
-  NOINLINE NOADDRSANITIZE __attribute__((aligned(16))) static u32 load32(u32 *ptr,
-                                                          u32 default_value) {
-    return *ptr;
-  }
+  static bool handle_safefetch(int sig, void* context);
 
-  NOINLINE NOADDRSANITIZE __attribute__((aligned(16))) static void *
-  loadPtr(void **ptr, void *default_value) {
-    return *ptr;
+  static inline void *load(void **ptr) {
+    return loadPtr(ptr, nullptr);
   }
 
-  NOADDRSANITIZE static uintptr_t skipLoad(uintptr_t pc) {
-    if (pc - (uintptr_t)load < sizeSafeLoadFunc) {
-#if defined(__x86_64__)
-      return *(u16 *)pc == 0x8b48 ? 3 : 0; // mov rax, [reg]
-#elif defined(__i386__)
-      return *(u8 *)pc == 0x8b ? 2 : 0; // mov eax, [reg]
-#elif defined(__arm__) || defined(__thumb__)
-      return (*(instruction_t *)pc & 0x0e50f000) == 0x04100000
-                 ? 4
-                 : 0; // ldr r0, [reg]
-#elif defined(__aarch64__)
-      return (*(instruction_t *)pc & 0xffc0001f) == 0xf9400000
-                 ? 4
-                 : 0; // ldr x0, [reg]
-#else
-      return sizeof(instruction_t);
-#endif
-    }
-    return 0;
+  static inline u32 load32(u32 *ptr, u32 default_value) {
+    int res = safefetch32_impl((int*)ptr, (int)default_value);
+    return static_cast<u32>(res);
   }
 
-  static uintptr_t skipLoadArg(uintptr_t pc) {
-#if defined(__aarch64__)
-    if ((pc - (uintptr_t)load32) < sizeSafeLoadFunc ||
-        (pc - (uintptr_t)loadPtr) < sizeSafeLoadFunc) {
-      return 4;
-    }
-#endif
-    return 0;
+  static inline void *loadPtr(void** ptr, void* default_value) {
+  #if defined(__x86_64__) || defined(__aarch64__)
+    int64_t res = safefetch64_impl((int64_t*)ptr, (int64_t)reinterpret_cast<uintptr_t>(default_value));
+    return (void*)static_cast<uintptr_t>(res);
+  #elif defined(__i386__) || defined(__arm__) || defined(__thumb__)
+    int res = safefetch32_impl((int*)ptr, (int)default_value);
+    return (void*)res;
+  #endif
+    return *ptr;
   }
-#ifndef __SANITIZE_ADDRESS__
-  constexpr static size_t sizeSafeLoadFunc = 16;
-#else
-  // asan significantly increases the size of the load function
-  // checking disassembled code can help adjust the value
-  // gdb --batch -ex 'disas _ZN10SafeAccess4loadEPPv' ./elfparser_ut
-  // I see that the functions can also have a 156 bytes size for the load32
-  // and 136 for the loadPtr functions
-  constexpr static inline size_t sizeSafeLoadFunc = 132;
-#endif
 };
 
 #endif // _SAFEACCESS_H
diff --git a/ddprof-lib/src/test/cpp/safefetch_ut.cpp b/ddprof-lib/src/test/cpp/safefetch_ut.cpp
@@ -1,5 +1,6 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include <climits>
 #include <signal.h>
 
 #include "safeAccess.h"
@@ -34,18 +35,64 @@ class SafeFetchTest : public ::testing::Test {
 };
 
 
-TEST_F(SafeFetchTest, validAccess) {
+TEST_F(SafeFetchTest, validAccess32) {
   int i = 42;
   int* p = &i;
   int res = SafeAccess::safeFetch32(p, -1);
   EXPECT_EQ(res, i);
+  i = INT_MAX;
+  res = SafeAccess::safeFetch32(p, -1);
+  EXPECT_EQ(res, i);
+  i = INT_MIN;
+  res = SafeAccess::safeFetch32(p, 0);
+  EXPECT_EQ(res, i);
 }
 
 
-TEST_F(SafeFetchTest, invalidAccess) {
+TEST_F(SafeFetchTest, invalidAccess32) {
   int* p = nullptr;
   int res = SafeAccess::safeFetch32(p, -1);
   EXPECT_EQ(res, -1);
   res = SafeAccess::safeFetch32(p, -2);
   EXPECT_EQ(res, -2);
 }
+
+TEST_F(SafeFetchTest, validAccess64) {
+  int64_t i = 42;
+  int64_t* p = &i;
+  int64_t res = SafeAccess::safeFetch64(p, -1);
+  EXPECT_EQ(res, i);
+  i = LONG_MIN;
+  res = SafeAccess::safeFetch64(p, -19);
+  EXPECT_EQ(res, i);
+  i = LONG_MAX;
+  res = SafeAccess::safeFetch64(p, -1);
+  EXPECT_EQ(res, i);
+}
+
+TEST_F(SafeFetchTest, invalidAccess64) {
+  int64_t* p = nullptr;
+  int64_t res = SafeAccess::safeFetch64(p, -1);
+  EXPECT_EQ(res, -1);
+  res = SafeAccess::safeFetch64(p, -2);
+  EXPECT_EQ(res, -2);
+}
+
+TEST_F(SafeFetchTest, validAccessPtr) {
+  char c = 'a';
+  void* p = (void*)&c;
+  void** pp = &p;
+  void* res = SafeAccess::loadPtr(pp, nullptr);
+  EXPECT_EQ(res, p);
+}
+
+TEST_F(SafeFetchTest, invalidAccessPtr) {
+  int a, b;
+  void* ap = (void*)&a;
+  void* bp = (void*)&b;
+  void** pp = nullptr;
+  void* res = SafeAccess::loadPtr(pp, ap);
+  EXPECT_EQ(res, ap);
+  res = SafeAccess::loadPtr(pp, bp);
+  EXPECT_EQ(res, bp);
+}
