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

[brhenc]

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.

[owent]

In SDK. Only factory classes and functions in them are public for users, other function and objects are only for internal usage. We must support to be built without RTTI, so we can not use dynamic_cast in our codes.

[mateenali66]

The RTTI point holds, there is no dynamic_cast in sdk/src or sdk/include and OPENTELEMETRY_RTTI_ENABLED is test-only today. But the SDK produces a non-Recordable itself, so this narrows to a path with no caller-supplied LogRecord in it.

A disabled logger returns kNoopLogger.CreateLogRecord() (logger.cc:137), which is a NoopLogRecord. EmitLogRecord only takes its noop branch while still disabled (:185). Enable the logger in between through LoggerProvider::UpdateLoggerConfigurator and the record reaches the static_cast at :194. The if (!log_record) guard at :188 caught this while the noop path returned nullptr, which #2668 changed.

Reproduced on 731099b under -fsanitize=undefined, bus error in the vtable lookup on the SetResource call at :195. Test available if useful.

No RTTI needed to fix it. Should the disabled path stop handing out a noop record, or should EmitLogRecord be able to recognise a record the SDK did not make?

Suggest triage/accepted.

[owent]

The CI job "Bazel nortti"(bazel_nortti) tests the building without RTTI. I think maybe we need another way to check if Logger emit a LogRecord not created by it self. Maybe we can add the logger's addres into LogRecord and just ignored all the wrong LogRecord.

[mateenali66]

bazel_nortti settles it, so RTTI is out.

The address tag does not reach this path though. The record that arrives is NoopLogger::NoopLogRecord (api/include/opentelemetry/logs/noop.h:58-72), handed back by the SDK's own CreateLogRecord() at logger.cc:137 and :163 while the logger is disabled. It has no data members and it lives in the API, so giving it a logger address means adding storage to the API LogRecord.

The check cannot live in the SDK either. EmitLogRecord gets an opentelemetry::logs::LogRecord * and Recordable is an SDK type, so with no RTTI nothing SDK-side can interrogate that pointer.

Two shapes:

1. A virtual on the API LogRecord that sdk::logs::Recordable overrides. Small, but it changes the vtable of an API class, so ABI v2 preview rather than v1.
2. Have CreateLogRecord() return a real recordable even when disabled, and check enabled state only at emit time. No API or ABI change, costs a MakeRecordable() on a disabled logger. It needs no detection either, since the only non-Recordable left is then a caller-constructed one, which you ruled out of scope.

Either way the discard should log rather than be silent. At emit time the logger is enabled and the caller expects the record to go out.

Which do you want? Happy to prototype either and measure the cost of 2.

[owent]

bazel_nortti settles it, so RTTI is out.

The address tag does not reach this path though. The record that arrives is NoopLogger::NoopLogRecord (api/include/opentelemetry/logs/noop.h:58-72), handed back by the SDK's own CreateLogRecord() at logger.cc:137 and :163 while the logger is disabled. It has no data members and it lives in the API, so giving it a logger address means adding storage to the API LogRecord.

The check cannot live in the SDK either. EmitLogRecord gets an opentelemetry::logs::LogRecord * and Recordable is an SDK type, so with no RTTI nothing SDK-side can interrogate that pointer.

Two shapes:

1. A virtual on the API LogRecord that sdk::logs::Recordable overrides. Small, but it changes the vtable of an API class, so ABI v2 preview rather than v1.
2. Have CreateLogRecord() return a real recordable even when disabled, and check enabled state only at emit time. No API or ABI change, costs a MakeRecordable() on a disabled logger. It needs no detection either, since the only non-Recordable left is then a caller-constructed one, which you ruled out of scope.

Either way the discard should log rather than be silent. At emit time the logger is enabled and the caller expects the record to go out.

Which do you want? Happy to prototype either and measure the cost of 2.

I prefer 2 personally. We can implement a special NoopLogger for sdk, which will returns a real recordable and leave all Set* APIs to do nothing.It wouldn't break ABI of opentelemetry::logs::LogRecord.

[mateenali66]

2 works, with one catch. LoggerContext always wraps the processors in MultiLogRecordProcessor (logger_context.cc:33), whose OnEmit does static_cast<MultiRecordable *> (multi_log_record_processor.cc:65). So a new noop record type is still UB there if the logger is enabled between create and emit.

An empty MultiRecordable avoids that. Its Set* loop over no children and OnEmit forwards nothing.

return opentelemetry::nostd::unique_ptr<opentelemetry::logs::LogRecord>(new MultiRecordable());

Tried it on 2e99940c under UBSan. Main crashes on the repro, patched drops the record, enabled records still reach the processor, and the logs tests pass. The v2 overload at logger.cc:163 needs the same change.

Want a PR?

[owent]

2 works, with one catch. LoggerContext always wraps the processors in MultiLogRecordProcessor (logger_context.cc:33), whose OnEmit does static_cast<MultiRecordable *> (multi_log_record_processor.cc:65). So a new noop record type is still UB there if the logger is enabled between create and emit.

An empty MultiRecordable avoids that. Its Set* loop over no children and OnEmit forwards nothing.

return opentelemetry::nostd::unique_ptropentelemetry::logs::LogRecord(new MultiRecordable());
Tried it on 2e99940c under UBSan. Main crashes on the repro, patched drops the record, enabled records still reach the processor, and the logs tests pass. The v2 overload at logger.cc:163 needs the same change.

Want a PR?

You are right.How about adding a virtual method for Recordable in SDK? That can get the processor which the Recordable is created by? Just ignore all the mismatch OnEmit calling?