fix(profiler): close TOCTOU race in line-number-table copy with safeCopy()

isReadableRange() + memcpy still races if the probed memory changes
mapping between the check and the copy. Use SafeAccess::safeCopy()
instead, which fault-protects each read as it happens.

Author: jbachorik

ddprof-lib/src/main/cpp/flightRecorder.cpp

@@ -50,7 +50,7 @@ static const char *const SETTING_CSTACK[] = {NULL, "no", "fp", "dwarf", "lbr"};
 // JVM spec SS4.7.3 caps a method's bytecode (code_length) at 65535 bytes (u2),
 // so a well-formed LineNumberTable can never have more entries than that.
 // Used to sanity-bound line_number_table_size before it drives the byte-count
-// passed to SafeAccess::isReadableRange()/memcpy(): if GetLineNumberTable()
+// passed to SafeAccess::safeCopy(): if GetLineNumberTable()
 // returns a corrupted pointer for a stale jmethodID (see the TOCTOU race
 // documented in fillJavaMethodInfo below), the paired out-param size is just
 // as likely to be corrupted, and an implausible size should be rejected
@@ -380,19 +380,25 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method,
         // much as to those calls, and crash telemetry already showed those
         // sibling calls returning JVMTI_ERROR_NONE with unmapped string
         // pointers despite the spec saying the returned array should be a
-        // fresh, caller-owned allocation. Nothing about this call guarantees
-        // it is exempt from the same failure mode, so probe before copying
-        // rather than assume the pointer is safe to dereference.
-        if (SafeAccess::isReadableRange(line_number_table, bytes)) {
-          owned_table = malloc(bytes);
-          if (owned_table != nullptr) {
-            memcpy(owned_table, line_number_table, bytes);
-          } else {
-            TEST_LOG("Failed to allocate %zu bytes for line number table copy", bytes);
+        // fresh, caller-owned allocation. A genuinely-valid returned table is
+        // fully decoupled from jmethodID lifetime per the JVMTI spec and
+        // can't be invalidated later by class unload; the actual risk is a
+        // corrupted pointer that happens to alias other live memory at
+        // check-time and stops being mapped moments later. A separate
+        // isReadableRange() probe followed by a plain memcpy() would still
+        // race that window, so copy via safeCopy() instead: it fault-protects
+        // each read as it happens rather than trusting a point-in-time check
+        // before an unprotected copy.
+        owned_table = malloc(bytes);
+        if (owned_table != nullptr) {
+          if (!SafeAccess::safeCopy(owned_table, line_number_table, bytes)) {
+            free(owned_table);
+            owned_table = nullptr;
+            line_number_table = nullptr; // make sure the invalid address is not used for jvmti->Deallocate
+            Counters::increment(LINE_NUMBER_TABLE_UNREADABLE);
           }
         } else {
-          line_number_table = nullptr; // make sure the invalid address is not used for jvmti->Deallocate
-          Counters::increment(LINE_NUMBER_TABLE_UNREADABLE);
+          TEST_LOG("Failed to allocate %zu bytes for line number table copy", bytes);
         }
       } else if (line_number_table_size != 0) {
         // A corrupted size out-param alongside a corrupted pointer is exactly

ddprof-lib/src/test/cpp/lineNumberTableCopy_ut.cpp

@@ -18,9 +18,14 @@
 //
 //   memcpy(owned_table, line_number_table, bytes);
 //
-// (flightRecorder.cpp now guards this copy with SafeAccess::isReadableRange()
-// -- see below -- so the line numbers of the unguarded version are no longer
-// present in the file; this file's tests reproduce that removed pattern.)
+// (flightRecorder.cpp now guards this copy with SafeAccess::safeCopy() -- see
+// below -- so the line numbers of the unguarded version are no longer present
+// in the file; this file's tests reproduce that removed pattern. An earlier
+// version of the fix used a separate SafeAccess::isReadableRange() probe
+// followed by a plain memcpy(), but that still left a check-then-use race:
+// nothing stops the probed memory from becoming invalid in the gap between
+// the probe and the copy. safeCopy() closes that gap by fault-protecting
+// each read as it happens instead of trusting a point-in-time check.)
 //
 // Unlike the class_name/method_name/method_sig strings returned by the
 // preceding JVMTI calls (which ARE probed with SafeAccess::isReadableRange
@@ -152,10 +157,10 @@ TEST(LineNumberTableCopyRawTest, UnguardedCopyCrashesWhenSourceUnmapped) {
       "");
 }
 
-// Documents the fix: guarding the same copy with
-// SafeAccess::isReadableRange() (the same primitive already used to probe
-// class_name/method_name/method_sig in fillJavaMethodInfo) turns the crash
-// into a clean, detectable failure with no memory touched past the guard.
+// Documents the fix: copying via SafeAccess::safeCopy() (which fault-protects
+// each read as it happens, rather than a point-in-time isReadableRange()
+// probe followed by a separate, unprotected memcpy()) turns the crash into a
+// clean, detectable failure with no memory touched past the fault.
 TEST_F(LineNumberTableCopyTest, GuardedCopySkipsSafelyWhenSourceUnmapped) {
   long page_size = sysconf(_SC_PAGESIZE);
   void *page = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
@@ -169,14 +174,14 @@ TEST_F(LineNumberTableCopyTest, GuardedCopySkipsSafelyWhenSourceUnmapped) {
   ASSERT_EQ(0, munmap(page, page_size));
 
   size_t bytes = (size_t)line_number_table_size * sizeof(jvmtiLineNumberEntry);
-  void *owned_table = nullptr;
-  if (SafeAccess::isReadableRange(line_number_table, bytes)) {
-    owned_table = malloc(bytes);
-    memcpy(owned_table, line_number_table, bytes);
+  void *owned_table = malloc(bytes);
+  ASSERT_NE(owned_table, nullptr);
+  if (!SafeAccess::safeCopy(owned_table, line_number_table, bytes)) {
+    free(owned_table);
+    owned_table = nullptr;
   }
 
   EXPECT_EQ(nullptr, owned_table);
-  free(owned_table);
 }
 
 // Sanity check: the guard must not reject a genuinely valid table, or every
@@ -188,10 +193,11 @@ TEST_F(LineNumberTableCopyTest, GuardedCopyStillWorksForValidSource) {
       makeFakeLineNumberTable(stack_table, line_number_table_size);
 
   size_t bytes = (size_t)line_number_table_size * sizeof(jvmtiLineNumberEntry);
-  void *owned_table = nullptr;
-  if (SafeAccess::isReadableRange(line_number_table, bytes)) {
-    owned_table = malloc(bytes);
-    memcpy(owned_table, line_number_table, bytes);
+  void *owned_table = malloc(bytes);
+  ASSERT_NE(owned_table, nullptr);
+  if (!SafeAccess::safeCopy(owned_table, line_number_table, bytes)) {
+    free(owned_table);
+    owned_table = nullptr;
   }
 
   ASSERT_NE(nullptr, owned_table);