Feature/rdkemw 9528 #250
Feature/rdkemw 9528 #250
Conversation
Reason for change : From traces, in race conditions the sequence of rbus async call causes double free Change the sequence to align with rbus code expectation . Harden the thread syncronization related to report upload status
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
Signed-off-by: Yogeswaran K <yogeswaran_k@comcast.com>
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix memory management and thread safety issues in the publishReportUploadStatus function by reordering resource cleanup operations and mutex unlocking. However, the changes introduce critical bugs rather than fixing issues.
Changes:
- Moved
rbusValue_Releaseto execute immediately afterrbusObject_SetValueinstead of after sending the async response - Reordered mutex unlock to occur after resource cleanup instead of before it
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rbusValue_Init(&value); | ||
| rbusValue_SetString(value, status); | ||
| rbusObject_SetValue(outParams, "UPLOAD_STATUS", value); | ||
| rbusValue_Release(value); // FIX: Release value immediately after SetValue to prevent double-free |
There was a problem hiding this comment.
The comment states "Release value immediately after SetValue to prevent double-free" but this is misleading. There is no double-free issue in the original code - each resource was released exactly once. The actual issue with this change is that releasing the value too early may cause rbusMethod_SendAsyncResponse to reference freed memory, since rbusObject_SetValue likely does not create a copy of the value. The comment should be removed or corrected.
| rbusValue_Release(value); // FIX: Release value immediately after SetValue to prevent double-free | |
| rbusValue_Release(value); // Release local reference; outParams now holds its own reference to this value |
| pthread_mutex_unlock(&asyncMutex); | ||
| rbusValue_Release(value); | ||
| rbusObject_Release(outParams); | ||
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above |
There was a problem hiding this comment.
The comment "Only release outParams, value is already released above" is correct but reinforces an incorrect change. The value should not have been released early (see comment on line 895).
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above | ||
| pthread_mutex_unlock(&asyncMutex); // FIX: Unlock mutex after cleanup to prevent race conditions |
There was a problem hiding this comment.
The comment "Unlock mutex after cleanup to prevent race conditions" is incorrect. Moving the mutex unlock after cleanup does not prevent race conditions; instead, it unnecessarily extends the critical section. The mutex protects the shared variable onDemandReportCallBackHandler (set at line 944, used at line 898, cleared at line 907). Once that variable is cleared at line 907, the critical section should end and the mutex should be unlocked. The resource cleanup operations don't need mutex protection. The original ordering (unlock before cleanup) was correct.
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above | |
| pthread_mutex_unlock(&asyncMutex); // FIX: Unlock mutex after cleanup to prevent race conditions | |
| pthread_mutex_unlock(&asyncMutex); // Unlock once shared handler is cleared | |
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above |
| rbusValue_Init(&value); | ||
| rbusValue_SetString(value, status); | ||
| rbusObject_SetValue(outParams, "UPLOAD_STATUS", value); | ||
| rbusValue_Release(value); // FIX: Release value immediately after SetValue to prevent double-free |
There was a problem hiding this comment.
Releasing the value immediately after rbusObject_SetValue may be premature. Throughout the codebase (lines 959-972, 1000-1016, 1130-1151), the consistent pattern is to release the value AFTER the object containing it has been used (e.g., after rbusMethod_SendAsyncResponse or rbusEvent_Publish). This is likely because rbusObject_SetValue may not create a copy of the value, so releasing it early could cause the object to reference freed memory. The original code that released value after SendAsyncResponse (at line 909 old) was correct.
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above | ||
| pthread_mutex_unlock(&asyncMutex); // FIX: Unlock mutex after cleanup to prevent race conditions |
There was a problem hiding this comment.
The mutex unlock has been moved to after resource cleanup, which unnecessarily extends the critical section. The mutex (asyncMutex) protects the shared state onDemandReportCallBackHandler, which is already set to NULL at line 907. The resource cleanup operations (rbusObject_Release) do not need mutex protection and should not be performed while holding the lock. This increases lock contention and could impact performance. The mutex should be unlocked immediately after line 907, before the cleanup operations, as it was in the original code.
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above | |
| pthread_mutex_unlock(&asyncMutex); // FIX: Unlock mutex after cleanup to prevent race conditions | |
| pthread_mutex_unlock(&asyncMutex); | |
| rbusObject_Release(outParams); // FIX: Only release outParams, value is already released above |
No description provided.