[CRASH] Segmentation fault in LogRecordSetterTrait<EventId> for events without a name

[sjinks]

Describe your environment opentelemetry-cpp v1.18.0 (I found this bug in v1.11.0)

Steps to reproduce

#include <memory>
#include <utility>

#include <opentelemetry/exporters/ostream/log_record_exporter.h>
#include <opentelemetry/logs/event_id.h>
#include <opentelemetry/logs/logger_provider.h>
#include <opentelemetry/logs/provider.h>
#include <opentelemetry/sdk/logs/exporter.h>
#include <opentelemetry/sdk/logs/logger_provider_factory.h>
#include <opentelemetry/sdk/logs/simple_log_record_processor_factory.h>

int main()
{
    auto exporter = std::unique_ptr<opentelemetry::sdk::logs::LogRecordExporter>(
        new opentelemetry::exporter::logs::OStreamLogRecordExporter
    );

    auto processor = opentelemetry::sdk::logs::SimpleLogRecordProcessorFactory::Create(std::move(exporter));

    std::shared_ptr<opentelemetry::sdk::logs::LoggerProvider> sdk_provider(
        opentelemetry::sdk::logs::LoggerProviderFactory::Create(std::move(processor))
    );

    const std::shared_ptr<opentelemetry::logs::LoggerProvider> &api_provider = sdk_provider;
    opentelemetry::logs::Provider::SetLoggerProvider(api_provider);

    const opentelemetry::logs::EventId event(123);

    auto logger = opentelemetry::logs::Provider::GetLoggerProvider()->GetLogger("example");
    logger->Info(event, "Hello!");

    return 0;
}

What is the expected behavior?
No crash

What is the actual behavior?
Segmentation fault

==227163==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x78818718b75d bp 0x7fff85666470 sp 0x7fff85665c28 T0)
==227163==The signal is caused by a READ memory access.
==227163==Hint: address points to the zero page.
    #0 0x78818718b75d in __strlen_avx2 string/../sysdeps/x86_64/multiarch/strlen-avx2.S:76
    #1 0x5ca9ba53df27 in strlen (/home/volodymyr/work/github/otel-heap-use-after-free/build/bug-repro+0x56f27) (BuildId: 3d0c8da11893a8ba14a9592b3606f7c585b371f5)
    #2 0x5ca9ba60f320 in opentelemetry::v1::nostd::string_view::string_view(char const*) /home/volodymyr/work/github/otel-heap-use-after-free/build/vcpkg_installed/x64-linux/include/opentelemetry/nostd/string_view.h:46:51
    #3 0x5ca9ba613371 in opentelemetry::v1::logs::LogRecord* opentelemetry::v1::logs::detail::LogRecordSetterTrait<opentelemetry::v1::logs::EventId>::Set<opentelemetry::v1::logs::EventId const&>(opentelemetry::v1::logs::LogRecord*, opentelemetry::v1::logs::EventId const&) /home/volodymyr/work/github/otel-heap-use-after-free/build/vcpkg_installed/x64-linux/include/opentelemetry/logs/logger_type_traits.h:50:37
    #4 0x5ca9ba619f57 in void opentelemetry::v1::logs::Logger::EmitLogRecord<opentelemetry::v1::logs::Severity, opentelemetry::v1::logs::EventId const&, char const (&) [7]>(opentelemetry::v1::nostd::unique_ptr<opentelemetry::v1::logs::LogRecord>&&, opentelemetry::v1::logs::Severity&&, opentelemetry::v1::logs::EventId const&, char const (&) [7]) /home/volodymyr/work/github/otel-heap-use-after-free/build/vcpkg_installed/x64-linux/include/opentelemetry/logs/logger.h:76:9
    #5 0x5ca9ba619d18 in void opentelemetry::v1::logs::Logger::EmitLogRecord<opentelemetry::v1::logs::Severity, opentelemetry::v1::logs::EventId const&, char const (&) [7]>(opentelemetry::v1::logs::Severity&&, opentelemetry::v1::logs::EventId const&, char const (&) [7]) /home/volodymyr/work/github/otel-heap-use-after-free/build/vcpkg_installed/x64-linux/include/opentelemetry/logs/logger.h:104:5
    #6 0x5ca9ba60fb0b in void opentelemetry::v1::logs::Logger::Info<opentelemetry::v1::logs::EventId const&, char const (&) [7]>(opentelemetry::v1::logs::EventId const&, char const (&) [7]) /home/volodymyr/work/github/otel-heap-use-after-free/build/vcpkg_installed/x64-linux/include/opentelemetry/logs/logger.h:176:11
    #7 0x5ca9ba60eff4 in main /home/volodymyr/work/github/otel-heap-use-after-free/repro.cpp:30:13
    #8 0x78818702a1c9 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #9 0x78818702a28a in __libc_start_main csu/../csu/libc-start.c:360:3
    #10 0x5ca9ba524a74 in _start (/home/volodymyr/work/github/otel-heap-use-after-free/build/bug-repro+0x3da74) (BuildId: 3d0c8da11893a8ba14a9592b3606f7c585b371f5)

