att_svr memory corruption: entry_mem is freed without relocating existing entries
- Dominant language
- C
- Stars
- 893
- Forks
- 512
- Avg merge
- 13d 31m
- Merged PRs (30d)
- 7
Description
# How to reproduce
I initialized the GATT server in the following sequence:
- init some included services, e.g.: `ble_svc_gap_init()`, `ble_svc_gatt_init()`
- start the GATT server: `ble_gatts_start()`
- add some custom services: `ble_gatts_count_cfg()`, `ble_gatts_add_svcs()`
- (re)start the GATT server, to load additional services: 'ble_gatts_start()`
This will lead to hard-faults on my platform (`nrf52dk`, RIOT, nimble controller).
Used this RIOT branch: https://github.com/haukepetersen/RIOT/tree/opt_nimble_gattsinitpr
NimBLE version: same on master and on 997dad8c9fc549e64b6c54eafcec109d92789418
# Problem
When initializing the GATT server, and thereby implicitly the ATT server, the ATT server frees and (re-)allocates the internal memory it uses for storing the servers entries (`ble_att_svr_entry_mem`). The problem is, that if `ble_gatts_start()` is called multiple times, the ATT server frees `ble_att_svr_entry_mem` which still contains valid entries. This freed memory is eventually re-used by other components in the system, leading to corrupted entries in the ATT server, and eventually making a system hard-fault...
Simple test: add `assert((void *)entry >= (void *)ble_att_svr_entry_mem);` into `ble_att_svr_find_by_uuid()` -> this assertion is triggered when executing the sequence sketched above
I am not so sure about a viable fix though: simply removing the `ble_att_svr_free_start_mem()` call from `ble_att_svr_start()` will fix this issue, but it sure enough opens a memory leak :-)
I guess we probably must probably do something like this in `ble_att_svr_start()`:
- allocate the new entry memory
- move all existing entries from the old entry memory into the new one
- and only then free the old entry memory again?
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with ble_att_svr_start() and the ble_att_svr_free_start_mem() call, then trace ble_att_svr_entry_mem and existing entries into ble_att_svr_find_by_uuid(). Reproduce the repeated ble_gatts_start() sequence on the nrf52dk/RIOT setup and verify that entries remain valid after reallocation, without leaving the old memory leaked.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100