This repository was archived by the owner on Jan 23, 2023. It is now read-only.
[release/2.1] Fix WebSocket server split header parsing with large payload (#30402)#30407
Merged
stephentoub merged 1 commit intodotnet:release/2.1from Jun 26, 2018
Merged
Conversation
…#30402) * Refactor WebSocket{Protocol}.CreateFromStream tests to be shared We have the WebSocket.CreateFromStream and WebSocketProtocol.CreateFromStream methods, which are identical, except that the former is netcoreapp-only and the latter is in a separate NuGet package for downlevel use. However, tests for them were separate, with some tests only for one and some tests for the other. This commit centralizes those tests so they're shared by and apply to both methods. There are no code changes to the actual bodies of tests, just moving code around to have it apply to both. * Fix WebSocket server split header parsing with large payload When ReceiveAsync is called, as a fast path it checks to see whether there's already enough data in the buffer to satisfy any possible header, skipping subsequent checks if there is. The max header size differs between client and server, though. The maximum size header a client can send to the server is 14 bytes, which includes a 4-byte masking value; the maximum size header a server can send to a client is 10 bytes, as it doesn't include a masking value. However, the code currently has those values reversed. If the code is running on the client, this means that we end up falling back to the slow-path unnecessarily if there are 10, 11, 12, or 13 bytes already in the buffer when ReceiveAsync is called. However, on the server, this means we end up potentially throwing an exception or misinterpreting the payload if 10, 11, 12, or 13 bytes are in the buffer and the packet contains a large payload (in which case it'll be using an 8-byte length and be the full 14 byte header), as we'll end up erroneously taking the fast path when we should have taken the slow path to read more data from the network. The fix is simply to swap the branches of the conditional. * Address PR feedback
stephentoub
added
Servicing-consider
danmoseley
removed
the
Servicing-consider
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Port #30402 to release/2.1
Fixes #30375