fix issues caused by websocket frame fragmentation#1962
Merged
fafhrd91 merged 1 commit intoaio-libs:masterfrom Jun 8, 2017
pjknkda:master
Merged
fix issues caused by websocket frame fragmentation#1962fafhrd91 merged 1 commit intoaio-libs:masterfrom pjknkda:master
fafhrd91 merged 1 commit intoaio-libs:masterfrom
pjknkda:master
Conversation
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
| - | ||
|
|
||
| - | ||
| - Fix websocket issues caused by frame fragmentation. |
Member
There was a problem hiding this comment.
Please add #1962 at the end of line
aiohttp/http_websocket.py
Outdated
| 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] |
Member
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
| 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
| 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
| 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
| else: | ||
| self._payload_length = 0 | ||
| payload.extend(buf[start_pos:start_pos+length]) | ||
| payload.extend(buf[start_pos:start_pos + length]) |
Contributor
Author
|
@asvetlov I've fixed the commit according to your comments. Thank you for reviewing! |
Member
|
Lgtm |
Member
|
@asvetlov ping |
Member
|
@pjknkda btw did you run autobahn test suit against your fix? |
Contributor
Author
Member
|
Awesome! Thanks |
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.
4 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.
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.