From 008d6889e4f9acc4be699bb2ac845f3396e20528 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Tue, 28 Jul 2026 13:02:09 +0200 Subject: [PATCH 1/2] Fix memory leak in JfrMetadata::reset() reset() cleared _root._children/_attributes without deleting the heap-allocated Element/NoField objects initialize() had allocated. Only reachable via a Profiler restart (reset() -> initialize() a second time in the same process); unrelated to PROF-15075, which crashes on a plain chunk dump with no restart involved (split out of #692 per https://datadoghq.atlassian.net/browse/PROF-15075). --- ddprof-lib/src/main/cpp/jfrMetadata.cpp | 20 +++++ ddprof-lib/src/main/cpp/jfrMetadata.h | 13 ++- ddprof-lib/src/test/cpp/jfrMetadata_ut.cpp | 98 ++++++++++++++++++++++ 3 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 ddprof-lib/src/test/cpp/jfrMetadata_ut.cpp diff --git a/ddprof-lib/src/main/cpp/jfrMetadata.cpp b/ddprof-lib/src/main/cpp/jfrMetadata.cpp index 33496bb1bc..4064ba241d 100644 --- a/ddprof-lib/src/main/cpp/jfrMetadata.cpp +++ b/ddprof-lib/src/main/cpp/jfrMetadata.cpp @@ -24,12 +24,32 @@ std::vector Element::_strings; JfrMetadata JfrMetadata::_root; bool JfrMetadata::_initialized = false; +std::vector JfrMetadata::_nofields; JfrMetadata::JfrMetadata() : Element("root") {} +// Private helper: recursively delete all heap-allocated Elements in the subtree. +// Safe to call only when no profiling engines are running (called from reset()). +static void deleteElementTree(Element *e) { + if (e == nullptr) return; + for (const Element *child : e->_children) { + deleteElementTree(const_cast(child)); + } + delete e; +} + // Must only be called after all profiler engines are stopped and no signal // handlers can fire. std::vector/std::map are not async-signal-safe. void JfrMetadata::reset() { + // Recursively delete all heap-allocated Elements in the tree before clearing vectors. + for (const Element *child : _root._children) { + deleteElementTree(const_cast(child)); + } + // Delete all tracked NoField instances that were allocated during initialize() + for (NoField *nf : _nofields) { + delete nf; + } + _nofields.clear(); _root._children.clear(); _root._attributes.clear(); _strings.clear(); diff --git a/ddprof-lib/src/main/cpp/jfrMetadata.h b/ddprof-lib/src/main/cpp/jfrMetadata.h index ac241a7a84..7af59c2205 100644 --- a/ddprof-lib/src/main/cpp/jfrMetadata.h +++ b/ddprof-lib/src/main/cpp/jfrMetadata.h @@ -165,6 +165,8 @@ class JfrMetadata : Element { private: static JfrMetadata _root; static bool _initialized; + // Track NoField instances allocated during initialize() for cleanup in reset() + static std::vector _nofields; enum FieldFlags { F_CPOOL = 0x1, @@ -204,7 +206,9 @@ class JfrMetadata : Element { const char *label = NULL, int flags = 0, bool condition = true) { if (!condition) { - return *new NoField(name); + NoField *nf = new NoField(name); + _nofields.push_back(nf); // Track for cleanup in reset() + return *nf; } Element &e = element("field"); e.attribute("name", name); @@ -261,7 +265,14 @@ class JfrMetadata : Element { public: JfrMetadata(); + // Initialize the JFR metadata tree with standard types and optional context attributes. + // PRECONDITION: Must be called with Profiler::_state_lock held. + // reset() must be called before each initialize() to clean up the prior tree. static void initialize(const std::vector &contextAttributes); + + // Reset and deallocate the JFR metadata tree. + // PRECONDITION: Must be called with Profiler::_state_lock held. + // Must be called before any signal handlers can fire (all profiling engines stopped). static void reset(); static Element *root() { return &_root; } diff --git a/ddprof-lib/src/test/cpp/jfrMetadata_ut.cpp b/ddprof-lib/src/test/cpp/jfrMetadata_ut.cpp new file mode 100644 index 0000000000..9f4edcbe1e --- /dev/null +++ b/ddprof-lib/src/test/cpp/jfrMetadata_ut.cpp @@ -0,0 +1,98 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +// Regression tests for PROF-15075 (SIGSEGV in Recording::writeElement). +// +// JfrMetadata::reset() used to clear _root._children (and the tracked +// NoField instances) without deleting the underlying heap-allocated Element +// objects that JfrMetadata::initialize() had allocated via element(), +// operator||(), and the conditional NoField path in field(). Once the +// allocator reused a freed address on the next initialize() call, any +// dangling pointer left over from before would point at unrelated memory. +// +// These tests exercise JfrMetadata::initialize()/reset() directly (no JVM +// attach required -- initialize() only touches VM::isHotspot()/ +// VM::hotspot_version(), which default to false/-1 in this test binary, +// so the conditional NoField path in field() is deterministically taken). +// Running this test under an ASan/LeakSanitizer build (testAsan) is what +// actually proves the fix: a pre-fix build leaks every Element and NoField +// allocated by initialize() on every reset(), and LeakSanitizer reports it +// at process exit. + +#include "jfrMetadata.h" + +#include +#include +#include + +TEST(JfrMetadataResetTest, ResetIsSafeBeforeAnyInitialize) { + // reset() must be safe to call even if initialize() was never called + // (e.g. Profiler::stop() racing a failed Profiler::start()). + JfrMetadata::reset(); + EXPECT_TRUE(JfrMetadata::root()->_children.empty()); + // reset() re-registers "root" at string id 0 so _root._name stays valid, + // so strings() always contains exactly that one entry, never empty. + EXPECT_EQ(JfrMetadata::strings().size(), 1u); + EXPECT_EQ(JfrMetadata::strings()[0], "root"); +} + +TEST(JfrMetadataResetTest, InitializeThenResetClearsTree) { + JfrMetadata::reset(); + JfrMetadata::initialize({}); + + EXPECT_FALSE(JfrMetadata::root()->_children.empty()); + EXPECT_FALSE(JfrMetadata::strings().empty()); + + JfrMetadata::reset(); + + EXPECT_TRUE(JfrMetadata::root()->_children.empty()); + // See ResetIsSafeBeforeAnyInitialize: "root" is always re-registered. + EXPECT_EQ(JfrMetadata::strings().size(), 1u); + EXPECT_EQ(JfrMetadata::strings()[0], "root"); +} + +TEST(JfrMetadataResetTest, MultipleInitializeResetCyclesDoNotCrash) { + // Simulates repeated Profiler::start()/stop() restart cycles. Each + // initialize() allocates a fresh Element/NoField tree; each reset() must + // fully delete the previous cycle's tree before the next initialize() + // reuses the freed heap addresses. + for (int i = 0; i < 5; i++) { + JfrMetadata::reset(); + JfrMetadata::initialize({}); + EXPECT_FALSE(JfrMetadata::root()->_children.empty()) + << "cycle " << i << " did not populate the metadata tree"; + } + JfrMetadata::reset(); + EXPECT_TRUE(JfrMetadata::root()->_children.empty()); +} + +TEST(JfrMetadataResetTest, RestartCyclesWithContextAttributesDoNotCrash) { + // Non-empty contextAttributes exercise Element::operator||(), which + // allocates one "field" Element per attribute name; those instances must + // also be reachable (and deleted) via reset()'s recursive tree cleanup. + std::vector contextAttributes = {"tag1", "tag2", "tag3"}; + for (int i = 0; i < 5; i++) { + JfrMetadata::reset(); + JfrMetadata::initialize(contextAttributes); + EXPECT_FALSE(JfrMetadata::root()->_children.empty()) + << "cycle " << i << " did not populate the metadata tree"; + } + JfrMetadata::reset(); +} + +TEST(JfrMetadataResetTest, InitializeIsIdempotentWithoutReset) { + // JfrMetadata::initialize() guards against double-initialization; calling + // it twice without an intervening reset() must not double-allocate or + // crash. This documents/protects the existing `if (_initialized) return;` + // safeguard called out in the PROF-15075 spec. + JfrMetadata::reset(); + JfrMetadata::initialize({}); + size_t childrenAfterFirst = JfrMetadata::root()->_children.size(); + + JfrMetadata::initialize({}); // no-op: _initialized guard short-circuits + EXPECT_EQ(childrenAfterFirst, JfrMetadata::root()->_children.size()); + + JfrMetadata::reset(); +} From 3ac838f7eab2fdbc275913359a86e2b9eebb3547 Mon Sep 17 00:00:00 2001 From: Jaroslav Bachorik Date: Thu, 6 Aug 2026 13:07:07 +0200 Subject: [PATCH 2/2] Add virtual destructor to Element to avoid UB on polymorphic delete --- ddprof-lib/src/main/cpp/jfrMetadata.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ddprof-lib/src/main/cpp/jfrMetadata.h b/ddprof-lib/src/main/cpp/jfrMetadata.h index 7af59c2205..4d3f2a86e9 100644 --- a/ddprof-lib/src/main/cpp/jfrMetadata.h +++ b/ddprof-lib/src/main/cpp/jfrMetadata.h @@ -122,6 +122,8 @@ class Element { Element(const char *name) : _name(getId(name)), _attributes(), _children() {} + virtual ~Element() = default; + Element &attribute(const char *key, const char *value) { _attributes.push_back(Attribute(getId(key), getId(value))); return *this;