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

BLEManagerImpl may have MTU overflow its storage #2569

Closed
bzbarsky-apple opened this issue Sep 11, 2020 · 6 comments · Fixed by #14693
Closed

BLEManagerImpl may have MTU overflow its storage #2569

bzbarsky-apple opened this issue Sep 11, 2020 · 6 comments · Fixed by #14693
Assignees
Labels
bug Something isn't working p1 priority 1 work V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

In src/platform/ESP32/BLEManagerImpl.cpp we have this line in BLEManagerImpl::HandleGATTCommEvent:

conState->MTU = param->mtu.mtu;

The right-hand side is a uint16_t. The left-hand side is a 10-bit bitfield. There seems to be no check that the value on the right will fit on the left, nor any explanation for why a check is not needed.

Proposed Solution

Either introduce a check or add an explanation for why it's not needed.

@sagar-apple it looks like you might know something about this code?

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label bug to this issue, with a confidence of 0.87. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@issue-label-bot issue-label-bot bot added the bug Something isn't working label Sep 11, 2020
@sagar-apple
Copy link
Contributor

@shana-apple amd @vivien-apple are probably more familiar with this. I can look if they're unable.

@bzbarsky-apple
Copy link
Contributor Author

src/platform/EFR32/BLEManagerImpl.cpp has a similar issue in BLEManagerImpl::UpdateMtu.

@woody-apple woody-apple added this to the V1.0 milestone Sep 21, 2020
@franck-apple franck-apple added the p2 priority 2 work label Oct 16, 2020
@woody-apple woody-apple added p1 priority 1 work and removed p2 priority 2 work labels Oct 22, 2020
@franck-apple franck-apple modified the milestones: V1.0, 2020-11-20 Oct 23, 2020
@woody-apple woody-apple modified the milestones: 2020-11-20, Next Feb 4, 2021
@franck-apple franck-apple modified the milestones: Next, Test Event 1 Feb 26, 2021
@franck-apple franck-apple removed the TE1 label Mar 9, 2021
@franck-apple franck-apple removed this from the Test Event 1 milestone Mar 9, 2021
@andy31415 andy31415 assigned dhrishi and unassigned shana-apple Jan 31, 2022
@andy31415
Copy link
Contributor

This looks like platform specific code - re-assigned to platform owner.

@dhrishi
Copy link
Contributor

dhrishi commented Feb 1, 2022

nor any explanation for why a check is not needed.

@bzbarsky-apple As per the BLE specification, the maximum MTU value can be 517 bytes. This can be accomodated in 10 bits. Good enough to close this?

@bzbarsky-apple
Copy link
Contributor Author

Good enough, but would be even better with a comment. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p1 priority 1 work V1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants