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

[pkg/stanza] Pass buffer by reference so buffer changes are not lost #36252

Merged

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented Nov 7, 2024

The root of the crash reported in #36179 was the fact that the Buffer struct being passed by value in recursive calls made it allocate the needed amount, but, after the return of the recursive call it attempted to read a buffer that was larger than the allocated buffer on the recursive call.

The crash was going to be hit whenever an XML was larger than the default size of the buffer (16KiB).

The code is a bit hard to test because its fully usage actually happens on the receiver where the buffer and its components are not visible. I'll look to add a test in a follow-up.

cc @djaglowski

Skipping changelog since #36179 covers it.

@pjanotti pjanotti requested review from djaglowski and a team as code owners November 7, 2024 04:57
@pjanotti pjanotti added Run Windows Enable running windows test on a PR Skip Changelog PRs that do not require a CHANGELOG.md entry labels Nov 7, 2024
@dmitryax dmitryax changed the title Pass buffer by reference so buffer changes are not lost [pkg/stanza] Pass buffer by reference so buffer changes are not lost Nov 8, 2024
@dmitryax dmitryax merged commit 234b5a7 into open-telemetry:main Nov 8, 2024
173 of 174 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 8, 2024
@pjanotti pjanotti deleted the pass-buffer-by-ref-on-stanza-windows branch November 8, 2024 22:00
pull bot pushed a commit to abaguas/opentelemetry-collector-contrib that referenced this pull request Nov 9, 2024
…pen-telemetry#36252)

The root of the crash reported in open-telemetry#36179 was the fact that the `Buffer`
struct being passed by value in recursive calls made it allocate the
needed amount, but, after the return of the recursive call it attempted
to read a buffer that was larger than the allocated buffer on the
recursive call.

The crash was going to be hit whenever an XML was larger than the
default size of the buffer (16KiB).

The code is a bit hard to test because its fully usage actually happens
on the receiver where the buffer and its components are not visible.
I'll look to add a test in a follow-up.

cc @djaglowski 

Skipping changelog since open-telemetry#36179 covers it.

---------

Co-authored-by: Daniel Jaglowski <[email protected]>
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…pen-telemetry#36252)

The root of the crash reported in open-telemetry#36179 was the fact that the `Buffer`
struct being passed by value in recursive calls made it allocate the
needed amount, but, after the return of the recursive call it attempted
to read a buffer that was larger than the allocated buffer on the
recursive call.

The crash was going to be hit whenever an XML was larger than the
default size of the buffer (16KiB).

The code is a bit hard to test because its fully usage actually happens
on the receiver where the buffer and its components are not visible.
I'll look to add a test in a follow-up.

cc @djaglowski 

Skipping changelog since open-telemetry#36179 covers it.

---------

Co-authored-by: Daniel Jaglowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/stanza Run Windows Enable running windows test on a PR Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants