apache / apache/mynewt-nimble

att_svr memory corruption: entry_mem is freed without relocating existing entries

Open
#556 2 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.