Skip to content

Fix request header capture corrupting tomcat request#11469

Merged
laurit merged 1 commit into
open-telemetry:mainfrom
laurit:tomcat-request-corrupt
May 29, 2024
Merged

Fix request header capture corrupting tomcat request#11469
laurit merged 1 commit into
open-telemetry:mainfrom
laurit:tomcat-request-corrupt

Conversation

@laurit

@laurit laurit commented May 27, 2024

Copy link
Copy Markdown
Contributor

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 May 27, 2024 12:29
@pepeshore

Copy link
Copy Markdown
Contributor

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
@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