Skip to content

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.

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