RDKEMW-12176: btrCore_PopulateListOfPairedDevCrash - #60
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent a crash (reported as a UAF during teardown/deepsleep) by gating btrCore_PopulateListOfPairedDevices() so it won’t run while BTRCore_DeInit() is in progress.
Changes:
- Introduces a termination flag (
gIsBtrCoreTerminating) intended to blockbtrCore_PopulateListOfPairedDevices()during teardown. - Sets/clears the termination flag in
BTRCore_DeInit()/BTRCore_Init(). - Expands deinit logging to include the termination flag value.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Prevent UAF during teardown */ | ||
| static volatile gint gIsBtrCoreTerminating = 0; | ||
|
|
There was a problem hiding this comment.
gIsBtrCoreTerminating is a module-global flag, so terminating/deinit of one BTRCore handle will affect all handles in the process. Most state in this file is kept per-stBTRCoreHdl, so this introduces cross-instance coupling and can cause hard-to-debug behavior if multiple cores are initialized (or re-initialized) in the same process.
Consider moving the terminating flag into stBTRCoreHdl (per-instance) or switching to an atomic “active handle” pointer check (e.g., g_atomic_pointer_set/get) so calls can be rejected based on which handle is tearing down, rather than globally.
ba310cd to
b58b3be
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b58b3be to
8099e5a
Compare
8099e5a to
1385761
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9ebe22a to
23b1577
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
23b1577 to
a6dea1a
Compare
a6dea1a to
3d3c000
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3d3c000 to
8866a00
Compare
8866a00 to
395cd18
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
395cd18 to
f2235cb
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
665f85c to
a70b8ec
Compare
a70b8ec to
ff10d06
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gint btrCore_AddAndGetCurrGenForTest(void) { | ||
| /* Return the incremented (current) generation */ | ||
| return g_atomic_int_add(&gBtrCoreGenerationCounter, 1) + 1; | ||
| } | ||
|
|
||
| gint btrCore_GetTerminatorForTest(void) { | ||
| return g_atomic_int_get(&gIsBtrCoreTerminating); | ||
| } |
There was a problem hiding this comment.
The test helper functions btrCore_AddAndGetCurrGenForTest / btrCore_GetTerminatorForTest are compiled into the main library and exported (not STATIC, not guarded by #ifdef UNIT_TEST). This increases the production ABI surface area and can create accidental dependencies.
Wrap these in #ifdef UNIT_TEST (or similar) and keep them out of non-test builds, or move them to a dedicated test-only compilation unit.
ff10d06 to
d81ed11
Compare
d81ed11 to
9f15818
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| gint btrCore_AddAndGetCurrGenForTest(void); | ||
| gint btrCore_GetTerminatorForTest(void); | ||
| void btrCore_SetTerminatorForTest(void); | ||
| void btrCore_ResetTerminatorForTest(void); |
There was a problem hiding this comment.
These *ForTest symbols are being added to the public API surface (include/btrCore.h). This makes test-only hooks available to production consumers and also couples production code (BTRCore_DeInit) to a test-named function. Prefer gating these declarations/definitions behind a unit-test compile flag (e.g., #ifdef UNIT_TEST) or moving them to a dedicated test header, and keep production teardown helpers internal with non-test naming.
| gint btrCore_AddAndGetCurrGenForTest(void); | |
| gint btrCore_GetTerminatorForTest(void); | |
| void btrCore_SetTerminatorForTest(void); | |
| void btrCore_ResetTerminatorForTest(void); | |
| #ifdef UNIT_TEST | |
| gint btrCore_AddAndGetCurrGenForTest(void); | |
| gint btrCore_GetTerminatorForTest(void); | |
| void btrCore_SetTerminatorForTest(void); | |
| void btrCore_ResetTerminatorForTest(void); | |
| #endif /* UNIT_TEST */ |
| /* Prevent UAF when worker threads run during teardown */ | ||
| if(g_atomic_int_get(&gIsBtrCoreTerminating)) { | ||
| BTRCORELOG_WARN("btrCore: Ignoring PopulateListOfPairedDevices during termination\n"); |
There was a problem hiding this comment.
The termination guard is global (gIsBtrCoreTerminating) but does not account for the handle’s generation. Since BTRCore_Init unconditionally resets gIsBtrCoreTerminating back to 0, a new Init happening while an old instance is still tearing down can re-enable work in old threads, reintroducing the UAF scenario. A more robust approach is to validate the handle generation at call sites (e.g., treat a handle as invalid if apsthBTRCore->generation != g_atomic_int_get(&gBtrCoreGenerationCounter)), or use a per-instance termination flag/token rather than a resettable global boolean.
| /* Prevent UAF when worker threads run during teardown */ | |
| if(g_atomic_int_get(&gIsBtrCoreTerminating)) { | |
| BTRCORELOG_WARN("btrCore: Ignoring PopulateListOfPairedDevices during termination\n"); | |
| /* Prevent UAF when worker threads run during teardown or using stale handles */ | |
| if (g_atomic_int_get(&gIsBtrCoreTerminating) || | |
| (apsthBTRCore->generation != g_atomic_int_get(&gBtrCoreGenerationCounter))) { | |
| BTRCORELOG_WARN("btrCore: Ignoring PopulateListOfPairedDevices during termination or generation mismatch\n"); |
| g_atomic_int_set(&gIsBtrCoreTerminating, 0); | ||
|
|
There was a problem hiding this comment.
Resetting gIsBtrCoreTerminating to 0 during BTRCore_Init can race with an in-progress teardown from a previous instance (or concurrent deinit), potentially allowing worker threads from the previous instance to proceed again. Consider removing this global reset and instead deriving ‘terminating’ from a generation/token check (or store a terminating_generation value) so that a new init cannot clear termination state for older generations.
| g_atomic_int_set(&gIsBtrCoreTerminating, 0); |
| if(g_atomic_int_get(&gIsBtrCoreTerminating)) { | ||
| BTRCORELOG_WARN("btrCore: Ignoring PopulateListOfPairedDevices during termination\n"); | ||
| return enBTRCoreFailure; | ||
| } |
There was a problem hiding this comment.
New behavior is introduced here (short-circuiting paired-device population during termination), but the diff doesn’t add a focused unit test that sets termination and asserts btrCore_PopulateListOfPairedDevices (or a public wrapper that triggers it) returns enBTRCoreFailure and does not touch freed state. Adding a small targeted test would help prevent regressions of the crash fix.
9f15818 to
e2c01b8
Compare
e2c01b8 to
184b721
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
184b721 to
5721ef3
Compare
5721ef3 to
5985fd7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -4180,6 +4361,7 @@ void test_BTRCore_SetAdvertisementInfo_NullAdvtBeaconName(void) { | |||
| tBTRCoreHandle hBTRCore = (tBTRCoreHandle)1; // Mock handle | |||
| char advtType[] = "Type"; | |||
|
|
|||
| /* Invalid handle value - should never be deferenced */ | |||
| enBTRCoreRet result = BTRCore_SetAdvertisementInfo(hBTRCore, advtType, NULL); | |||
There was a problem hiding this comment.
Typo in comment: “deferenced” should be “dereferenced”.
5985fd7 to
c16587c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define BTCORE_DEFAULT_CONTROLLER_NAME "Game Controller" | ||
|
|
||
| /* Prevent UAF during teardown */ | ||
| static volatile gint gIsBtrCoreTerminating = 0; |
There was a problem hiding this comment.
gIsBtrCoreTerminating is declared volatile but is only accessed via g_atomic_int_* APIs. Mixing volatile with atomic operations is unnecessary and can be misleading; prefer a plain gint (or a dedicated atomic type if available) and rely on the atomic accessors for ordering/visibility.
| static volatile gint gIsBtrCoreTerminating = 0; | |
| static gint gIsBtrCoreTerminating = 0; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Invalid handle value - should never be deferenced */ | ||
| enBTRCoreRet result = BTRCore_SetAdvertisementInfo(hBTRCore, NULL, advtBeaconName); |
There was a problem hiding this comment.
Typo in comment: "deferenced" should be "dereferenced".
| @@ -2890,6 +3008,7 @@ void test_BTRCore_BatteryWriteOTAControl_NullArguments(void) { | |||
| ret = BTRCore_BatteryWriteOTAControl(NULL, 0, "some_uuid", 0); | |||
| TEST_ASSERT_EQUAL(enBTRCoreInvalidArg, ret); | |||
|
|
|||
| /* here, even before unrefering btrCoreHandle, it will return enBTRCoreInvalidArg */ | |||
There was a problem hiding this comment.
Typo in comment: "unrefering" is misspelled/unclear here; it looks like the intent is "dereferencing" (or "referencing"). Please fix to improve readability.
| /* here, even before unrefering btrCoreHandle, it will return enBTRCoreInvalidArg */ | |
| /* here, even before dereferencing btrCoreHandle, it will return enBTRCoreInvalidArg */ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); | ||
| } | ||
|
|
||
| void btrCore_SetTerminatorForTest(void) { | ||
| g_atomic_int_set(&gIsBtrCoreTerminating, 1); | ||
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); |
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); | ||
| } | ||
|
|
||
| void btrCore_SetTerminatorForTest(void) { | ||
| g_atomic_int_set(&gIsBtrCoreTerminating, 1); | ||
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/btrCore.c:1226
- The service-data logging loop iterates using apstBTDeviceInfo->saServices[count].len, but the copied length is now clamped to BTRCORE_MAX_SERVICE_DATA_LEN. If the incoming len exceeds BTRCORE_MAX_SERVICE_DATA_LEN, this loop will read past stAdServiceData[count].pcData. Iterate using the clamped length (e.g., stAdServiceData[count].len) to avoid out-of-bounds reads.
lstFoundDevice.stAdServiceData[count].len = (apstBTDeviceInfo->saServices[count].len < BTRCORE_MAX_SERVICE_DATA_LEN) ? apstBTDeviceInfo->saServices[count].len : BTRCORE_MAX_SERVICE_DATA_LEN;
MEMCPY_S(lstFoundDevice.stAdServiceData[count].pcData, BTRCORE_MAX_SERVICE_DATA_LEN, apstBTDeviceInfo->saServices[count].pcData, lstFoundDevice.stAdServiceData[count].len);
BTRCORELOG_TRACE ("ServiceData from %s\n", __FUNCTION__);
for (int i =0; i < apstBTDeviceInfo->saServices[count].len; i++){
BTRCORELOG_TRACE ("ServiceData[%d] = [%x]\n ", i, lstFoundDevice.stAdServiceData[count].pcData[i]);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); | ||
| } | ||
|
|
||
| void btrCore_SetTerminatorForTest(void) { | ||
| g_atomic_int_set(&gIsBtrCoreTerminating, 1); | ||
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/btrCore.c:1226
- The trace loop uses the unclamped service-data length (apstBTDeviceInfo->saServices[count].len) but indexes into the destination buffer (lstFoundDevice.stAdServiceData[count].pcData). When the incoming length exceeds BTRCORE_MAX_SERVICE_DATA_LEN, this will read past pcData and can crash. Iterate up to lstFoundDevice.stAdServiceData[count].len (the clamped length) or clamp the loop bound the same way.
BTRCORELOG_TRACE ("ServiceData from %s\n", __FUNCTION__);
for (int i =0; i < apstBTDeviceInfo->saServices[count].len; i++){
BTRCORELOG_TRACE ("ServiceData[%d] = [%x]\n ", i, lstFoundDevice.stAdServiceData[count].pcData[i]);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); | ||
| } | ||
|
|
||
| void btrCore_SetTerminatorForTest(void) { | ||
| g_atomic_int_set(&gIsBtrCoreTerminating, 1); | ||
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); |
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); | ||
| } | ||
|
|
||
| void btrCore_SetTerminatorForTest(void) { | ||
| g_atomic_int_set(&gIsBtrCoreTerminating, 1); | ||
| gint val = g_atomic_int_get(&gIsBtrCoreTerminating); |
Reason for change: Crash fix Test Procedure: Device deepsleep causing the crash Risks: Low Priority: P2 Signed-off-by: ppalan289 <preethi_palanisamy@comcast.com>
Reason for change: Crash fix
Test Procedure: Device deepsleep causing the crash
Risks: Low
Priority: P2