[BUG] Logger::EmitLogRecord() blindly static_casts a caller-supplied LogRecord to Recordable
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 78/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Active
- Tech stack
- cpp
- Domain
- observability
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.
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.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 78
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.
More from open-telemetry/opentelemetry-cpp
-
triage/accepted
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
open-telemetry/opentelemetry-cpp#4596 · 2 comments · 1 reaction ·
-
bug help wanted triage/accepted
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
open-telemetry/opentelemetry-cpp#4535 · 1 comment · 2 reactions ·
-
[BUG] OnResponse() can call std::terminate() when the response body fails to parse as JSON/protobuf Openbug help wanted triage/accepted
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
open-telemetry/opentelemetry-cpp#4534 · 2 comments · 1 reaction ·
-
needs-triage
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
open-telemetry/opentelemetry-cpp#4347 ·
-
issue:blocked removal spec-compliance Stale triage/accepted
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
open-telemetry/opentelemetry-cpp#3967 · 1 comment · 1 reaction ·
All issues in open-telemetry/opentelemetry-cpp
Similar issues
-
Difficulty 1/5 Under an hour Newbie friendliness 90/100
AXERA-TECH/ax-llm#77 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
-
bug-unconfirmed
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
NVIDIA/cuda-samples#453 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
infiniflow/infinity#3502 ·