Merge branch 'main' into jb/malloc_profiles

Author: jbachorik

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/main/cpp/frame.h

@@ -79,8 +79,11 @@ class FrameType {
     return (FrameTypeId)((bci >> TYPE_SHIFT) & FRAME_TYPE_MASK);
   }
 
+  // Raw pointers only exist on HotSpot (see hotspotSupport.cpp fillFrameRaw).
+  // On other VMs an unencoded, VM-supplied bci can coincidentally have bit 30
+  // set; that must not be mistaken for our raw-pointer encoding.
   static inline bool isRawPointer(int bci) {
-    return bci > 0 && (bci & RAW_POINTER_MASK) != 0;
+    return VM::isHotspot() && bci > 0 && (bci & RAW_POINTER_MASK) != 0;
   }
 };
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2025 Datadog, Inc
+ * Copyright 2025, 2026 Datadog, Inc
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,12 +16,44 @@
 
 
 #include "safeAccess.h"
+#include <cstdio>
+#include <cstdlib>
 #include <signal.h>
 #include <ucontext.h>
 
 extern "C" int safefetch32_cont(int* adr, int errValue);
 extern "C" int64_t safefetch64_cont(int64_t* adr, int64_t errValue);
 
+// safecopy_impl copies `len` bytes from `src` to `dst` one byte at a time and
+// returns 1 on success. If any load from `src` faults, the signal handler
+// redirects the pc anywhere in [safecopy_impl, safecopy_cont) to safecopy_cont,
+// which returns 0. Both are leaf routines with no stack frame, so the redirect
+// keeps the return address intact.
+extern "C" int safecopy_impl(void* dst, const void* src, size_t len);
+extern "C" int safecopy_cont(void* dst, const void* src, size_t len);
+
+#ifdef DEBUG
+// handle_safefetch protects safeCopy by treating any fault whose pc lands in
+// [safecopy_impl, safecopy_cont) as a recoverable read fault. That range is
+// only valid if the assembler/linker keeps safecopy_cont strictly above
+// safecopy_impl (they are emitted adjacently in one asm block). A future
+// toolchain change could in principle reorder them, collapsing the range to
+// empty and silently disabling fault protection. Assert the invariant once at
+// load time so such a regression aborts loudly at startup instead of turning
+// safeCopy into an unprotected crash. Debug builds only: this is a
+// toolchain-regression tripwire, not a runtime safety check.
+__attribute__((constructor))
+static void verify_safecopy_range() {
+  if ((uintptr_t)safecopy_cont <= (uintptr_t)safecopy_impl) {
+    fprintf(stderr,
+            "FATAL: safecopy_cont (%p) is not above safecopy_impl (%p); "
+            "safeCopy fault protection would be lost\n",
+            (void*)safecopy_cont, (void*)safecopy_impl);
+    abort();
+  }
+}
+#endif // DEBUG
+
 #ifdef __APPLE__
     #if defined(__x86_64__)
       #define current_pc context_rip
@@ -67,6 +99,25 @@ extern "C" int64_t safefetch64_cont(int64_t* adr, int64_t errValue);
         _safefetch64_cont:
             movq %rsi, %rax
             ret
+        .globl _safecopy_impl
+        .private_extern _safecopy_impl
+        _safecopy_impl:
+            xorl %ecx, %ecx
+        Lsafecopy_loop:
+            cmpq %rdx, %rcx
+            jae Lsafecopy_done
+            movzbl (%rsi,%rcx,1), %eax
+            movb %al, (%rdi,%rcx,1)
+            incq %rcx
+            jmp Lsafecopy_loop
+        Lsafecopy_done:
+            movl $1, %eax
+            ret
+        .globl _safecopy_cont
+        .private_extern _safecopy_cont
+        _safecopy_cont:
+            xorl %eax, %eax
+            ret
     )");
   #else
     asm(R"(
@@ -95,6 +146,27 @@ extern "C" int64_t safefetch64_cont(int64_t* adr, int64_t errValue);
         safefetch64_cont:
             movq %rsi, %rax
             ret
+        .globl safecopy_impl
+        .hidden safecopy_impl
+        .type safecopy_impl, %function
+        safecopy_impl:
+            xorl %ecx, %ecx
+        .Lsafecopy_loop:
+            cmpq %rdx, %rcx
+            jae .Lsafecopy_done
+            movzbl (%rsi,%rcx,1), %eax
+            movb %al, (%rdi,%rcx,1)
+            incq %rcx
+            jmp .Lsafecopy_loop
+        .Lsafecopy_done:
+            movl $1, %eax
+            ret
+        .globl safecopy_cont
+        .hidden safecopy_cont
+        .type safecopy_cont, %function
+        safecopy_cont:
+            xorl %eax, %eax
+            ret
     )");
   #endif // __APPLE__
 #elif defined(__aarch64__)
@@ -120,6 +192,25 @@ extern "C" int64_t safefetch64_cont(int64_t* adr, int64_t errValue);
         _safefetch64_cont:
             mov      x0, x1
             ret
