open-telemetry / open-telemetry/opentelemetry-cpp
[BUG] Logger::EmitLogRecord() blindly static_casts a caller-supplied LogRecord to Recordable
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 75
Description
Describe your environment
Reproduced by source read + build on main at 11fa0db0 (also present in v1.28.0, the latest release -- not a regression). sdk/src/logs/logger.cc, Logger::EmitLogRecord.
Steps to reproduce
void Logger::EmitLogRecord(
opentelemetry::nostd::unique_ptr<opentelemetry::logs::LogRecord> &&log_record) noexcept
{
...
if (!log_record)
{
return;
}
std::unique_ptr<Recordable> recordable =
std::unique_ptr<Recordable>(static_cast<Recordable *>(log_record.release()));
recordable->SetResource(context_->GetResource());
recordable->SetInstrumentationScope(GetInstrumentationScope());
...
}
EmitLogRecord takes opentelemetry::logs::LogRecord* -- the public API type -- and unconditionally static_casts it to the SDK's internal Recordable* (which does derive from LogRecord, but that doesn't make every LogRecord a Recordable). Logger::MakeRecordable() is a public, overridable entry point, so a caller (or another SDK/wrapper sitting on top of this one) can legitimately hand EmitLogRecord a LogRecord implementation that is not actually a Recordable.
static_cast between unrelated polymorphic types performs no runtime check -- it just reinterprets the pointer. The very next lines (recordable->SetResource(...), SetInstrumentationScope(...)) then call through a vtable/member layout that doesn't match the real object, which is undefined behavior -- in practice, a call through a garbage vtable slot or a write past the real object's actual size.
What is the expected behavior?
If the supplied LogRecord isn't actually a Recordable, EmitLogRecord should drop it (and log why) rather than operate on it as if it were one.
What is the actual behavior?
The cast always "succeeds" (no runtime check), and the type confusion happens silently until something reads or writes through the mismatched layout.
Additional context
Suggested fix -- use dynamic_cast (RTTI is on by default in this project; LogRecord is already polymorphic, so this is a plain runtime-checked downcast) and reject the record if it doesn't actually hold a Recordable. Checking on .get() before releasing means a rejected record is destroyed exactly as before, through the nostd::unique_ptr's own deleter -- no change to how a foreign LogRecord implementation gets torn down on the "not applicable" path:
+#include "opentelemetry/sdk/common/global_log_handler.h"
#include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h"
...
if (!log_record)
{
return;
}
- std::unique_ptr<Recordable> recordable =
- std::unique_ptr<Recordable>(static_cast<Recordable *>(log_record.release()));
+ Recordable *recordable_ptr = dynamic_cast<Recordable *>(log_record.get());
+ if (recordable_ptr == nullptr)
+ {
+ OTEL_INTERNAL_LOG_WARN(
+ "[Logger::EmitLogRecord] Dropping log record: not a Recordable implementation.");
+ return;
+ }
+ log_record.release();
+ std::unique_ptr<Recordable> recordable(recordable_ptr);
recordable->SetResource(context_->GetResource());
recordable->SetInstrumentationScope(GetInstrumentationScope());
The success path is unchanged -- same object, same ownership transfer, same subsequent calls -- so this only affects the case that was previously undefined behavior. Compile-checked against a clean build of this file -- no warnings or errors. Happy to open a PR with this if useful.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in sdk/src/logs/logger.cc at Logger::EmitLogRecord and review how Logger::MakeRecordable and the public LogRecord type interact. Build the file or project, then verify that a non-Recordable LogRecord is safely dropped while the existing Recordable path continues to receive its resource and instrumentation scope.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- observability
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100