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

fix: better handling of invalid headers #132

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Conversation

dmehala
Copy link
Collaborator

@dmehala dmehala commented Jun 17, 2024

Description

When the tracer encounters an invalid propagation header value, logging the list of headers and their values is not handled properly.

Details

For each key and value in the list of headers, a nlohmann::json instance is created with the following code:

stream << nlohmann::json(it->first + ": " + it->second);

It is possible that an input for any of the headers inspected by the tracer exhibits unexpected behavior when the JSON library attempts to parse or dump the input.

When the tracer encounters an invalid propagation header value,
logging the list of headers and their values can lead to a crash.
@dmehala dmehala requested a review from a team as a code owner June 17, 2024 18:15
@dmehala dmehala requested review from pablomartinezbernardo and removed request for a team June 17, 2024 18:15
Copy link
Contributor

@bm1549 bm1549 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include some unit tests that fail for the old behavior, but pass for the new behavior?

@dgoffredo
Copy link
Contributor

dgoffredo commented Jun 17, 2024

Can we include some unit tests that fail for the old behavior, but pass for the new behavior?

nlohmann::json might fail to encode a string? That's surprising.

Anyway, if the issue was due to an exception thrown by the JSON library, then this might help in testing.

edit: Ah, I bet it's UTF related. Consider auditing the library for other "for convenience" uses of nlohmann::json with string input. In the past I've used it as a shorthand for "give me a quoted, escaped string." If arbitrary byte sequences can trigger an exception, though, that's problematic.

@dmehala
Copy link
Collaborator Author

dmehala commented Jun 18, 2024

@dgoffredo Yes, it's a character encoding / serializing issue.

In the past I've used it as a shorthand for "give me a quoted, escaped string."

I was trying to understand what was the purpose, thanks for the context. I will definitely audit for other uses of nlohmann::json.

As always thanks for keeping an eye on dd-trace-cpp David 🙏

@pr-commenter
Copy link

pr-commenter bot commented Jun 18, 2024

Benchmarks

Benchmark execution time: 2024-06-18 08:56:47

Comparing candidate commit d8cd334 in PR branch dmehala/fix-error-handling with baseline commit bb5358e in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

@dmehala dmehala merged commit 5f743f4 into main Jun 18, 2024
22 checks passed
@dmehala dmehala deleted the dmehala/fix-error-handling branch June 18, 2024 10:38
@bm1549 bm1549 changed the title fix: prevent crash due to invalid headers fix: better handling of invalid headers Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants