Skip to content

Refine when content is expected #2044

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

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Feb 8, 2025

Consider Content-Length and Transfer-Encoding headers when determining whether to expect content. Don't expect content for DELETE requests and don't handle the HTTP/2 connection preface pseudo-method PRI.

Fixes #2028.

@yhirose
Copy link
Owner

yhirose commented Feb 8, 2025

@falbrechtskirchinger thanks for the pull request, but #609 appears again with this change. Could you take a look at it?

@yhirose
Copy link
Owner

yhirose commented Feb 8, 2025

I added a unit test for #609 for you. You can rebase your branch with the master to get the test.
eb10c22

@falbrechtskirchinger
Copy link
Contributor Author

@falbrechtskirchinger thanks for the pull request, but #609 appears again with this change. Could you take a look at it?

Sure, I'll take a look.

I added a unit test for #609 for you. You can rebase your branch with the master to get the test. eb10c22

Thanks!

Consider Content-Length and Transfer-Encoding headers when determining
whether to expect content. Don't handle the HTTP/2 connection preface
pseudo-method PRI.

Fixes yhirose#2028.
@falbrechtskirchinger
Copy link
Contributor Author

@yhirose I've re-added DELETE to expect_content(). Test passes now.

@@ -5381,10 +5381,14 @@ write_multipart_ranges_data(Stream &strm, const Request &req, Response &res,

inline bool expect_content(const Request &req) {
if (req.method == "POST" || req.method == "PUT" || req.method == "PATCH" ||
req.method == "PRI" || req.method == "DELETE") {
req.method == "DELETE") {
Copy link

Choose a reason for hiding this comment

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

Nice, thanks for the fix. Btw as explained in #2028 (comment) , we should also add OPTIONS here (or maybe replace HEAD with OPTIONS here, I'm not sure if the browser even use HEAD or not)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. As long as Content-Length is set, things should work. The explicit handling of DELETE is specifically for handlers with content receivers. For OPTIONS requests, we have no such handlers.

I cannot reproduce any issues with OPTIONS. If that still fails, let me know what you're doing and I'll check on my end.

@yhirose yhirose merged commit 7adbccb into yhirose:master Feb 8, 2025
4 checks passed
@yhirose
Copy link
Owner

yhirose commented Feb 8, 2025

@falbrechtskirchinger thanks for the fine contribution!

@falbrechtskirchinger falbrechtskirchinger deleted the fix2028 branch February 15, 2025 16:27
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.

Potential memory corruption in stream_line_reader
3 participants