swanandx
commented
8 months ago broker can never send a connect packet, and hence, it can never read an ConnAck ( same with unsuback ). Hence it isn't required and is unreachable.
Open izik1 opened 8 months ago
swanandx
commented
8 months ago broker can never send a connect packet, and hence, it can never read an ConnAck ( same with unsuback ). Hence it isn't required and is unreachable.
izik1
commented
8 months ago It's absolutely reachable, a non compliant (malicious) client can send a ConnAck/UnsubAck specifically because it knows a rumqttd broker is running (this happens before auth is checked, since checking auth requires reading a packet) and would panic the connection. Perhaps the correct course of action would instead be an error?
I got the body of the fix from "do what v4 does" and it does the same thing (hence writing a PR instead of an issue): https://github.com/bytebeamio/rumqtt/blob/8af9d88c7862f2cbc2c69dcd98b8751ddf8c85dc/rumqttd/src/protocol/v4/mod.rs#L318 https://github.com/bytebeamio/rumqtt/blob/8af9d88c7862f2cbc2c69dcd98b8751ddf8c85dc/rumqttd/src/protocol/v4/mod.rs#L328 if this is undesirable a change to v4 parsing is required as well.
swanandx
commented
8 months ago oh yeah, you are right, we should not panic. Instead of error, lets parse it for now as you mentioned as do same in v4.
can we move handling connack right after connect ( like in v4, and same for unsuback ).
also, we can remove that _ => unreachable from v4 as well right?
for changelog, you can add under fix that "handle Connack and unsuback packets properly instead of panic" or something similar ( i'm not that good at this either haha )
thanks for pointing it out and the fix!
coveralls
commented
8 months ago This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| rumqttd/src/protocol/v5/mod.rs | 0 | 4 | 0.0% | ||
| <!-- | Total: | 0 | 4 | 0.0% | --> |
| Totals | |
|---|---|
| Change from base Build 8063409788: | -0.004% |
| Covered Lines: | 6255 |
| Relevant Lines: | 16536 |
izik1
commented
8 months ago Yeah, I can do all that.
swanandx
commented
2 months ago hey @izik1 , this PR somehow slipped through, can you please fix the merge conflicts so that we can merge it? Thanks for understanding!
If re-adding the
_ => unreachable!()is desired, let me know. I can't think of any benefit to it so I removed it.Type of change
Checklist:
cargo fmtCHANGELOG.mdif it's relevant to the users of the library. If it's not relevant mention why.