Fix several independent BLE bugs, split from #11178 - #11225
Open
dhalbert wants to merge 7 commits into
Open
Conversation
`_incoming` is `uint32_t[64]`, so `sizeof(_incoming)` is already 256 bytes. Multiplying by `sizeof(uint32_t)` told the ring buffer it had 1024, letting it write past the array. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
any_connected was initialized to false, so the wait loop never ran and the stack was torn down before the disconnect went out. The central then saw a supervision timeout instead of a clean disconnect. Same copied bug in nordic, espressif, and silabs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ble_drv_remove_event_handler() clears the removed entry's next pointer, so iterating via it->next after removal ended the walk at the first heap handler, leaving the rest pointing into a heap about to be freed. Capture next first. Also remove entries that themselves live on the heap, not just those with a heap param. Same copied bug in nordic ble_drv.c and espressif ble_events.c. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
common_hal_bleio_packet_buffer_deinit() left self->characteristic set, so deinited() -- which tests it against NULL -- never reported the buffer as deinited, and guards like the ones in supervisor/shared/bluetooth/serial.c never took effect. Same copied bug in nordic, silabs, and ble_hci. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Registration chooses packet_buffer_on_ble_client_evt or packet_buffer_on_ble_server_evt by self->client, but deinit always removed the client handler, leaving a server packet buffer's handler registered and pointing at a freed buffer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
For unlimited advertising (timeout 0), private_addr_cycle_s = timeout + 1 rotated the address every second -- too fast for a central to resolve an address and connect to it before it changed. Use 0, which selects the SoftDevice default cycle of 15 minutes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On BLE_GAP_EVT_CONNECTED, a recycled slot kept the previous peer's bonding keys, ediv, and pending bond-save flags. The SoftDevice fills in only the keys the new peer distributes, so the leftovers could mix one peer's IRK with another's LTK in a stored bond, or save this connection's bond data under the previous peer's ediv. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claude found these bugs in the course of working on #11178.
_incomingbuffer size passed to_common_hal_bleio_characteristic_buffer_construct().bleio_adapter_reset()(nordic, espressif, silabs): while loop never ran because initial condition was false, so explicit disconnect didn't happen. The central saw a supervision timeout instead of a clean disconnect. Changed to do-while.ble_drv.c, espressifble_events.c): only the first entry was removed.common_hal_bleio_packet_buffer_deinited()could never return true (nordic, silabs, ble_hci):deinit()did not clearself->characteristic, so the guards insupervisor/shared/bluetooth/serial.cnever took effect.packet_buffer_deinit()removed the wrong event handler: it always removed the client-event handler, leaving a server packet buffer's handler registered and pointing at a freed buffer.private_addr_cycle_s = timeout + 1was only one second. Changed to use the SoftDevice default of 15 minutes.ediv, and pending bond-save flags, which could mix one peer's IRK with another's LTK or save bond data under the wrongediv.First of the series of small PRs replacing #11178, as discussed there. These are the independent bug fixes; the reset-logic and advertising-behavior changes will follow in separate PRs.
Tested on Nordic and Espressif. This does not fix the primary bug that #11178 started to address. That will come later in a further PR.
@tannewt We can look for these issues in the zephyr BLE port, since it took inspiration from the other ports.