Skip to content

RDKEMW-12176: btrCore_PopulateListOfPairedDevCrash #60

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.

Sign up for GitHub

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

Open
PreethiLakshmi91 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

RDKEMW-12176: btrCore_PopulateListOfPairedDevCrash #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions include/btrCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,13 @@ enBTRCoreRet BTRCore_RegisterConnectionIntimationCb (tBTRCoreHandle hBTRCore, fP
/* BTRCore_RegisterConnectionAuthenticationCallback - callback for receiving a connection request from another device */
enBTRCoreRet BTRCore_RegisterConnectionAuthenticationCb (tBTRCoreHandle hBTRCore, fPtr_BTRCore_ConnAuthCb afpcBBTRCoreConnAuth, void* apUserData);

#ifdef UNIT_TEST
gint btrCore_AddAndGetCurrGenForTest(void);
Comment on lines 1366 to +1368
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btrCore_AddAndGetCurrGenForTest is declared in the public btrCore.h API surface, but its name/behavior are test-only and it mutates global state (gBtrCoreGenerationCounter). This risks external callers accidentally changing teardown behavior and also couples production ABI to unit-test helpers. Consider moving this declaration to a unit-test-only header (or guarding it with a build define like #ifdef UNIT_TEST), and/or making the symbol non-exported in non-test builds.

Suggested change
gint btrCore_AddAndGetCurrGenForTest(void);
#ifdef UNIT_TEST
gint btrCore_AddAndGetCurrGenForTest(void);
#endif

Copilot uses AI. Check for mistakes.
gint btrCore_GetTerminatorForTest(void);
void btrCore_SetTerminatorForTest(void);
void btrCore_ResetTerminatorForTest(void);
Comment on lines 1366 to +1371
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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 */

Copilot uses AI. Check for mistakes.
#endif

/* @} */ //BLUETOOTH_APIS

#ifdef __cplusplus
Expand Down
55 changes: 52 additions & 3 deletions src/btrCore.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ int b_rdk_logger_enabled = 0;
#define BTRCORE_GOOGLE_OUI_LENGTH 8
#define BTCORE_DEFAULT_CONTROLLER_NAME "Game Controller"

/* Prevent UAF during teardown */
static volatile gint gIsBtrCoreTerminating = 0;
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
static volatile gint gIsBtrCoreTerminating = 0;
static gint gIsBtrCoreTerminating = 0;

Copilot uses AI. Check for mistakes.

Comment on lines +77 to +79
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
/* Track active instance generation - helps to check if current handle is same as that of terminated handle. */
static gint gBtrCoreGenerationCounter = 0;

static char * BTRCORE_REMOTE_OUI_VALUES[] = {
"20:44:41", //LC103
"E8:0F:C8", //EC302
Expand Down Expand Up @@ -214,6 +220,7 @@ typedef struct _stBTRCoreHdl {
GCond hidNameWaitCond;
BOOLEAN hidNameWaitInitialized;
stBTRCorePendingHidNameInfo stPendingHidNameInfo[BTRCORE_MAX_NUM_BT_DISCOVERED_DEVICES];
gint generation;
} stBTRCoreHdl;

typedef struct _stBTRCoreHidNameTimeoutData {
Expand Down Expand Up @@ -278,6 +285,27 @@ STATIC enBTRCoreRet btrCore_BTMediaStatusUpdateCb (stBTRCoreAVMediaStatusUpdate
#endif
STATIC enBTRCoreRet btrCore_BTLeStatusUpdateCb (stBTRCoreLeGattInfo* apstBtrLeInfo, const char* apcBtdevAddr, void* apvUserData);

#ifdef UNIT_TEST
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);
}
Comment on lines +289 to +296
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

void btrCore_ResetTerminatorForTest(void) {
g_atomic_int_set(&gIsBtrCoreTerminating, 0);
gint val = g_atomic_int_get(&gIsBtrCoreTerminating);
}

void btrCore_SetTerminatorForTest(void) {
g_atomic_int_set(&gIsBtrCoreTerminating, 1);
gint val = g_atomic_int_get(&gIsBtrCoreTerminating);
Comment on lines +300 to +305
Comment on lines +300 to +305
Comment on lines +300 to +305
Comment on lines +300 to +305
Comment on lines +300 to +305
}
#endif

/* Static Function Definition */
static void
btrCore_InitDataSt (
Expand Down Expand Up @@ -1189,8 +1217,8 @@ btrCore_AddDeviceToScannedDevicesArr (

if(0 != apstBTDeviceInfo->saServices[count].len)
{
lstFoundDevice.stAdServiceData[count].len = apstBTDeviceInfo->saServices[count].len;
MEMCPY_S(lstFoundDevice.stAdServiceData[count].pcData,lstFoundDevice.stAdServiceData[count].len, apstBTDeviceInfo->saServices[count].pcData, BTRCORE_MAX_SERVICE_DATA_LEN);
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++){
Expand Down Expand Up @@ -1482,9 +1510,21 @@ btrCore_PopulateListOfPairedDevices (
stBTPairedDeviceInfo* pstBTPairedDeviceInfo = NULL;
stBTRCoreBTDevice knownDevicesArr[BTRCORE_MAX_NUM_BT_DEVICES];

if (!apsthBTRCore) {
BTRCORELOG_WARN("apsthBTRCore is null\n");
return enBTRCoreNotInitialized;
}

/* Prevent UAF when worker threads run during teardown */
if(g_atomic_int_get(&gIsBtrCoreTerminating)) {
BTRCORELOG_WARN("btrCore: Ignoring PopulateListOfPairedDevices during termination\n");
Comment on lines +1518 to +1520
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/* 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");

Copilot uses AI. Check for mistakes.
return enBTRCoreFailure;
}
Comment on lines +1519 to +1522
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

if ((pstBTPairedDeviceInfo = g_malloc0(sizeof(stBTPairedDeviceInfo))) == NULL)
if ((pstBTPairedDeviceInfo = g_malloc0(sizeof(stBTPairedDeviceInfo))) == NULL) {
BTRCORELOG_WARN("btrCore: gmalloc0 failed\n");
return enBTRCoreFailure;
}


pstBTPairedDeviceInfo->numberOfDevices = 0;
Expand Down Expand Up @@ -3524,6 +3564,10 @@ BTRCore_Init (
}
MEMSET_S(pstlhBTRCore, sizeof(stBTRCoreHdl), 0, sizeof(stBTRCoreHdl));

/* Assign a new generation for this instance */
pstlhBTRCore->generation = g_atomic_int_add(&gBtrCoreGenerationCounter, 1)+1;

g_atomic_int_set(&gIsBtrCoreTerminating, 0);

Comment on lines +3570 to 3571
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
g_atomic_int_set(&gIsBtrCoreTerminating, 0);

Copilot uses AI. Check for mistakes.
pstlhBTRCore->connHdl = BtrCore_BTInitGetConnection();
if (!pstlhBTRCore->connHdl) {
Expand Down Expand Up @@ -3688,6 +3732,11 @@ BTRCore_DeInit (

BTRCORELOG_INFO ("hBTRCore = %8p\n", hBTRCore);

/* Only end global teardown if this is the active generation */
if (pstlhBTRCore->generation == g_atomic_int_get(&gBtrCoreGenerationCounter)) {
g_atomic_int_set(&gIsBtrCoreTerminating, 1);
}
Comment on lines +3735 to +3738
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The teardown gating logic depends on pstlhBTRCore->generation == g_atomic_int_get(&gBtrCoreGenerationCounter), but with the current Init assignment this comparison likely never succeeds. This means gIsBtrCoreTerminating may remain 0 during teardown, defeating the added UAF prevention in btrCore_PopulateListOfPairedDevices. Fix the generation bookkeeping (Init and/or this comparison) so the active instance reliably sets gIsBtrCoreTerminating at the start of BTRCore_DeInit.

Suggested change
/* Only end global teardown if this is the active generation */
if (pstlhBTRCore->generation == g_atomic_int_get(&gBtrCoreGenerationCounter)) {
g_atomic_int_set(&gIsBtrCoreTerminating, 1);
}
/* Mark global teardown in progress to prevent UAF in other threads */
g_atomic_int_set(&gIsBtrCoreTerminating, 1);

Copilot uses AI. Check for mistakes.

if (pstlhBTRCore->hidNameWaitInitialized) {
GThread* lapPendingThreads[BTRCORE_MAX_NUM_BT_DISCOVERED_DEVICES];
int liNumPendingThreads = 0;
Expand Down
Loading
Loading