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 request header capture corrupting tomcat request #11469

Merged
merged 1 commit into from
May 29, 2024

Conversation

laurit
Copy link
Contributor

@laurit laurit commented May 27, 2024

Hopefully resolves #11464
Related to #6766
Avoid calling toString on MessageBytes and use messageBytesToString instead as was done in #6766 for other places.
I wasn't able to figure out how exactly reading headers manages to corrupt other parts of the request (there seems to be shared buffer, part of which gets overwritten so request uri starts returning an unexpected result). I believe that this can only be reproduced with some versions of tomcat (test app uses 10.1.5). #6766 mentions that the issue was introduced in 10.1.0 but was later backported to earlier versions. Latest version does not seem to contain the problematic code any more.

@laurit laurit requested a review from a team as a code owner May 27, 2024 12:29
@pepeshore
Copy link

Calling toString will execute the code in the screenshot below which will overwrite a globle buffer。
image

The code above was introduced in Tomcat version 10.1.0 and fixed in version 10.1.6, fixed code is below
image

It seems to be a tomcat bug, but does it a good choice to read header in @Advice.OnMethodEnter of Tomcat method org.apache.catalina.connector.CoyoteAdapter.service

I notice that postParseRequest is called in org.apache.catalina.connector.CoyoteAdapter.service, It means that the request is not ready? so we should not reader header here?

@laurit laurit merged commit 300ad5e into open-telemetry:main May 29, 2024
53 checks passed
@laurit laurit deleted the tomcat-request-corrupt branch May 29, 2024 06:11
zeitlinger pushed a commit to zeitlinger/opentelemetry-java-instrumentation that referenced this pull request Jun 12, 2024
laurit added a commit to laurit/opentelemetry-java-instrumentation that referenced this pull request Jun 17, 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.

-Dotel.instrumentation.http.server.capture-request-headers=Cookie may cause http request 404
3 participants