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

fix decodePayloadAsBinary memory leak #27

Merged
merged 1 commit into from Jun 24, 2014
Merged

fix decodePayloadAsBinary memory leak #27

merged 1 commit into from Jun 24, 2014

Conversation

christophwitzko
Copy link
Contributor

If data has no byte with the value 255, the for loop will not exit and bufferTail gets bigger and bigger until the nodejs server (in my case) crashes.

strLen.length > 310 (Number.MAX_VALUE is about 310 characters long)

rauchg added a commit that referenced this pull request Jun 24, 2014
fix decodePayloadAsBinary memory leak
@rauchg rauchg merged commit 93a415b into socketio:master Jun 24, 2014
@@ -1 +1,2 @@
node_modules
.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to revert this.

@rase-
Copy link
Contributor

rase- commented Jun 24, 2014

Great fix. Thanks @christophwitzko!

@rase-
Copy link
Contributor

rase- commented Jun 24, 2014

We should never face this problem unless the packet is flaud. Long term we should handle the case as an error, probably so that is propagates all the way to the socket so that the error can easily be handled and still doesn't break the connection.

@rauchg
Copy link
Contributor

rauchg commented Jun 24, 2014

Well, since we don't have the idea of packet retransmission, I think error is a good way of handling this here.

@rase-
Copy link
Contributor

rase- commented Jun 24, 2014

Yup. Agreed.

@christophwitzko
Copy link
Contributor Author

Thank you!

@christophwitzko christophwitzko deleted the patch-memory-leak branch August 31, 2014 15:45
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.

None yet

3 participants