This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Fix WebSocket server split header parsing with large payload#30402
Merged
stephentoub merged 3 commits intodotnet:masterfrom Jun 15, 2018
Merged
Fix WebSocket server split header parsing with large payload#30402stephentoub merged 3 commits intodotnet:masterfrom
stephentoub merged 3 commits intodotnet:masterfrom
Conversation
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.
Contributor
|
Do these new tests run in NETFX or UAP test runs? Since there are no SkipOnTargetFramework attributes, I think the answer is "yes". But perhaps the answer is "no"? |
Member
Author
The base class that contains the [Fact]s is used in two different projects. In the System.Net.WebSockets tests, it's used as the base class for tests that are netcoreapp-only, compiled in only for netcoreapp, because the method being tested, WebSocket.CreateFromStream, is netcoreapp-specific. In the System.Net.WebSockets.WebSocketsProtocols tests, that will run on netfx, and presumably on uap as well, but it's still not testing the WebSocket implementation in those platforms, rather it's testing WebSocketProtocol.CreateFromStream, which is built from corefx sources and shipped OOB. |
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.
stephentoub
force-pushed
the
websocketcreatetests
branch
from
ffa63bc to
6b0595a
Compare
davidsh
reviewed
Jun 14, 2018
| [InlineData(1)] | ||
| [InlineData(14)] | ||
| [InlineData(4096)] | ||
| public void CreateFromStream_ValidBufferSizes_Succeed(int bufferSize) |
Contributor
There was a problem hiding this comment.
nit:
In this PR, I see "_Succeed", "_Succeeds", and "_Success".
Maybe just pick "_Success" ?
davidsh
approved these changes
Jun 14, 2018
Contributor
davidsh
left a comment
davidsh
left a comment
There was a problem hiding this comment.
LGTM. I think the Outerloop tests should be run also since there are other websocket tests that would be affected by this code change.
Member
Author
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
stephentoub
added a commit
to stephentoub/corefx
that referenced
this pull request
Jun 15, 2018
…#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 a commit
that referenced
this pull request
Jun 26, 2018
#30407) * 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
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
…/corefx#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 Commit migrated from dotnet/corefx@d6d925e
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.
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.
As part of testing this, I also refactored the WebSocket{Protocol}.CreateFromStream tests to be shared. We have the WebSocket.CreateFromStream and WebSocketProtocol.CreateFromStream methods, which are identical (both using ManagedWebSocket), 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. I centralized those tests into a base class 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. I then added a test for this specific bug, which asserts in debug prior to the fix.
Contributes to https://github.com/dotnet/corefx/issues/30375
cc: @davidsh, @anurse