Skip to content

RDKEMW-15927: Addressing the occurring race condition while deleting all report profiles #308

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
yogeswaransky wants to merge 1 commit into
base: support/1.8.5
Choose a base branch
from
+19 −0
Open

RDKEMW-15927: Addressing the occurring race condition while deleting all report profiles #308

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
10 changes: 10 additions & 0 deletions source/bulkdata/profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1159,14 +1159,23 @@ T2ERROR deleteAllProfiles(bool delFromDisk)
int count = 0;
int profileIndex = 0;
Profile *tempProfile = NULL;
static bool deleteInProgress = false;

pthread_mutex_lock(&plMutex);
if(deleteInProgress)
{
T2Info("deleteAllProfiles already in progress, skipping\n");
pthread_mutex_unlock(&plMutex);
return T2ERROR_SUCCESS;
Comment on lines +1165 to +1169
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

The new deleteInProgress early-return changes the function’s semantics from “delete completes before return” to “may return while another thread is still deleting”. This is dangerous for callers like uninitProfileList() (calls deleteAllProfiles(false) and then destroys plMutex), because a concurrent in-flight delete can continue using plMutex after it has been destroyed, leading to undefined behavior/crash.

Instead of returning success when deleteInProgress is true, either (a) wait for the active deletion to complete (e.g., pthread_cond_wait under plMutex and pthread_cond_broadcast when clearing the flag), or (b) return an error code and update all callers to handle it safely (including uninitProfileList).

Suggested change
if(deleteInProgress)
{
T2Info("deleteAllProfiles already in progress, skipping\n");
pthread_mutex_unlock(&plMutex);
return T2ERROR_SUCCESS;
if (deleteInProgress)
{
T2Info("deleteAllProfiles already in progress, waiting for completion\n");
/* Wait until the ongoing delete operation completes.
* This restores the guarantee that deleteAllProfiles does not
* return while another thread is still deleting and using plMutex.
*/
while (deleteInProgress)
{
pthread_mutex_unlock(&plMutex);
/* Sleep briefly to avoid busy-waiting while allowing the
* active deleter to make progress.
*/
usleep(1000);
pthread_mutex_lock(&plMutex);
}

Copilot uses AI. Check for mistakes.
Comment on lines +1162 to +1169
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

deleteInProgress is set/reset without any test coverage to verify the new behavior (re-entrant calls, waiting/skipping behavior, and interaction with teardown paths like uninitProfileList). Since this change is specifically addressing a race condition, please add unit tests that exercise concurrent/overlapping deleteAllProfiles calls (e.g., block the first call mid-way via mocks and verify the second call’s behavior).

Copilot generated this review using guidance from repository custom instructions.
}

if(profileList == NULL)
{
T2Error("profile list is not initialized yet or profileList is empty, ignoring\n");
pthread_mutex_unlock(&plMutex);
return T2ERROR_FAILURE;
}
deleteInProgress = true;

count = Vector_Size(profileList);
pthread_mutex_unlock(&plMutex);
Expand Down Expand Up @@ -1216,6 +1225,7 @@ T2ERROR deleteAllProfiles(bool delFromDisk)
Vector_Destroy(profileList, freeProfile);
profileList = NULL;
Vector_Create(&profileList);
deleteInProgress = false;
pthread_mutex_unlock(&plMutex);

T2Debug("%s --out\n", __FUNCTION__);
Expand Down
9 changes: 9 additions & 0 deletions source/ccspinterface/rbusInterface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,17 @@ rbusError_t eventSubHandler(rbusHandle_t handle, rbusEventSubAction_t action, co
}

#if defined(PRIVACYMODES_CONTROL)
static pthread_mutex_t privacyWorkerMutex = PTHREAD_MUTEX_INITIALIZER;

/**
* Worker thread to handle privacy mode callbacks off the RBUS handler thread.
* Prevents RBUS handler starvation by moving heavy operations (profile deletion,
* XConf restart) to a detached thread.
*
* Serialized via privacyWorkerMutex to prevent concurrent invocations racing on
* profile deletion when PrivacyMode is toggled rapidly (e.g. DO_NOT_SHARE ->
* SHARE -> DO_NOT_SHARE in quick succession), which causes double-free of
* grepSeekProfile in deleteAllProfiles().
*/
static void* privacyModeCallbackWorker(void *arg)
{
Expand All @@ -395,6 +402,7 @@ static void* privacyModeCallbackWorker(void *arg)
return NULL;
}
T2Debug("%s ++in mode=%s\n", __FUNCTION__, mode);
pthread_mutex_lock(&privacyWorkerMutex);
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Serializing the entire worker with a global mutex can cause unbounded accumulation of detached threads if PrivacyMode is toggled rapidly: each event still spawns a new thread, and additional threads will block on privacyWorkerMutex while holding full thread stacks. Consider coalescing events (single long-lived worker thread + queue), or using pthread_mutex_trylock/an atomic “worker running” flag to drop or merge redundant invocations rather than creating another thread that will just wait.

Suggested change
pthread_mutex_lock(&privacyWorkerMutex);
/* Use trylock to avoid blocking multiple detached threads on the
* global mutex. If another worker is already running, drop this
* redundant invocation to prevent unbounded accumulation of waiting
* threads (each holding a full stack).
*/
if(pthread_mutex_trylock(&privacyWorkerMutex) != 0)
{
T2Debug("%s: privacyWorkerMutex busy, skipping redundant privacyModeCallbackWorker for mode=%s\n",
__FUNCTION__, mode);
free(mode);
return NULL;
}

Copilot uses AI. Check for mistakes.

if(strcmp(mode, "DO_NOT_SHARE") == 0)
{
Expand Down Expand Up @@ -424,6 +432,7 @@ static void* privacyModeCallbackWorker(void *arg)
}

free(mode);
pthread_mutex_unlock(&privacyWorkerMutex);
T2Debug("%s --out\n", __FUNCTION__);
return NULL;
}
Expand Down
Loading