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

bluetil: Ensure advertisement length does not exceed pkt len #20881

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

Teufelchen1
Copy link
Contributor

Contribution description

Hey 🦪

this fixes an erroneous advertisement parsing, in which the length of the adv. was inherently trusted.

Testing procedure

Good review should be sufficient.

@github-actions github-actions bot added Area: network Area: Networking Area: BLE Area: Bluetooth Low Energy support Area: sys Area: System labels Oct 1, 2024
@maribu
Copy link
Member

maribu commented Oct 1, 2024

lgtm. The CI has nitpicks, though.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 1, 2024
@riot-ci
Copy link

riot-ci commented Oct 1, 2024

Murdock results

✔️ PASSED

84f1ae3 bluetil: Ensure advertisement length does not exceed pkt len

Success Failures Total Runtime
10197 0 10197 16m:38s

Artifacts

@benpicco benpicco requested a review from haukepetersen October 1, 2024 16:50
@miri64
Copy link
Member

miri64 commented Oct 1, 2024

Maybe @BOZHENG001 wants to have a look.

@BOZHENG001
Copy link

The code meets its purpose. I never tried, but it seems some errors may occur if the advertisement length exceeds the packet length? In the BLE standard, I do not see such a limit for the advertisement packet length?

@Teufelchen1
Copy link
Contributor Author

I am unsure what action your comment demands?

@BOZHENG001
Copy link

Sorry for the confusion. Just wanted to know if our change meets the BLE standard or not. If yes, of course I will have no objection.

@Teufelchen1
Copy link
Contributor Author

Okay, took for ever but I found it, page 1432 in the current bluetooth spec. I think my fix is within the spec.
https://www.bluetooth.com/specifications/specs/core-specification-6-0/

@BOZHENG001
Copy link

Cool cool, thanks for the explanation. As long as it meets the standard, we are still BLE.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

I interpret @BOZHENG001's last comment as ACK. :-)

@miri64 miri64 enabled auto-merge October 2, 2024 16:53
@miri64 miri64 added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@benpicco benpicco added this pull request to the merge queue Oct 3, 2024
Merged via the queue into RIOT-OS:master with commit 248371e Oct 3, 2024
28 checks passed
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants