fix(profiler): guard line-number-table copy against stale jmethodID

AGENTS.md

@@ -437,6 +437,19 @@ table: see [doc/build/JdkUpgrades.md](doc/build/JdkUpgrades.md).
 - All code needs to strive to be lean in terms of resources consumption and easy to follow -
     do not shy away from factoring out self containing code to shorter functions with explicit name
 
+## CI / Automation Script Changes
+- Before rewriting or replacing a script invoked from `.gitlab-ci.yml`, `.gitlab/**/*.yml`, or any CI job,
+  run `git log -p -- <script>` (or diff against the version being replaced) and enumerate every behavior
+  branch it has — fallbacks, env-var-driven paths, error exits. Preserve all of them unless explicitly
+  asked to drop one; do not silently narrow behavior while refactoring.
+- Grep the calling `.gitlab-ci.yml` / `.gitlab/**/*.yml` jobs for how the script is invoked (env vars set,
+  exit codes expected) and confirm the rewrite still satisfies every caller, not just the common/local case.
+- When removing an item from a default list or config (an antagonist, a test tag, a feature flag), cite
+  the exact file/line proving it is inert or unused in that context before removing it — never remove
+  based on assumption alone.
+- After changing a CI-invoked script, run a syntax check (e.g. `bash -n <script>`) and trace through each
+  CI job that calls it before considering the change complete.
+
 ### C/C++ Code Style
 - **Indentation**: Match the exact indentation style of the surrounding code in each file. Do not introduce inconsistent indentation — reviewers will flag it.
 - **Minimal complexity**: Do not split inline logic into separate helper functions unless the helpers are reused or the original is genuinely hard to follow. Unnecessary splits add indirection without value.

ddprof-lib/src/main/cpp/counters.h

@@ -74,6 +74,7 @@
   X(UNWINDING_TIME_JVMTI, "unwinding_ticks_jvmti")                             \
   X(CALLTRACE_STORAGE_DROPPED, "calltrace_storage_dropped_traces")             \
   X(LINE_NUMBER_TABLES, "line_number_tables")                                  \
+  X(LINE_NUMBER_TABLE_UNREADABLE, "line_number_table_unreadable")              \
   X(REMOTE_SYMBOLICATION_FRAMES, "remote_symbolication_frames")                \
   X(REMOTE_SYMBOLICATION_LIBS_WITH_BUILD_ID, "remote_symbolication_libs_with_build_id") \
   X(REMOTE_SYMBOLICATION_BUILD_ID_CACHE_HITS, "remote_symbolication_build_id_cache_hits") \

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

@@ -47,6 +47,16 @@
 static const char *const SETTING_RING[] = {NULL, "kernel", "user", "any"};
 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::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
+// before it is trusted to compute a byte range.
+static const jint MAX_LINE_NUMBER_TABLE_ENTRIES = 65535;
+
 // Compute a non-negative event duration from TSC timestamps.  Unsigned u64
 // subtraction wraps to a near-2^64 value when end < start, which can happen if
 // the thread migrates cores between the two TSC reads and the per-core counters
@@ -360,18 +370,50 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method,
       // the JVMTI-allocated memory immediately. This keeps _ptr valid even
       // after the underlying class is unloaded.
       void *owned_table = nullptr;
-      if (line_number_table_size > 0) {
+      if (line_number_table_size > 0 &&
+          line_number_table_size <= MAX_LINE_NUMBER_TABLE_ENTRIES) {
         size_t bytes = (size_t)line_number_table_size * sizeof(jvmtiLineNumberEntry);
+        // GetLineNumberTable() is called on the same possibly-stale jmethodID
+        // that GetMethodDeclaringClass/GetClassSignature/GetMethodName above
+        // were probed for -- the TOCTOU race documented above (class
+        // unloaded between sample capture and dump) applies here just as
+        // 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. 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) {
-          memcpy(owned_table, line_number_table, bytes);
+          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 {
           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
+        // as plausible as the corrupted-pointer case above (both come from
+        // the same GetLineNumberTable() call on the same stale jmethodID);
+        // an implausible entry count -- including a negative one, since this
+        // is a signed jint and a corrupted value can fall on either side of
+        // zero -- means the pointer can't be trusted for Deallocate() either,
+        // so treat it the same as the unreadable case.
+        line_number_table = nullptr;
+        Counters::increment(LINE_NUMBER_TABLE_UNREADABLE);
       }
-      jvmtiError dealloc_err = jvmti->Deallocate((unsigned char *)line_number_table);
-      if (dealloc_err != JVMTI_ERROR_NONE) {
-        TEST_LOG("Unexpected error while deallocating linenumber table: %d", dealloc_err);
+      if (line_number_table != nullptr) {
+        jvmtiError dealloc_err = jvmti->Deallocate((unsigned char *)line_number_table);
+        assert(dealloc_err == JVMTI_ERROR_NONE && "Unexpected error while deallocating linenumber table");
       }
       if (owned_table != nullptr) {
         mi->_line_number_table = std::make_shared<SharedLineNumberTable>(

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

@@ -0,0 +1,206 @@
+/*
+ * Copyright 2026, Datadog, Inc.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// Reproduces a crash reported in production:
+//
+//   __memcpy_evex_unaligned_erms
+//   Lookup::resolveMethod(_asgct_callframe&)
+//   ...
+//   Recording::writeStackTraces(Buffer*, Lookup*)
+//   ...
+//   Profiler::dump / FlightRecorder::dump
+//
+// Lookup::fillJavaMethodInfo() (inlined into resolveMethod() under -O2/-O3)
+// calls jvmtiEnv::GetLineNumberTable(method, ...) and then copied the
+// result with an unguarded:
+//
+//   memcpy(owned_table, line_number_table, bytes);
+//
+// (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
+// before use, see PR #537 / commit ef13aa29f), the line_number_table pointer
+// is used directly.
+//
+// Per the JVMTI spec, GetLineNumberTable() hands back a freshly-allocated,
+// caller-owned array (hence the Deallocate() call after the copy) that is
+// decoupled from the Method's lifetime -- a spec-compliant implementation
+// cannot invalidate it out from under the caller. The actual risk, per the
+// comments already in fillJavaMethodInfo, is the TOCTOU race documented
+// right there: "GetMethodDeclaringClass may return a jclass wrapping a
+// stale/garbage oop when the class was unloaded between sample capture and
+// dump" -- a race against class unloading, not tied to any one JVM vendor.
+// That same comment block notes crash telemetry showed GetClassSignature/
+// GetMethodName returning JVMTI_ERROR_NONE with unmapped string pointers
+// despite the spec saying they should be valid (a separate OpenJ9-specific
+// jmethodID-corruption mode is also handled a few lines above, but is not
+// the only source of this). The production crash motivating this file was
+// on a stock HotSpot JDK 11, confirming the race is not OpenJ9-specific.
+// GetLineNumberTable() is called on the exact same jmethodID as
+// GetMethodDeclaringClass/GetClassSignature/GetMethodName, with nothing
+// about it that would exempt it from that same observed failure mode.
+//
+// These tests isolate the copy step from JVMTI/JNI (which fillJavaMethodInfo
+// requires and which is impractical to fake in a plain gtest) by exercising
+// the exact copy pattern against a pointer whose backing memory is gone --
+// standing in for "GetLineNumberTable() returned a bad pointer" regardless
+// of the precise reason. The result at the memcpy call site is identical
+// either way, so this is sufficient to prove both the crash and the fix.
+
+#include <gtest/gtest.h>
+#include <cstring>
+#include <signal.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include "flightRecorder.h"
+#include "os.h"
+#include "safeAccess.h"
+
+namespace {
+
+// SafeAccess::isReadableRange()/isReadable() rely on SIGSEGV/SIGBUS handlers
+// registered via OS::replaceSigsegvHandler()/replaceSigbusHandler() to catch
+// the fault at a known trampoline address and turn it into a safe return
+// value (see safefetch_ut.cpp for the same pattern). The safefetch trampoline
+// can fault with either signal depending on platform, so both must be
+// installed; otherwise a fault delivered as the other signal is an ordinary
+// unhandled fault.
+void (*orig_segvHandler)(int signo, siginfo_t *siginfo, void *ucontext);
+void (*orig_busHandler)(int signo, siginfo_t *siginfo, void *ucontext);
+
+void lineNumberTableSegvHandler(int signo, siginfo_t *siginfo, void *context) {
+  if (!SafeAccess::handle_safefetch(signo, context)) {
+    if (signo == SIGBUS) {
+      if (orig_busHandler != nullptr) {
+        orig_busHandler(signo, siginfo, context);
+      }
+    } else if (orig_segvHandler != nullptr) {
+      orig_segvHandler(signo, siginfo, context);
+    }
+  }
+}
+
+class LineNumberTableCopyTest : public ::testing::Test {
+protected:
+  void SetUp() override {
+    orig_segvHandler = OS::replaceSigsegvHandler(lineNumberTableSegvHandler);
+    orig_busHandler = OS::replaceSigbusHandler(lineNumberTableSegvHandler);
+  }
+
+  void TearDown() override {
+    OS::replaceSigsegvHandler(orig_segvHandler);
+    OS::replaceSigbusHandler(orig_busHandler);
+  }
+};
+
+// Allocates a page-sized region seeded with `count` jvmtiLineNumberEntry
+// records, standing in for a buffer jvmtiEnv::GetLineNumberTable() would
+// have returned.
+jvmtiLineNumberEntry *makeFakeLineNumberTable(void *page, int count) {
+  jvmtiLineNumberEntry *table = (jvmtiLineNumberEntry *)page;
+  for (int i = 0; i < count; i++) {
+    table[i].start_location = i * 4;
+    table[i].line_number = i + 1;
+  }
+  return table;
+}
+
+} // namespace
+
+// Reproducer: the code shape that used to be in flightRecorder.cpp's
+// fillJavaMethodInfo() before the fix, with no readability guard before the
+// memcpy. Demonstrates that once the source page is gone, the copy step used
+// by resolveMethod() crashes the process rather than failing gracefully.
+TEST(LineNumberTableCopyRawTest, UnguardedCopyCrashesWhenSourceUnmapped) {
+  EXPECT_DEATH(
+      {
+        long page_size = sysconf(_SC_PAGESIZE);
+        void *page = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+                          MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+        if (page == MAP_FAILED) {
+          _exit(1); // treat as death via non-zero exit if mmap itself fails
+        }
+        jint line_number_table_size = 4;
+        jvmtiLineNumberEntry *line_number_table =
+            makeFakeLineNumberTable(page, line_number_table_size);
+
+        // Stand-in for GetLineNumberTable() handing back a bad pointer for a
+        // corrupted/stale jmethodID: the backing memory is simply gone by
+        // the time the copy runs.
+        munmap(page, page_size);
+
+        // This mirrors the pre-fix fillJavaMethodInfo() shape verbatim: no
+        // readability check on line_number_table before the copy.
+        size_t bytes =
+            (size_t)line_number_table_size * sizeof(jvmtiLineNumberEntry);
+        void *owned_table = malloc(bytes);
+        memcpy(owned_table, line_number_table, bytes); // <-- crashes here
+        // Force the copied bytes to be observed before free(); otherwise an
+        // optimizing (Release) build can prove owned_table's contents are
+        // never read and eliminate the memcpy as dead code, silently
+        // skipping the very fault this test exists to demonstrate.
+        volatile unsigned char sink = *(volatile unsigned char *)owned_table;
+        (void)sink;
+        free(owned_table);
+      },
+      "");
+}
+
+// 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,
+                    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  ASSERT_NE(page, MAP_FAILED);
+
+  jint line_number_table_size = 4;
+  jvmtiLineNumberEntry *line_number_table =
+      makeFakeLineNumberTable(page, line_number_table_size);
+
+  ASSERT_EQ(0, munmap(page, page_size));
+
+  size_t bytes = (size_t)line_number_table_size * sizeof(jvmtiLineNumberEntry);
+  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);
+}
+
+// Sanity check: the guard must not reject a genuinely valid table, or every
+// real dump would silently lose line-number info.
+TEST_F(LineNumberTableCopyTest, GuardedCopyStillWorksForValidSource) {
+  jint line_number_table_size = 8;
+  jvmtiLineNumberEntry stack_table[8];
+  jvmtiLineNumberEntry *line_number_table =
+      makeFakeLineNumberTable(stack_table, line_number_table_size);
+
+  size_t bytes = (size_t)line_number_table_size * sizeof(jvmtiLineNumberEntry);
+  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);
+  EXPECT_EQ(0, memcmp(owned_table, line_number_table, bytes));
+  free(owned_table);
+}