Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for frame spillover issue (#218, #230) #220

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hyperair
Copy link

@hyperair hyperair commented Jun 6, 2017

When Frame.parser is given more data than expected, the next frame's header +
contents can end up as part of the payload, causing a UTF-8 validation error, or
ends up being thrown away. This can happen in the SSL case if we read slowly and
allow the socket's buffer to fill up a little.

This commit fixes that by amending WebSocket.once() to only provide bytes <=
reading_buffer_size to the stream parser and keeping the rest in a buffer,
giving the Frame a chance to stop receiving data at a frame boundary.

Fixes: #218

Edit: This PR now only contains tests for the issue since the issue itself has been fixed by #239.

@Ngo-The-Trung
Copy link

I think the assertion is contrived and can be removed (it's not validating anything outside the function's scope).

@hyperair hyperair force-pushed the fix-frame-spillover branch 4 times, most recently from ad6b1db to 0714fb2 Compare June 6, 2017 10:29
@Ngo-The-Trung
Copy link

Ngo-The-Trung commented Jun 12, 2017

@tito looking for your comments

@hyperair hyperair force-pushed the fix-frame-spillover branch 3 times, most recently from 56c38ee to 7a94b0a Compare June 12, 2017 09:46
@bytesofmyself
Copy link

Any ETA on this being merged?

@gfemec gfemec mentioned this pull request Jul 29, 2017
@Ngo-The-Trung
Copy link

@bytesofmyself unfortunately it seems like this is not maintained until there's a new maintainer

@amiasato-zz
Copy link

Commenting on this PR to show some appreciation, because this was an issue which was affecting me pretty badly. Thanks a lot for the fix.

It would be nice if #222 would check on this as well.

@Serpens66
Copy link

I used veersion 0.3.4 a long time without any problems.
Then I thought it might be good to upgrade to 0.4.2 and did it... bad idea.. since then I'm missing alot of messages... unusable...
I downgraded and it is working fine again.

Is this pull request fixing this issue? Or is it another issue?
Should I stay with 0.3.4 version ? Or are there other bugs I did not discover yet?

@amiasato-zz
Copy link

@Serpens66 It is very likely if you are using SSL/TLS sockets, otherwise the only way of knowing is testing for yourself.

@hyperair
Copy link
Author

This seems to have been superseded by #239. I've rebased onto master so that this PR only adds tests for the frame spillover issue now.

@hyperair hyperair changed the title Fix frame spillover when not read quickly enough Add test for frame spillover issue (#218, #230) Aug 21, 2019
Travis has dropped support for Python 3.3, so drop it from .travis.yml so that
builds succeed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream parser throws away data if too much read from socket
5 participants