Adafruit_MQTT::publishPacket: Protect against memory corruption.

flavio-fernandes commented 4 years ago

Fixes https://github.com/adafruit/Adafruit_MQTT_Library/issues/109 Fixes https://github.com/adafruit/Adafruit_MQTT_Library/issues/122

After spending a long time pulling my hair to understand why I was hitting a panic when attempting to read from my registered subscriptions, I found out that the subscriptions member of the Adafruit_MQTT instance was corrupted. :(

Turns out the memory corruption was caused by my publish call, where the payload I was providing was bigger than the allocated space in the buffer for construction of the packet (see buffer[MAXBUFFERSIZE]).

To protect myself from ever making this mistake again, I am proposing a simple logic in publishPacket where instead of silently corrupting memory, the code uses as much payload as it can fit in the available space. By seeing the truncated payload, user can decide whether he/she should 1)break it up into different topics, 2) put the payload on a diet, or 3) increase MAXBUFFERSIZE.

====

Describe the scope of your change--i.e. what the change does and what parts of the code were modified. This will help us understand any risks of integrating the code.

This pull requests improves the handling of buffer for packet creation of Adafruit_MQTT::publishPacket. It adds argument maxPacketLen to keep track of how much space is available in buffer.

Describe any known limitations with your change. For example if the change doesn't apply to a supported platform of the library please mention it.

This change is not modifying the sizes of any buffers. Instead, it truncates the payload to avoid memory corruption of the class Adafruit_MQTT by keeping packet creation from going beyond MAXBUFFERSIZE.

Please run any tests or examples that can exercise your modified code. We strive to not break users of the code and running tests/examples helps with this process.

See this gist with an example code where a packet has a payload that exceeds MAXBUFFERSIZE and corrupts packet_id_counter and subscriptions of the Adafruit_MQTT instance.

See comments below that gist with the crash and stack trace of code above, where memory is corrupted and the LoadProhibited exception takes place.

Lastly, see the comment that follows with the proposed modification, where the same code simply gets the payload truncated instead of allowing memory to get messed up.

BrunoMartins11 commented 3 years ago

It's 2021 and I still struggled to find that this is the reason why my program is crashing. Please review this PR asap for the sake of our mental health.

Duckle29 commented 3 years ago

I share the frustration :( There seems to be a lot of nice PRs that have been sitting for more than a year. It's a shame. Thankfully platformIO lets me use git repos for lib_deps, so I forked your fix @flavio-fernandes, and used that. Thanks for the work you put in! :)

I'd switch to something like Async MQTT, but I need to use a library that uses a Client object like this, to work with my bearSSL certificate store.

flavio-fernandes commented 3 years ago

I share the frustration :( There seems to be a lot of nice PRs that have been sitting for more than a year. It's a shame. Thankfully platformIO lets me use git repos for lib_deps, so I forked your fix @flavio-fernandes, and used that. Thanks for the work you put in! :)

I'd switch to something like Async MQTT, but I need to use a library that uses a Client object like this, to work with my bearSSL certificate store.

Maybe @ladyada or @brentru can listen to our cries. I do not mind if a different PR gets used; just wish for a better experience using this awesome library.

flavio-fernandes commented 3 years ago

@flavio-fernandes Made two small comments. Rest looks good and is similar to a portion of MiniMQTT which I've implemented to address a similar issue.

After you're done addressing them, please bump the patch version number in https://github.com/adafruit/Adafruit_MQTT_Library/blob/master/library.properties#L2 by 1 so we can get this picked up by the Arduino Library Manager tool.

@flavio-fernandes Made two small comments. Rest looks good and is similar to a portion of MiniMQTT which I've implemented to address a similar issue.

After you're done addressing them, please bump the patch version number in https://github.com/adafruit/Adafruit_MQTT_Library/blob/master/library.properties#L2 by 1 so we can get this picked up by the Arduino Library Manager tool.

Ack! TBH, it has been a while since I thought about this PR. Let me think a little bit about what you said and make the changes.

brentru commented 3 years ago

@flavio-fernandes ok, let me know when its ready to review. I have a pretty large MQTT application which uses this library and I'd like to test with that.

flavio-fernandes commented 3 years ago

@flavio-fernandes ok, let me know when its ready to review. I have a pretty large MQTT application which uses this library and I'd like to test with that.

Hi @brentru ! Okay, I made the changes and pushed an update. Let me know what you think, please. As for testing the bug, consider using this gist to easily make the corruption take place without the fix; and the non-crashing version once the pr changes are added. Lastly, I bumped the version to 2.1.1, as you requested.

Best!

brentru commented 3 years ago

@flavio-fernandes Could you bump to 2.2.0? I tested this against a SECRET project and it prevents corruption there as well.

flavio-fernandes commented 3 years ago

@flavio-fernandes Could you bump to 2.2.0? I tested this against a SECRET project and it prevents corruption there as well.

@brentru : Done bumping to 2.2.0 . Secret project? You make me curious! :) Best, -- flaviof

adafruit / Adafruit_MQTT_Library

Adafruit_MQTT::publishPacket: Protect against memory corruption. #166