==227163==Register values:
rax = 0x0000000000000000  rbx = 0x0000000085666401  rcx = 0xf3f30000f1f1f1f1  rdx = 0x0000000000000000  
rdi = 0x0000000000000000  rsi = 0x0000000000000000  rbp = 0x00007fff85666470  rsp = 0x00007fff85665c28  
 r8 = 0x0000000000000000   r9 = 0x00007fffffffff01  r10 = 0x0000000000001f01  r11 = 0xe2e30db1468c3f01  
r12 = 0x0000000000000001  r13 = 0x0000000000000000  r14 = 0x0000000000000000  r15 = 0x00007881877eb000  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV string/../sysdeps/x86_64/multiarch/strlen-avx2.S:76 in __strlen_avx2
==227163==ABORTING

Additional context

1. An EventId is created with an empty name_: EventId(int64_t id) noexcept : id_{id}, name_{nullptr} {} (ref)
2. The EventId is passed to Logger::Info(), then to EmitLogRecord().
3. LogRecordSetterTrait<EventId> is invoked to set the Event ID for the LogRecord: log_record->SetEventId(arg.id_, nostd::string_view{arg.name_.get()}); (ref)
4. Because EventId::name_ holds a nullptr value, nostd::string_view() is constructed with nullptr.
5. nostd::string_view::string_view(const char*) gets invoked, and it calls std::strlen() on a nullptr, crashing the application.

[sjinks]

For this particular bug, the fix could be

diff --git a/api/include/opentelemetry/logs/logger_type_traits.h b/api/include/opentelemetry/logs/logger_type_traits.h
index d88a6ffb..83a78c4d 100644
--- a/api/include/opentelemetry/logs/logger_type_traits.h
+++ b/api/include/opentelemetry/logs/logger_type_traits.h
@@ -47,7 +47,7 @@ struct LogRecordSetterTrait<EventId>
   template <class ArgumentType>
   inline static LogRecord *Set(LogRecord *log_record, ArgumentType &&arg) noexcept
   {
-    log_record->SetEventId(arg.id_, nostd::string_view{arg.name_.get()});
+    log_record->SetEventId(arg.id_, nostd::string_view{arg.name_ ? arg.name_.get() : ""});
 
     return log_record;
   }

However, it would be interesting to try to find other places where a similar bug is present.

[sjinks]

Another possible bug: https://github.com/sjinks/opentelemetry-cpp/blob/f1b5fbdedd5beb7e7ca6aacfb3f430686d694d9c/exporters/etw/include/opentelemetry/exporters/etw/etw_properties.h#L303-L304

        const char *data = nostd::get<const char *>(*this);
        return nostd::string_view(data, (data) ? strlen(data) : 0);

In C++17, when std::string_view is used as nonstd::string_view, it is undefined behavior to construct a string_view from nullptr.

Maybe

-        return nostd::string_view(data, (data) ? strlen(data) : 0);
+        return data ? nostd::string_view(data, strlen(data)) : nostd::string_view();

[owent]

For this particular bug, the fix could be

diff --git a/api/include/opentelemetry/logs/logger_type_traits.h b/api/include/opentelemetry/logs/logger_type_traits.h
index d88a6ffb..83a78c4d 100644
--- a/api/include/opentelemetry/logs/logger_type_traits.h
+++ b/api/include/opentelemetry/logs/logger_type_traits.h
@@ -47,7 +47,7 @@ struct LogRecordSetterTrait<EventId>
   template <class ArgumentType>
   inline static LogRecord *Set(LogRecord *log_record, ArgumentType &&arg) noexcept
   {
-    log_record->SetEventId(arg.id_, nostd::string_view{arg.name_.get()});
+    log_record->SetEventId(arg.id_, nostd::string_view{arg.name_ ? arg.name_.get() : ""});
 
     return log_record;
   }

However, it would be interesting to try to find other places where a similar bug is present.

Thanks, @ThomsonTan I'm not sure about the specification of event id. could you please check if it's OK to let event name be empty or set a default name?

According to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md#eventlogger and https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/events.md#event-definition .

In my understanding, a event only has event.name attribute, event.id and event.domain are removed in the latest specification.