rtmessage: fix coverity RESOURCE_LEAK in rtConnection_CreateInternal #398
Conversation
- Coverity issue ID: 111 (line 555) - Fix generated by RDKDevPilot AI Bot - Add cleanup for mutexes, condition variable, and buffers on allocation failures
There was a problem hiding this comment.
Pull request overview
This PR fixes a Coverity-detected resource leak in the rtConnection_CreateInternal function by adding proper cleanup of mutexes, condition variables, and memory when buffer allocations fail. The changes ensure that when either send_buffer or recv_buffer allocation fails, all previously initialized pthread synchronization primitives are properly destroyed before returning an error.
Key Changes
- Added cleanup code for
send_bufferallocation failure (lines 552-559) to destroy 3 mutexes, 1 condition variable, and free the connection structure - Added cleanup code for
recv_bufferallocation failure (lines 562-570) to free thesend_buffer, destroy 3 mutexes, 1 condition variable, and free the connection structure - Ensures no resource leaks occur during early initialization failures
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pthread_mutex_destroy(&c->mutex); | ||
| pthread_mutex_destroy(&c->callback_message_mutex); | ||
| pthread_mutex_destroy(&c->reconnect_mutex); | ||
| pthread_cond_destroy(&c->callback_message_cond); |
There was a problem hiding this comment.
[nitpick] The order of destroying mutexes and condition variables is inconsistent with the existing cleanup pattern in the codebase. At lines 770-773, the existing destroy function uses the order: mutex, callback_message_mutex, callback_message_cond, reconnect_mutex. For consistency, consider destroying the condition variable between callback_message_mutex and reconnect_mutex instead of after all three mutexes.
| pthread_mutex_destroy(&c->mutex); | ||
| pthread_mutex_destroy(&c->callback_message_mutex); | ||
| pthread_mutex_destroy(&c->reconnect_mutex); | ||
| pthread_cond_destroy(&c->callback_message_cond); |
There was a problem hiding this comment.
[nitpick] The order of destroying mutexes and condition variables is inconsistent with the existing cleanup pattern in the codebase. At lines 770-773, the existing destroy function uses the order: mutex, callback_message_mutex, callback_message_cond, reconnect_mutex. For consistency, consider destroying the condition variable between callback_message_mutex and reconnect_mutex instead of after all three mutexes.
Address Copilot review feedback: - Destroy condition variable between callback_message_mutex and reconnect_mutex - Matches existing cleanup pattern at lines 770-773
|
Thank you @copilot-pull-request-reviewer for the excellent review! 🙏 Issue identified: Cleanup order inconsistency You're absolutely right - the existing cleanup pattern at lines 770-773 destroys resources in this order:
I've updated both cleanup blocks (lines 553-556 and 564-567) to follow the same pattern for consistency. ✅ Fixed in commit 8165c75 |
|
@svc-rdkeportal01 I've opened a new pull request, #399, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Fix Coverity RESOURCE_LEAK in rtConnection_CreateInternal
In the original code, when
send_bufferallocation fails at line 551 orrecv_bufferallocation fails at line 554, the function returns immediately without cleaning up previously allocated resources. This causes multiple resource leaks:c(allocated at line 524)send_buffer(when recv_buffer allocation fails)The fix adds proper cleanup before returning on allocation failures:
This ensures no resources are leaked when allocation failures occur during connection initialization.
Coverity Defect: Line 555 in
src/rtmessage/rtConnection.c, functionrtConnection_CreateInternal()Coverity Issue ID: 111 (from Confluence wiki)