fix issues caused by websocket frame fragmentation #1962
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
pjknkda
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1962 +/- ##
==========================================
+ Coverage 97.05% 97.08% +0.02%
==========================================
Files 37 37
Lines 7612 7609 -3
Branches 1328 1327 -1
==========================================
- Hits 7388 7387 -1
+ Misses 101 100 -1
+ Partials 123 122 -1
Continue to review full report at Codecov.
|
asvetlov
requested changes
Jun 7, 2017
CHANGES.rst
Outdated
| @@ -11,7 +11,7 @@ Changes | |||
|
|
|||
| - | |||
|
|
|||
| - | |||
| - Fix websocket issues caused by frame fragmentation. | |||
There was a problem hiding this comment.
Please add #1962 at the end of line
aiohttp/http_websocket.py
Outdated
| # read header | ||
| if self._state == WSParserState.READ_HEADER: | ||
| if buf_length - start_pos >= 2: | ||
| data = buf[start_pos:start_pos+2] | ||
| data = buf[start_pos:start_pos + 2] |
There was a problem hiding this comment.
Please don't touch what is not broken: spaces are not required here.
aiohttp/http_websocket.py
Outdated
| @@ -345,7 +334,7 @@ def parse_frame(self, buf, continuation=False, EMPTY=b''): | |||
| length = self._payload_length_flag | |||
| if length == 126: | |||
| if buf_length - start_pos >= 2: | |||
| data = buf[start_pos:start_pos+2] | |||
| data = buf[start_pos:start_pos + 2] | |||
aiohttp/http_websocket.py
Outdated
| @@ -357,7 +346,7 @@ def parse_frame(self, buf, continuation=False, EMPTY=b''): | |||
| break | |||
| elif length > 126: | |||
| if buf_length - start_pos >= 8: | |||
| data = buf[start_pos:start_pos+8] | |||
| data = buf[start_pos:start_pos + 8] | |||
aiohttp/http_websocket.py
Outdated
| @@ -377,7 +366,7 @@ def parse_frame(self, buf, continuation=False, EMPTY=b''): | |||
| # read payload mask | |||
| if self._state == WSParserState.READ_PAYLOAD_MASK: | |||
| if buf_length - start_pos >= 4: | |||
| self._frame_mask = buf[start_pos:start_pos+4] | |||
| self._frame_mask = buf[start_pos:start_pos + 4] | |||
aiohttp/http_websocket.py
Outdated
| @@ -394,7 +383,7 @@ def parse_frame(self, buf, continuation=False, EMPTY=b''): | |||
| start_pos = buf_length | |||
| else: | |||
| self._payload_length = 0 | |||
| payload.extend(buf[start_pos:start_pos+length]) | |||
| payload.extend(buf[start_pos:start_pos + length]) | |||
|
@asvetlov I've fixed the commit according to your comments. Thank you for reviewing! |
|
Lgtm |
|
@asvetlov ping |
|
@pjknkda btw did you run autobahn test suit against your fix? |
|
Awesome! Thanks |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What do these changes do?
This MR fixes issues caused by websocket frame fragmentation.
Based on autobahn-testsuite, there were several bugs in websocket implementation which is mainly caused by wrong handling of fragmented frames.
The rationale of deleting the test case "test_parse_frame_header_continuation" is that according to RFC6455 Section 5.5, control frames can be injected in the middle of a fragmented message and therefore we cannot reject the frame only seeing the "FIN" flag of the last frame. Also, test for invalid continuation frame without preceding text/binary frame is done by test case "test_unknown_frame".
Here is the report generated from autobahn-testsuite.
Related issue number
#1845, #1951
Checklist
CONTRIBUTORS.txtCHANGES.rst#issue_numberformat at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.