search

njh / ruby-mqtt

Pure Ruby gem that implements the MQTT protocol, a lightweight protocol for publish/subscribe messaging.
http://www.rubydoc.info/gems/mqtt
MIT License
538 stars 135 forks source link

Handle instantaneous and unsolicited PUBACKs #158

Closed leoarnold closed 2 months ago

leoarnold commented 1 year ago

Prior to this commit, it was possible to crash the read thread by sending a PUBACK with an unexpected ID, e.g. an ID the client had not used before, or had already deleted the queue for. These packages will now simple be ignored.

Furthermore, with QoS 1 or 2, the PUBACK queue for a package was only created after the package had already been published. If the PUBACK arrived within this tiny gap, the read thread would crash. We fix this by creating the queue before starting to publish.

phlegx commented 10 months ago

@leoarnold nice work! @njh can you check this PR please and merge/release?

njh commented 3 months ago

Thank you for this PR. I am wondering if this is the correct behaviour - to ignore invalid PUBACK identifiers. Under what situation would you expect this to occur? Due to a faulty server? Or due to a bad actor? I think if you are connected to a server that is trying to do harm to its clients, you might have bigger problems.

I haven't read the specification in a long time. But typically in the MQTT protocol, if something goes wrong, the action is to disconnect. I did a quick scan and I don't think this scenario is documented in version 3.1.1. of the specification.

Interested in your thoughts.

leoarnold commented 3 months ago

@njh Malicious actors aside, the current code will first send a package and then add it to the list of "awaiting PUBACK". This allows for a race condition: If the PUBACK is processed before you can register that you are actually waiting for it, then the code will break. This PR essentially suggests to first register your expectation for a PUBACK before publishing, thereby eliminating the race condition.

jeltz commented 2 months ago

Yeah, ran into this issue too. The race condition mentioned is very real and I hit it quite often when I use QoS 1. I am not a fan of ignoring unknown packets but the part of the patch which fixes the race condition is something which is useful.

njh commented 2 months ago

Build is now failing. Want to understand why / fix it before making a release.