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 Issue #156 #158

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Newman101

Newman101 commented Apr 28, 2016

Added prevention for buffer overflow in src/PubSubClient.cpp. Fixes issue #156.

@knolleary

This comment has been minimized.

Owner

knolleary commented Apr 28, 2016

Hi, thanks for this.

Simply returning 0 isn't sufficient. The client will then try to read the next packet... but we're no longer aligned to packet boundaries and there's no hope of recovering that.

If this condition is detected, the client needs to disconnect.

@Newman101

This comment has been minimized.

Newman101 commented Apr 28, 2016

Sure. I'll make this disconnect then. Thanks for notification.

@Newman101

This comment has been minimized.

Newman101 commented Apr 28, 2016

Do you wish the client to produce an error message upon disconnection?

@edwin-oetelaar

This comment has been minimized.

Contributor

edwin-oetelaar commented May 10, 2016

I personally do not find this very elegant or logical in program flow.
You close the TCP connection inside a reading loop and do not bail out cleanly even when you know of the error condition.
You depend on the readByte() function to return false on the closed connection and then make it return 0..
On detecting the error : Closing the connection, setting the state to 'protocol error' and directly returning 0 would be much cleaner.
I would even prefer the readPacket() function to return a signed int instead of an unsigned one, that way a negative number could indicate all kinds of errors... like connection-lost, protocol-error, whatever.
The readPacket() function is only used in 2 places, so the impact would be low.

Greetings,
Edwin van den Oetelaar

@knolleary

This comment has been minimized.

Owner

knolleary commented May 10, 2016

@edwin-oetelaar Agreed - the disconnection needs to done via a more graceful flow.

@Newman101 fyi, I don't get notified when you add a commit to the pull request, which is why I hadn't spotted your changes. Please add a comment if you have changed the pr.

@Newman101

This comment has been minimized.

Newman101 commented May 10, 2016

Sure, thanks for the notification. I can amend the current PR to correspond more graceful flow. At the moment the error detection does not return 0.

@knolleary

This comment has been minimized.

Owner

knolleary commented Nov 2, 2018

I pushed an alternative fix for this back in July - and have just refined it (it was still off-by-one) with 4daba0a

@knolleary knolleary closed this Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment