Guard Recording::writeElement against SIGSEGV on a corrupted metadata tree

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

@@ -134,6 +134,11 @@
   X(SAFECOPY_FAILED, "safecopy_failed")                                       \
   X(SAFEFETCH_FAILED, "safefetch_failed")                                     \
   X(STACKWALK_LONGJMP_RECOVERED, "stackwalk_longjmp_recovered")               \
+  /* writeElement() guards against a corrupted/dangling JfrMetadata tree.     \
+   * Root cause is still unconfirmed, so these counters are the durable       \
+   * signal for spotting a recurrence. */                                     \
+  X(METADATA_TREE_NULL_CHILD, "metadata_tree_null_child")                     \
+  X(METADATA_TREE_DEPTH_EXCEEDED, "metadata_tree_depth_exceeded")             \
   DD_COUNTER_TABLE_FAULT_INJECTION(X)                                          \
   DD_COUNTER_TABLE_FI_DEBUG(X)                                                 \
   DD_COUNTER_TABLE_DEBUG(X)

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

@@ -1145,7 +1145,35 @@ void Recording::writeHeader(Buffer *buf) {
   flushIfNeeded(buf);
 }
 
-void Recording::writeElement(Buffer *buf, const Element *e) {
+size_t Recording::countSerializableChildren(
+    const std::vector<const Element *> &children, int depth) {
+  // Children one level deeper than `depth` are what writeElement() would
+  // truncate on its own depth check, so exclude them here too, before being
+  // counted, so child_count always matches the number of children actually
+  // serialized below (an inflated count would make the metadata stream
+  // itself malformed).
+  bool truncate_children = depth + 1 > 10;
+
+  size_t child_count = 0;
+  for (size_t i = 0; i < children.size(); i++) {
+    if (children[i] == nullptr) {
+      Counters::increment(METADATA_TREE_NULL_CHILD);
+      fprintf(stderr, "[ddprof] [WARN] writeElement skipping null child at index %zu\n", i);
+    } else if (truncate_children) {
+      Counters::increment(METADATA_TREE_DEPTH_EXCEEDED);
+      fprintf(stderr, "[ddprof] [WARN] writeElement truncating child at index %zu, depth limit exceeded\n", i);
+    } else {
+      child_count++;
+    }
+  }
+  return child_count;
+}
+
+void Recording::writeElement(Buffer *buf, const Element *e, int depth) {
+  if (e == nullptr) {
+    return;
+  }
+
   buf->putVar64(e->_name);
 
   buf->putVar64(e->_attributes.size());
@@ -1155,10 +1183,18 @@ void Recording::writeElement(Buffer *buf, const Element *e) {
     buf->putVar64(e->_attributes[i]._value);
   }
 
-  buf->putVar64(e->_children.size());
-  for (size_t i = 0; i < e->_children.size(); i++) {
-    flushIfNeeded(buf);
-    writeElement(buf, e->_children[i]);
+  bool truncate_children = depth + 1 > 10;
+  size_t child_count = countSerializableChildren(e->_children, depth);
+
+  buf->putVar64(child_count);
+  if (!truncate_children) {
+    for (size_t i = 0; i < e->_children.size(); i++) {
+      if (e->_children[i] == nullptr) {
+        continue;
+      }
+      flushIfNeeded(buf);
+      writeElement(buf, e->_children[i], depth + 1);
+    }
   }
   flushIfNeeded(buf);
 }

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

@@ -174,13 +174,26 @@ class Recording {
   friend ObjectSampler;
   friend Profiler;
   friend Lookup;
+  // Grants gtest access to the private countSerializableChildren() helper below,
+  // since Recording itself can't be constructed in a plain gtest binary (its
+  // constructor needs a live JVMTI environment). Same pattern as
+  // VMTestAccessor/ProfilerTestAccessor in the test sources.
+  friend class RecordingTestAccessor;
 
 private:
   static char *_agent_properties;
   static char *_jvm_args;
   static char *_jvm_flags;
   static char *_java_command;
 
+  // Determines how many of `children` writeElement() will actually serialize
+  // at the given depth, applying the same null-child and depth-limit skip
+  // rules the recursive writer uses. Both the child_count written to the
+  // buffer and the recursion in writeElement() call this single function, so
+  // the encoded count can never diverge from what actually gets serialized.
+  static size_t countSerializableChildren(
+      const std::vector<const Element *> &children, int depth);
+
   RecordingBuffer _buf[CONCURRENCY_LEVEL];
   // we have several tables to avoid lock contention
   // we have a second dimension to allow a switch in the active table
@@ -236,7 +249,7 @@ class Recording {
 
   void writeMetadata(Buffer *buf);
 
-  void writeElement(Buffer *buf, const Element *e);
+  void writeElement(Buffer *buf, const Element *e, int depth = 0);
 
   void writeEventSizePrefix(Buffer *buf, int start);
 

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

@@ -60,11 +60,10 @@ static void verify_safecopy_range() {
 #endif // DEBUG
 
 #ifdef __APPLE__
+    #define DU3_PREFIX(s, m) __ ## s.__ ## m
     #if defined(__x86_64__)
-      #define DU3_PREFIX(s, m) __ ## s.__ ## m
       #define current_pc uc_mcontext->DU3_PREFIX(ss,rip)
     #elif defined(__aarch64__)
-      #define DU3_PREFIX(s, m) __ ## s.__ ## m
       #define current_pc uc_mcontext->DU3_PREFIX(ss,pc)
     #endif
 #else

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

@@ -0,0 +1,140 @@
+/*
+ * Copyright 2026, Datadog, Inc.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// Regression tests for Recording::countSerializableChildren(), the helper
+// writeElement() uses to decide which children of a JfrMetadata::Element
+// tree get serialized when the tree is corrupted (null children) or
+// unexpectedly deep (cycles / excessive recursion).
+//
+// Recording itself can't be constructed in a plain gtest binary -- its
+// constructor unconditionally calls VM::jvmti()->GetAvailableProcessors(),
+// and writeSettings()/writeOsCpuInfo()/writeJvmInfo() reach into
+// VM::libjvm() and Profiler::instance(), none of which are set up without a
+// live JVM attached. countSerializableChildren() is extracted specifically
+// so the counting/truncation logic writeElement() depends on can be tested
+// directly, without needing any of that.
+//
+// The bug this guards against: originally, child_count was computed by
+// counting every non-null child, while the recursive write skipped children
+// once `depth > 10`. A non-null child at the depth boundary was included in
+// child_count but never serialized, so the encoded count didn't match the
+// number of children actually written -- a structurally invalid JFR
+// metadata event. countSerializableChildren() now applies the exact same
+// depth-truncation rule used to decide whether to recurse, so the count it
+// returns can never diverge from what gets serialized.
+
+#include <gtest/gtest.h>
+#include <memory>
+
+#include "counters.h"
+#include "flightRecorder.h"
+#include "jfrMetadata.h"
+
+// Friend of Recording (see flightRecorder.h), giving this test access to the
+// private countSerializableChildren() helper. Same pattern as
+// VMTestAccessor/ProfilerTestAccessor used elsewhere in this test suite.
+class RecordingTestAccessor {
+public:
+  static size_t countSerializableChildren(
+      const std::vector<const Element *> &children, int depth) {
+    return Recording::countSerializableChildren(children, depth);
+  }
+};
+
+namespace {
+
+// Owns the "field" Elements handed out by makeChild() below, so each test's
+// children are freed when its owner goes out of scope instead of leaking
+// (unlike JfrMetadata::root(), whose tree intentionally lives for the
+// process lifetime, these are throwaway test fixtures).
+class ElementOwner {
+public:
+  const Element *makeChild() {
+    _owned.push_back(std::make_unique<Element>("field"));
+    return _owned.back().get();
+  }
+
+private:
+  std::vector<std::unique_ptr<Element>> _owned;
+};
+
+} // namespace
+
+TEST(WriteElementMetadataGuardTest, AllValidChildrenAreCountedAtShallowDepth) {
+  ElementOwner owner;
+  std::vector<const Element *> children = {owner.makeChild(), owner.makeChild(),
+                                           owner.makeChild()};
+
+  size_t count = RecordingTestAccessor::countSerializableChildren(children, 0);
+
+  EXPECT_EQ(3u, count);
+}
+
+TEST(WriteElementMetadataGuardTest, NullChildrenAreExcludedAndCounted) {
+  ElementOwner owner;
+  std::vector<const Element *> children = {owner.makeChild(), nullptr,
+                                           owner.makeChild(), nullptr};
+
+  long long null_before = Counters::getCounter(METADATA_TREE_NULL_CHILD);
+
+  size_t count = RecordingTestAccessor::countSerializableChildren(children, 0);
+
+  long long null_after = Counters::getCounter(METADATA_TREE_NULL_CHILD);
+
+  // Only the two non-null children are counted -- a naive
+  // children.size()-based count would report 4, which would make the
+  // encoded metadata event advertise two children that are never written.
+  EXPECT_EQ(2u, count);
+  EXPECT_EQ(2, null_after - null_before);
+}
+
+// This is the exact structural-validity bug the depth guard must not
+// reintroduce: a non-null child sitting exactly at the depth boundary
+// (depth 10, so its would-be recursive call is at depth 11) must be
+// excluded from child_count, not just skipped by the recursive writer --
+// otherwise the encoded count and the actually-serialized children diverge.
+TEST(WriteElementMetadataGuardTest, ChildAtDepthBoundaryIsExcludedFromCount) {
+  ElementOwner owner;
+  std::vector<const Element *> children = {owner.makeChild()};
+
+  long long depth_before = Counters::getCounter(METADATA_TREE_DEPTH_EXCEEDED);
+
+  size_t count = RecordingTestAccessor::countSerializableChildren(children, 10);
+
+  long long depth_after = Counters::getCounter(METADATA_TREE_DEPTH_EXCEEDED);
+
+  EXPECT_EQ(0u, count);
+  EXPECT_EQ(1, depth_after - depth_before);
+}
+
+// Sanity check: a child one level shallower than the boundary (depth 9, so
+// its recursive call lands at depth 10, still within the depth > 10 limit)
+// must still be counted normally -- the truncation must not kick in early.
+TEST(WriteElementMetadataGuardTest, ChildJustBeforeDepthBoundaryIsStillCounted) {
+  ElementOwner owner;
+  std::vector<const Element *> children = {owner.makeChild(), owner.makeChild()};
+
+  size_t count = RecordingTestAccessor::countSerializableChildren(children, 9);
+
+  EXPECT_EQ(2u, count);
+}
+
+// A null child at the depth boundary must be reported as a null child, not
+// double-counted against the depth-exceeded counter as well.
+TEST(WriteElementMetadataGuardTest, NullChildAtDepthBoundaryIsReportedAsNull) {
+  std::vector<const Element *> children = {nullptr};
+
+  long long null_before = Counters::getCounter(METADATA_TREE_NULL_CHILD);
+  long long depth_before = Counters::getCounter(METADATA_TREE_DEPTH_EXCEEDED);
+
+  size_t count = RecordingTestAccessor::countSerializableChildren(children, 10);
+
+  long long null_after = Counters::getCounter(METADATA_TREE_NULL_CHILD);
+  long long depth_after = Counters::getCounter(METADATA_TREE_DEPTH_EXCEEDED);
+
+  EXPECT_EQ(0u, count);
+  EXPECT_EQ(1, null_after - null_before);
+  EXPECT_EQ(0, depth_after - depth_before);
+}