+        .globl _safecopy_impl
+        .private_extern _safecopy_impl
+        _safecopy_impl:
+            mov      x3, xzr
+        Lsafecopy_loop:
+            cmp      x3, x2
+            b.hs     Lsafecopy_done
+            ldrb     w4, [x1, x3]
+            strb     w4, [x0, x3]
+            add      x3, x3, #1
+            b        Lsafecopy_loop
+        Lsafecopy_done:
+            mov      w0, #1
+            ret
+        .globl _safecopy_cont
+        .private_extern _safecopy_cont
+        _safecopy_cont:
+            mov      w0, #0
+            ret
     )");
   #else
     asm(R"(
@@ -148,61 +239,39 @@ extern "C" int64_t safefetch64_cont(int64_t* adr, int64_t errValue);
         safefetch64_cont:
             mov      x0, x1
             ret
+        .globl safecopy_impl
+        .hidden safecopy_impl
+        .type safecopy_impl, %function
+        safecopy_impl:
+            mov      x3, xzr
+        .Lsafecopy_loop:
+            cmp      x3, x2
+            b.hs     .Lsafecopy_done
+            ldrb     w4, [x1, x3]
+            strb     w4, [x0, x3]
+            add      x3, x3, #1
+            b        .Lsafecopy_loop
+        .Lsafecopy_done:
+            mov      w0, #1
+            ret
+        .globl safecopy_cont
+        .hidden safecopy_cont
+        .type safecopy_cont, %function
+        safecopy_cont:
+            mov      w0, #0
+            ret
     )");
   #endif
 #endif
 
 bool SafeAccess::safeCopy(void* dst, const void* src, size_t len) {
-  // Two-sentinel pattern (same as isReadable): a real-data word may equal
-  // one sentinel by chance, but not both — if both fetches return their
-  // sentinel, the access truly faulted.
-  //
-  // All safefetch32 loads issued here use 4-byte-aligned addresses. Pages
-  // are 4 KiB (or 16 KiB on Apple Silicon), both divisible by 4, so an
-  // aligned 4-byte load never spans a page boundary. The only fault
-  // possible is when the aligned address itself lies in an unmapped page;
-  // we never spuriously fault on an over-read past `src + len`.
-  static const int32_t SENT_A = (int32_t)0x55AA55AA;
-  static const int32_t SENT_B = (int32_t)0xAA55AA55;
-  uint8_t* d = (uint8_t*)dst;
-  const uint8_t* s = (const uint8_t*)src;
-  size_t i = 0;
-
-  // Front fixup: if `src` is not 4-byte aligned, fetch at the previous
-  // aligned address (1..3 bytes before src). That address lies in the
-  // same 4-byte word as src — and since pages are 4-byte aligned, in
-  // the same page as src. The leading k bytes of the fetched word lie
-  // before the caller's range and are discarded via the +k offset; they
-  // never reach `dst`.
-  size_t k = (uintptr_t)s & 3u;
-  if (k != 0 && i < len) {
-    int32_t* aligned = (int32_t*)(s - k);
-    int32_t v1 = safefetch32_impl(aligned, SENT_A);
-    int32_t v2 = safefetch32_impl(aligned, SENT_B);
-    if (v1 == SENT_A && v2 == SENT_B) {
-      return false;
-    }
-    size_t take = (4 - k < len) ? (4 - k) : len;
-    memcpy(d, ((const uint8_t*)&v1) + k, take);
-    i = take;
-  }
-
-  // Middle + tail: (s + i) is now 4-byte aligned. The final iteration may
-  // load up to 3 over-read bytes past `src + len`, but those bytes sit in
-  // the same 4-byte-aligned word and therefore the same page as the bytes
-  // we actually wanted — never a fault from the over-read alone.
-  while (i < len) {
-    int32_t* aligned = (int32_t*)(s + i);
-    int32_t v1 = safefetch32_impl(aligned, SENT_A);
-    int32_t v2 = safefetch32_impl(aligned, SENT_B);
-    if (v1 == SENT_A && v2 == SENT_B) {
-      return false;
-    }
-    size_t chunk = (len - i >= 4) ? 4 : (len - i);
-    memcpy(d + i, &v1, chunk);  // memcpy from local — no UAF risk
-    i += chunk;
-  }
-  return true;
+  // The copy runs entirely inside the safecopy_impl assembly stub, which
+  // reads `src` one byte at a time. If a load faults, handle_safefetch
+  // redirects execution to safecopy_cont, which returns 0. Because the copy
+  // is byte-granular it only ever touches bytes in [src, src+len) — there is
+  // no over-read past the requested range and no alignment reasoning needed.
+  // A fault mid-copy may leave up to len-1 bytes already written to `dst`.
+  return safecopy_impl(dst, src, len) != 0;
 }
 
 bool SafeAccess::handle_safefetch(int sig, void* context) {
@@ -215,6 +284,11 @@ bool SafeAccess::handle_safefetch(int sig, void* context) {
     } else if (pc == (uintptr_t)safefetch64_impl) {
       uc->current_pc = (uintptr_t)safefetch64_cont;
       return true;
+    } else if (pc >= (uintptr_t)safecopy_impl && pc < (uintptr_t)safecopy_cont) {
+      // Unlike safefetch, the faulting load can be at any pc inside the copy
+      // loop, so match the whole [safecopy_impl, safecopy_cont) range.
+      uc->current_pc = (uintptr_t)safecopy_cont;
+      return true;
     }
   }
   return false;

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

@@ -1,5 +1,6 @@
 /*
  * Copyright 2021 Andrei Pangin
+* Copyright 2026 Datadog, Inc
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -64,11 +65,12 @@ class SafeAccess {
     return safefetch64_impl(ptr, errorValue);
   }
 
-  // Copies up to len bytes from src to dst using safefetch32_impl so that a
-  // page-unmap or repurpose of src memory during the copy does not crash the
-  // process. Returns true on full success, false if any read faulted. dst must
-  // have at least len bytes capacity; reads from src may over-read up to 3
-  // bytes past src+len (over-read is also safefetch-protected).
+  // Copies len bytes from src to dst via the safecopy_impl assembly stub so
+  // that a page-unmap or repurpose of src memory during the copy does not
+  // crash the process. Returns true on full success, false if any read
+  // faulted (in which case dst may hold a partial prefix). dst must have at
+  // least len bytes capacity. The copy is byte-granular, so it never reads
+  // past src+len.
   NOINLINE
   static bool safeCopy(void* dst, const void* src, size_t len);