Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OStreamLogRecordExporter doesn't copy message data and attributes #2402

Open
lalitb opened this issue Nov 13, 2023 · 4 comments
Open

OStreamLogRecordExporter doesn't copy message data and attributes #2402

lalitb opened this issue Nov 13, 2023 · 4 comments
Assignees
Labels
bug Something isn't working do-not-stale

Comments

@lalitb
Copy link
Member

lalitb commented Nov 13, 2023

OStreamLogExporter uses ReadWriteLogRecord as the recordable implementation, and this implementation doesn't maintain copy of message body and attributes:

std::unordered_map<std::string, opentelemetry::common::AttributeValue> attributes_map_;
opentelemetry::common::AttributeValue body_;

This can result in the use-after-free crash in case the message body or attributes are cleaned-up before the data is processed by batch processor as in below:

#include "opentelemetry/logs/provider.h"
#include "opentelemetry/sdk/logs/batch_log_record_processor.h"
#include "opentelemetry/sdk/logs/logger_provider.h"

#include "opentelemetry/exporters/ostream/log_record_exporter.h"
#include "opentelemetry/sdk/logs/batch_log_record_processor_factory.h"
#include "opentelemetry/sdk/logs/logger_provider_factory.h"
#include "opentelemetry/sdk/logs/processor.h"

namespace logs_api = opentelemetry::logs;
namespace logs_sdk = opentelemetry::sdk::logs;
namespace logs_exporter = opentelemetry::exporter::logs;

using namespace std;
namespace nostd = opentelemetry::nostd;

int
main(int argc, char** argv)
{
    logs_sdk::BatchLogRecordProcessorOptions batch_opts {};

    auto os_exporter = unique_ptr<logs_sdk::LogRecordExporter>(
        new opentelemetry::exporter::logs::OStreamLogRecordExporter);
    auto os_processor =
        logs_sdk::BatchLogRecordProcessorFactory::Create(move(os_exporter), batch_opts);

    auto provider =
        shared_ptr<logs_api::LoggerProvider>(new logs_sdk::LoggerProvider(move(os_processor)));

    // Set the global logger provider
    logs_api::Provider::SetLoggerProvider(provider);

    auto logger = logs_api::Provider::GetLoggerProvider()->GetLogger("anything");

    string* sp = new string("a message "s + std::to_string(random()) + " is being created "s +
                            std::to_string(random()));
    logger->Info(*sp);
    delete sp;

    sp = new string("a message "s + std::to_string(random()));
    logger->Info(*sp);
    *sp = "Now it is another message entirely";
    delete sp;

    sleep(10);
}

Just like SpanData implementation which is used in OStream span exporter, the ReadWriteLogRecord should also maintain the copy of these data.

@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 13, 2023
@ThomsonTan
Copy link
Contributor

Another thought, should we limit the usage of OStreamLogRecordExporter in async pipeline as the BatchLogRecordProcessor? So we don't need to make copies of the attributes for most common cases.

@lalitb
Copy link
Member Author

lalitb commented Nov 13, 2023

The OStreamLogRecordExporter serves as a demonstration model to implement exporters for logs, and it should do it in right way. And also, good to have parity with OStreamSpanExporter which can be used both with Simple and Batch Processor. SpanData in that case maintains the copy of attributes as shared in link above.

@lalitb lalitb changed the title OStreamLogExporter doesn't copy message data and attributes OStreamLogRecordExporter doesn't copy message data and attributes Nov 13, 2023
@marcalff marcalff added bug Something isn't working and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 15, 2023
@marcalff marcalff assigned marcalff and ThomsonTan and unassigned marcalff Nov 15, 2023
Copy link

This issue was marked as stale due to lack of activity.

@ThomsonTan
Copy link
Contributor

Making copy could make sense because the OStream exporter is mostly for debugging purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working do-not-stale
Projects
None yet
Development

No branches or pull requests

3 participants