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

Handle bad length in header #155

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Handle bad length in header #155

merged 1 commit into from
Dec 1, 2023

Conversation

madmo
Copy link
Contributor

@madmo madmo commented Nov 23, 2023

Prevents a panic in fn receive_buffer if a packet with a too big length is received.

Copy link
Member

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! Thanks for the PR :)

I have some minor questions since I don't fully understand the issue.

src/de/packet_reader.rs Outdated Show resolved Hide resolved
@madmo madmo changed the title Reset too big packet lengths back to None Handle bad length in header Nov 24, 2023
Copy link
Member

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is protecting against an issue where the packet-header of the MQTT datagram may be corrupt and indicate a much larger size than we support, which we obviously should not use to index into self.buffer, so this is definitely a bug. Thanks for the fix here!

Can you please:

  • Add an entry to CHANGELOG.md indicating this defect has been fixed?
  • Fix the style formatting?

return error instead of panicing
@madmo
Copy link
Contributor Author

madmo commented Dec 1, 2023

It looks like this is protecting against an issue where the packet-header of the MQTT datagram may be corrupt and indicate a much larger size than we support, which we obviously should not use to index into self.buffer, so this is definitely a bug. Thanks for the fix here!

Can you please:

* [ ]  Add an entry to CHANGELOG.md indicating this defect has been fixed?

* [x]  Fix the style formatting?

Sure, updated the CHANGELOG. Thanks for reviewing!

Copy link
Member

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks like a nice fix. I can cut a patch release out for this as well.

@ryan-summers ryan-summers merged commit 0699e8c into quartiq:master Dec 1, 2023
7 checks passed
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.

2 participants