Skip to content

Improve validation checking/error handling process context C/C++ impl#88

Merged
felixge merged 5 commits into
open-telemetry:mainfrom
ivoanjo:ivoanjo/ref-small-improvements
Mar 31, 2026
Merged

Improve validation checking/error handling process context C/C++ impl#88
felixge merged 5 commits into
open-telemetry:mainfrom
ivoanjo:ivoanjo/ref-small-improvements

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR collects a few more improvements to the process context C/C++ impl:

  • Add a few more size checks that were missing to make sure we stay within the limits of the embedded protobuf encoder
  • Fix for leaking the memfd file descriptor in a weird corner case
  • Make sure the no-op implementation does not need any Linux-specific headers

Motivation:

These came up as we were adding process context support to Datadog's dd-trace-java profiler in DataDog/java-profiler#414 .

Additional Notes:

I've considered if the encoder should follow the design of the decoder and always check the position of the pointer... It'd be a bigger change so I hesitated in going for it.

How to test the change?

The no-op implementation build validates the minimal set of headers is correct; for the other changes the build still passes and the example script works fine.

ivoanjo added 4 commits March 18, 2026 11:16
In practice I'm not sure `ftruncate` could fail unless the kernel is
already on fire but no reason not to fix the leak anyway.
We were accidentally importing at least one Linux-specific header,
which made NOOP not work.

To make sure NOOP keeps working, let's move everything it doesn't need
inside the ifdef -- this way it'll be very obvious what's being compiled
for NOOP and what's not.
@ivoanjo ivoanjo requested a review from a team as a code owner March 18, 2026 15:10
@felixge felixge enabled auto-merge (squash) March 31, 2026 09:26
@felixge felixge merged commit 2d5622a into open-telemetry:main Mar 31, 2026
1 check passed
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.

3 participants