net: Fix several issues with HTTP2 frame processing #75510
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rlubos
changed the title
jukkar
previously approved these changes
Jul 5, 2024
* Remove unneeded variable. * Use system utilities to read big endian numbers instead of parsing manually. * Remove `payload` member from the http_frame structure. It's not used for anything useful, and could actually be misleading, as in case of large frames, where not entire frame is parsed at once it will point to incorrect location. Signed-off-by: Robert Lubos <[email protected]>
For HTTP2-specific structures and enums, use "http2_" prefix to clearly indicate the distinction from the generic HTTP stuff. Additionally, some structures/enums describing HTTP2 protocol details had "server" in the name, while in reality they describe nothing server-specific. Hence, drop the "server" part where applicable. Remove unused macros. Signed-off-by: Robert Lubos <[email protected]>
Instead of multiplying function to check header flags, just have a single one, with flag mask as parameter. Signed-off-by: Robert Lubos <[email protected]>
Data and header frames can contain padding - we need to take this into account when parsing them, otherwise the stream is broken. Signed-off-by: Robert Lubos <[email protected]>
In case priority flag is present in the HTTP2 headers frame header, we should expect additional priority fields before the actual frame content. The stream priority signalling has been deprecated by RFC 9113, however we should still be able to handle this in case some implementation (nghttp for instance) sends them. Signed-off-by: Robert Lubos <[email protected]>
In case RST_STREAM frame is received it should not be ignored, but the corresponding stream should be closed. Signed-off-by: Robert Lubos <[email protected]>
There's really no good reason to have an upper bound on the buffer sizes and this limits testing in some cases, so just remove them. Signed-off-by: Robert Lubos <[email protected]>
Frame printouts should not be done from the state handlers, but rather during state transition, otherwise a single frame can be printed several times as new data arrive. This also simplifies code a bit, as we just print the frame in a single place, instead of duplicating code. Signed-off-by: Robert Lubos <[email protected]>
CONTINUATION frames are tricky, because individual header fields can be split between HEADERS frame and CONTINUATION frame, or two CONTINUATION frames. Therefore, some extra logic is needed when header parsing returns -EAGAIN, as we may need to remove the CONTINUATION frame header from the stream before proceeding with headers parsing. This commit implements the above logic and additionally adds more checks to detect when CONTINUATION frame is expected. Not receiving a CONTINUATION frame when expect should be treated as a protocol error. Signed-off-by: Robert Lubos <[email protected]>
In case client decides to send a trailing headers frame, the last data frame will not carry END_STREAM flag. In result, with current logic server would not include END_STREAM flag either, causing the connection to stall. This commit fixes this logic, so that the server replies accordingly in case END_STREAM flag is present in the trailing headers frame. Signed-off-by: Robert Lubos <[email protected]>
The information about replied headers or END_OF_STREAM flag are stream-specific and not general for a client. Hence, need to move them to the stream context. For the upgrade case, we need to allocate a new stream now when HTTP1 request /w upgrade field is received. The stream ID in such case is assumed to be 1 according to RFC. Signed-off-by: Robert Lubos <[email protected]>
rlubos
force-pushed
the
net/improve-http2-frame-handling
branch
from
d008b71
to
cf1e0b0
Compare
|
Had to push one additional change to commit cf1e0b0 for the HTTP upgrade case - needed to allocate stream context now as the flags moved. |
jukkar
approved these changes
Jul 9, 2024
pdgendt
approved these changes
Jul 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the details, please check individual commit messages.