Skip to content

Commit 71ca0f2

Browse files
lukeina2zCopilot
andcommitted
[EXPORTER ETW] Fix Span::End() use-after-free when caller drops Tracer
Fixes #3849. etw::TracerProvider::GetTracer() was newing up a fresh Tracer per call and returning the only shared_ptr to it. The catch is that etw::Span stores its parent as a raw reference (Tracer &owner_), so the moment a caller drops or reassigns that shared_ptr, every Span that came from that Tracer is left with a dangling owner_. Span::End() then crashes inside owner_.EndSpan(). The snippet from the issue makes it obvious: auto provider = std::make_unique<etw::TracerProvider>(); auto tracer = provider->GetTracer("Foo"); auto spanFoo = tracer->StartSpan("Span-Foo"); tracer = provider->GetTracer("Bar"); // Tracer "Foo" goes away auto spanBar = tracer->StartSpan("Span-Bar"); spanFoo->End(); // crash spanBar->End(); Under MSVC Debug + cdb this lands as an AV in Span::End dereferencing 0xFEEEFEEE -- the debug heap's freed-memory fill -- which is about as unambiguous as UAF gets. The fix is to let TracerProvider keep the Tracers it hands out, keyed by name and guarded by a mutex, and have GetTracer() return the cached entry on repeat calls. That makes the Tracer outlive any Span derived from it, which is the lifetime contract Span's raw owner_ already assumes. It is also the same pattern sdk::trace::TracerProvider has always used, which is why the issue reporter noted the SDK provider does not crash under the same usage. There was an earlier attempt in PR #4070 that switched Span to take a shared_ptr<Tracer> via shared_from_this(). That was turned down because StartSpan() is the ETW exporter's hot path and nobody wants an extra atomic refcount bump on every span just to keep the parent alive. Doing the lifetime work once in GetTracer() keeps owner_ as a plain reference and costs nothing per span. A few things worth calling out: - No API or ABI change. Only private members are added to TracerProvider, and <map>/<memory>/<mutex> are already included by etw_tracer.h. - The cache deliberately does not evict. Span holds its parent by raw reference, so dropping a Tracer that still has live Spans would put the original bug right back. In practice the ETW exporter is built around "one Tracer per provider name, reused for the lifetime of the process", and Windows itself caps registered ETW providers per process well below anything you would worry about for memory. - Keying on name only is consistent with the existing etw::Tracer constructor, which already declares args and schema_url UNREFERENCED_PARAMETER. - etw::LoggerProvider was checked for the same shape and does not have it -- Logger holds a back-reference to the Provider (long-lived), not to itself -- so no equivalent change is needed there. Repro before the change: AV in Span::End with the freed-fill operand. Same repro after: runs cleanly to completion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 72953ea commit 71ca0f2

1 file changed

Lines changed: 38 additions & 3 deletions

File tree

  • exporters/etw/include/opentelemetry/exporters/etw

exporters/etw/include/opentelemetry/exporters/etw/etw_tracer.h

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1152,6 +1152,31 @@ class TracerProvider : public opentelemetry::trace::TracerProvider
11521152
config_.encoding = ETWProvider::EventFormat::ETW_MANIFEST;
11531153
}
11541154

1155+
/**
1156+
* @brief Cache of Tracers handed out by GetTracer(), keyed by name.
1157+
*
1158+
* The ETW Span class stores its parent Tracer as a raw reference
1159+
* (Tracer &owner_); if the only strong refcount on the Tracer is the
1160+
* nostd::shared_ptr returned by GetTracer(), the Tracer is destroyed
1161+
* as soon as the caller drops it, leaving every Span derived from it
1162+
* with a dangling owner_ that crashes on End(). Cache here so the
1163+
* provider keeps the Tracer alive as long as it is alive itself,
1164+
* matching the lifetime contract used by sdk::trace::TracerProvider.
1165+
*
1166+
* Growth: this cache is unbounded by design and mirrors
1167+
* sdk::trace::TracerProvider's tracers_ vector. Eviction is unsafe
1168+
* because the ETW Span stores its parent Tracer by raw reference, so
1169+
* dropping a Tracer that still has outstanding Spans would
1170+
* re-introduce the use-after-free this cache exists to prevent.
1171+
*
1172+
* In practice the cache stays small because the ETW exporter is
1173+
* designed around "one Tracer per ETW provider name, reused for the
1174+
* process/component lifetime" -- and Windows itself caps the number
1175+
* of registered ETW providers per process well below any RAM concern.
1176+
*/
1177+
std::map<std::string, std::shared_ptr<opentelemetry::trace::Tracer>> tracers_;
1178+
std::mutex tracers_lock_;
1179+
11551180
/**
11561181
* @brief Obtain ETW Tracer.
11571182
* @param name ProviderId (instrumentation name) - Name or GUID
@@ -1177,9 +1202,19 @@ class TracerProvider : public opentelemetry::trace::TracerProvider
11771202
UNREFERENCED_PARAMETER(args);
11781203
UNREFERENCED_PARAMETER(schema_url);
11791204
ETWProvider::EventFormat evtFmt = config_.encoding;
1180-
std::shared_ptr<opentelemetry::trace::Tracer> tracer{new (std::nothrow)
1181-
Tracer(*this, name, evtFmt)};
1182-
return nostd::shared_ptr<opentelemetry::trace::Tracer>{tracer};
1205+
// Cache Tracers by name. The exporter Span class holds a raw
1206+
// Tracer& as owner_, so the Tracer must live at least as long as
1207+
// the longest-lived Span derived from it. Without caching, each
1208+
// GetTracer() call mints a fresh heap Tracer that is destroyed the
1209+
// moment the caller drops the returned shared_ptr, orphaning any
1210+
// outstanding Spans (use-after-free in Span::End()).
1211+
std::lock_guard<std::mutex> lock(tracers_lock_);
1212+
auto &cached = tracers_[std::string{name.data(), name.size()}];
1213+
if (!cached)
1214+
{
1215+
cached.reset(new (std::nothrow) Tracer(*this, name, evtFmt));
1216+
}
1217+
return nostd::shared_ptr<opentelemetry::trace::Tracer>{cached};
11831218
}
11841219
};
11851220

0 commit comments

Comments
 (0)