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(rumqttd/protocol/v5): Parse ConnAck and UnsubAck packets instead of panicking #812

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

izik1
Copy link

@izik1 izik1 commented Mar 4, 2024

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

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.
    • I have zero clue what to write in the changelog entry for this, so I currently haven't written anything

@swanandx
Copy link
Member

swanandx commented Mar 5, 2024

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
Copy link
Author

izik1 commented Mar 5, 2024

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):

PacketType::ConnAck => Packet::ConnAck(connack::read(fixed_header, packet)?, None),

PacketType::UnsubAck => Packet::UnsubAck(unsuback::read(fixed_header, packet)?, None),

if this is undesirable a change to v4 parsing is required as well.

@swanandx
Copy link
Member

swanandx commented Mar 6, 2024

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
Copy link

coveralls commented Mar 6, 2024

Pull Request Test Coverage Report for Build 8208492265

Warning: This coverage report may be inaccurate.

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.

Details

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 37.827%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rumqttd/src/protocol/v5/mod.rs 0 4 0.0%
Totals Coverage Status
Change from base Build 8063409788: -0.004%
Covered Lines: 6255
Relevant Lines: 16536

💛 - Coveralls

@izik1
Copy link
Author

izik1 commented Mar 8, 2024

Yeah, I can do all that.

Signed-off-by: Skyler Ross <[email protected]>
@swanandx
Copy link
Member

hey @izik1 , this PR somehow slipped through, can you please fix the merge conflicts so that we can merge it? Thanks for understanding!

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.

3 participants