Skip to content

Conversation

@svc-rdkeportal01
Copy link

@svc-rdkeportal01 svc-rdkeportal01 commented Nov 29, 2025

Fix Coverity RESOURCE_LEAK in rtMessage Unit Test

Coverity Issues Fixed

This PR fixes two related Coverity RESOURCE_LEAK defects in unittests/rbus_unit_test_server.cpp:

  • Coverity CID 115 (line 1260): TestServer_rtmsg_rtMessage_SetMessage_test1_Test::TestBody()
  • Coverity CID 116 (line 1260): TestServer_rtmsg_rtMessage_SetMessage_test1_Test::TestBody()

Note: These are Coverity defect IDs, not GitHub issue numbers.

Root Cause

The test function creates an rtMessage object named item at line 1211:

rtMessage_Create(&item);

Later, at line 1233, the pointer is reassigned:

err = rtMessage_GetMessage(req, "config", &item);

This reassignment leaks the original allocation from line 1211. While the function has cleanup at the end (line 1257), it only releases the reassigned pointer, not the original allocation.

Leak Path:

  1. Line 1211: rtMessage_Create(&item) - Allocates item Free the memory leaks from rbus gtest - Session & msg #1
  2. Line 1233: rtMessage_GetMessage(req, "config", &item) - Overwrites pointer, leaking item Free the memory leaks from rbus gtest - Session & msg #1
  3. Line 1257: rtMessage_Release(item) - Only releases item RDKB-44513 : Free the memory leaks from rbus gtest #2

Changes Made

Added rtMessage_Release(item) before the pointer reassignment:

// Line 1233 - Release original before reassignment
rtMessage_Release(item);  // Release original item before reassignment
err = rtMessage_GetMessage(req, "config", &item);

This ensures:

  • ✅ Original item is properly released
  • ✅ No memory leak when pointer is reassigned
  • ✅ Existing cleanup at end still works for reassigned pointer

Why Both CIDs?

Coverity detected this as two separate issues (CID 115 and 116) because there are two leak paths:

  1. The original allocation not being freed
  2. The pointer reassignment causing the leak

Both are fixed by this single change.


Coverity Defect Details:

  • CID 115: Line 1260 in unittests/rbus_unit_test_server.cpp
  • CID 116: Line 1260 in unittests/rbus_unit_test_server.cpp
  • Function: TestServer_rtmsg_rtMessage_SetMessage_test1_Test::TestBody()
  • Coverity Checker: RESOURCE_LEAK

Fixes Coverity defects CID 115, 116 (not GitHub issues)
Fix generated by RDKDevPilot AI Bot with pattern validation

Root Cause:
The test function TestServer_rtmsg_rtMessage_SetMessage_test1_Test::TestBody()
creates an rtMessage object 'item' at line 1211, but line 1233 reassigns the
pointer via rtMessage_GetMessage(), leaking the original allocation.

While the function has cleanup at the end (line 1257), it only releases the
reassigned pointer, not the original allocation from line 1211.

Changes:
- Add rtMessage_Release(item) before pointer reassignment at line 1233
- Prevents leak of original rtMessage object
- Maintains existing cleanup at function end

Coverity CID 115: Line 1260, function TestServer_rtmsg_rtMessage_SetMessage_test1_Test::TestBody()
Coverity CID 116: Line 1260, function TestServer_rtmsg_rtMessage_SetMessage_test1_Test::TestBody()

Validation scores: Pipeline 95/100, Pattern 100/100
@svc-rdkeportal01 svc-rdkeportal01 requested a review from a team as a code owner November 29, 2025 23:23
Copilot AI review requested due to automatic review settings November 29, 2025 23:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR attempts to fix Coverity RESOURCE_LEAK defects (CID 115 and 116) in the rtMessage unit test by adding an rtMessage_Release(item) call before a pointer reassignment. While the PR correctly identifies a potential memory leak path, the fix introduces a critical double-free bug.

Key Issues:

  • The fix adds rtMessage_Release(item) before rtMessage_GetMessage (line 1234), which is expected to fail in this negative test case
  • When rtMessage_GetMessage fails, it doesn't modify the item pointer, leaving it dangling after the release
  • The existing cleanup code at line 1255 then calls rtMessage_Release(item) again on the already-freed memory, causing a double-free

Recommended Solution:
Set item = NULL after releasing it and add NULL checks in cleanup, or use a separate variable for the negative test case.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

err = rtMessage_SetMessage(NULL, "params", item);
EXPECT_EQ(err, RT_ERROR_INVALID_ARG) << "rtMessage_SetMessage failed";
//Neg test passing invalid param
rtMessage_Release(item); // Release original item before reassignment
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

Double-free bug introduced by this fix: After releasing item here, the subsequent rtMessage_GetMessage call at line 1234 is expected to fail (returning RT_PROPERTY_NOT_FOUND). When rtMessage_GetMessage fails, it returns early without modifying the output parameter (see src/rtmessage/rtMessage.c:411-412), leaving item as a dangling pointer to already-freed memory. This dangling pointer is then passed to rtMessage_Release(item) again at line 1255, causing a double-free.

Recommended fix: Set item = NULL after releasing it, and add a NULL check before the release at line 1255:

rtMessage_Release(item);
item = NULL;  // Prevent double-free
err = rtMessage_GetMessage(req, "config", &item);
EXPECT_EQ(err, RT_PROPERTY_NOT_FOUND) << "rtMessage_GetMessage failed";

Then at line 1255:

if (item) rtMessage_Release(item);

Alternatively, use a separate variable for this negative test instead of reusing item.

Suggested change
rtMessage_Release(item); // Release original item before reassignment
if(item) rtMessage_Release(item); // Release original item before reassignment
item = NULL; // Prevent double-free

Copilot uses AI. Check for mistakes.
Copilot correctly identified that our initial fix introduced a double-free:
- rtMessage_GetMessage() at line 1234 is a negative test that FAILS
- When it fails, it doesn't modify the item pointer
- After releasing item at line 1233, item becomes a dangling pointer
- Line 1255 would then try to release the already-freed memory

Changes:
- Set item = NULL after releasing to prevent dangling pointer
- Add NULL check before final release: if (item) rtMessage_Release(item)

This ensures:
- Original item is properly released before reassignment
- No double-free if rtMessage_GetMessage fails
- Safe cleanup at function end

Addresses Copilot comment on line 1233
@svc-rdkeportal01
Copy link
Author

✅ Fixed in commit 5a311f0

Added:

  • item = NULL; after release
  • NULL check before final release

Prevents double-free when rtMessage_GetMessage fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants