Skip to content

Fix GH-9316: $http_response_header is wrong for long status line - #9319

Closed
cmb69 wants to merge 4 commits into
php:PHP-8.0from
cmb69:cmb/gh9316
Closed

cmb69 wants to merge 4 commits into
php:PHP-8.0from
cmb69:cmb/gh9316

Conversation

@cmb69

@cmb69 cmb69 commented Aug 12, 2022

Copy link
Copy Markdown
Member

While the reason-phrase in a HTTP response status line is usually
short, there is no actual limit specified by the RFCs. As such, we
must not assume that the line fits into the buffer (which is currently
128 bytes large).

Since there is no real need to present the complete status line, we
simply read and discard the rest of a long line.

@cmb69

cmb69 commented Aug 16, 2022

Copy link
Copy Markdown
Member Author

Any thoughts about this? Should we append the rest of the status line (instead of discarding it)? Should we just increase the buffer size (currently 256 bytes) to able to read longer status lines without further changes? Should there be a test case? Or should we just leave this as is? Oh, and of course, which branch should this target?

@cmb69
cmb69 marked this pull request as ready for review August 16, 2022 12:45
@TimWolla

Copy link
Copy Markdown
Member

Should we append the rest of the status line (instead of discarding it)?

Not necessary. The reason phrase only exists with HTTP/1.x and even then it is entirely optional and might be missing.

Should we just increase the buffer size (currently 256 bytes) to able to read longer status lines without further changes?

I'd say 256 is plenty for the status line. However the other headers should be able to support longer values. So if they already do, then making a dynamic allocation for the status line, might also make sense.

Should there be a test case?

Yes, I think there should be. It should test both overly long reason phrases and also an entirely empty one (unless there already is).

Or should we just leave this as is?

What exactly does "as is" refer to? The current situation as it is in “master”? Then no, it is a real bug when the status line spills over into another header. In fact this might even have security implications.

Oh, and of course, which branch should this target?

As this is a real bugfix, it should be backported to 8.0.x at the very least. 7.4 might be a stretch.

@cmb69

cmb69 commented Aug 17, 2022

Copy link
Copy Markdown
Member Author

However the other headers should be able to support longer values.

The other headers are already properly handled by not using a fixed size buffer. The buffer is only used to read the status line. Increasing the buffer would have helped in this case, but we never can be sure how long a status line actually is, so discarding the rest still would be necessary.

It should test both overly long reason phrases and also an entirely empty one (unless there already is).

Make sense. I'll add those tests.

As this is a real bugfix, it should be backported to 8.0.x at the very least. 7.4 might be a stretch.

ACK

cmb69 added 2 commits August 17, 2022 11:58
While the reason-phrase in a HTTP response status line is usually
short, there is no actual limit specified by the RFCs.  As such, we
must not assume that the line fits into the buffer (which is currently
128 bytes large).

Since there is no real need to present the complete status line, we
simply read and discard the rest of a long line.
@cmb69
cmb69 changed the base branch from master to PHP-8.0 August 17, 2022 10:33

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix and test look good to me, apart from this technicality, consider this an approval.

Comment thread ext/standard/tests/http/gh9316.phpt Resolved
Comment thread ext/standard/tests/http/gh9316.phpt Resolved
@cmb69 cmb69 closed this in 72da418 Aug 18, 2022
@cmb69
cmb69 deleted the cmb/gh9316 branch August 18, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

contents of $http_response_header is incorrect with long http message

2 participants