diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index deb6e40d9..a3b3ea34f 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/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) diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 6064e594c..7d096dc6c 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/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 &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); } diff --git a/ddprof-lib/src/main/cpp/flightRecorder.h b/ddprof-lib/src/main/cpp/flightRecorder.h index ee6929b4f..f7319f55a 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.h +++ b/ddprof-lib/src/main/cpp/flightRecorder.h @@ -174,6 +174,11 @@ 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; @@ -181,6 +186,14 @@ class Recording { 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 &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); diff --git a/ddprof-lib/src/main/cpp/safeAccess.cpp b/ddprof-lib/src/main/cpp/safeAccess.cpp index 5738c0f4a..4bcf596b1 100644 --- a/ddprof-lib/src/main/cpp/safeAccess.cpp +++ b/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 diff --git a/ddprof-lib/src/test/cpp/flightRecorder_metadata_ut.cpp b/ddprof-lib/src/test/cpp/flightRecorder_metadata_ut.cpp new file mode 100644 index 000000000..c1c6258b2 --- /dev/null +++ b/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 +#include + +#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 &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("field")); + return _owned.back().get(); + } + +private: + std::vector> _owned; +}; + +} // namespace + +TEST(WriteElementMetadataGuardTest, AllValidChildrenAreCountedAtShallowDepth) { + ElementOwner owner; + std::vector 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 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 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 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 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); +}