open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] Logger::EmitLogRecord() blindly static_casts a caller-supplied LogRecord to Recordable

Open Beginner friendly
#4,537 7 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug triage/accepted
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.