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

Fixed FCGI content parsing. #9790

Merged
merged 2 commits into from
May 25, 2023
Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented May 22, 2023

WordPress sends the initial content in the same frame as the response headers. StreamContentParser was receiving the frame, delegating to HttpParser to parse the headers, but the buffer was still containing some content, that was lost. Now the content after the headers is correctly retained.

WordPress sends the initial content in the same frame as the response headers.
StreamContentParser was receiving the frame, delegating to HttpParser to parse the headers,
but the buffer was still containing some content, that was lost.
Now the content after the headers is correctly retained.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from lorban May 22, 2023 12:13
@lorban
Copy link
Contributor

lorban commented May 22, 2023

Does this bug only affect 12 and not 10/11?

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

LGTM but I wonder if that fix shouldn't go to 10/11 as well.

Now a read-only version of the buffer is passed to the listener,
and the original buffer is consumed as the caller expects.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Contributor Author

sbordet commented May 25, 2023

@lorban I verified that this is not a problem in 10/11.
The reason is that in 10/11 we have a "push" model for the content, so we parse the headers, notify the application, then continue parsing, find the content, notify the application, so the buffer is fully consumed and the content is not lost.
In 12 we have a "pull" model for the content, so we stop parsing after the headers because we give control to the application to call Content.Source.read(), but by doing so we re-enter the parsing but lost the content.

@sbordet sbordet merged commit 476145b into jetty-12.0.x May 25, 2023
@sbordet sbordet deleted the fix/jetty-12-fcgi-content-parsing branch May 25, 2023 13:22
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.

2 participants