Skip to content

Conversation

@svc-rdkeportal01
Copy link

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

Fix Coverity RESOURCE_LEAK in Discovery Commands

Coverity Issues Fixed

This PR fixes three related Coverity RESOURCE_LEAK defects in utils/rbuscli/rbuscli.c:

  • Coverity CID 112 (line 1004): execute_discover_component_cmd
  • Coverity CID 113 (line 1093): execute_discover_wildcard_dests_cmd
  • Coverity CID 114 (line 1049): execute_discover_elements_cmd

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

Root Cause

All three functions had similar resource leak patterns:

  1. CID 112: Memory allocated by rbus_discoverComponentName() was not freed in the error path
  2. CID 113: Uninitialized destinations pointer + no cleanup when rbus_discoverWildcardDestinations() fails
  3. CID 114: Memory potentially allocated by rbus_discoverComponentDataElements() even when numElements == 0, not freed in error path

Changes Made

For each function, the fix includes:

  1. Initialize pointers to NULL to prevent undefined behavior

    char **pComponentNames = NULL;
    char **pElementNames = NULL;
    char **destinations = NULL;
  2. Add cleanup in error paths to prevent memory leaks

    free(pointer);
  3. Add return statements to prevent fall-through to unintended code paths


Coverity Defect Details:

  • CID 112: Line 1004 in utils/rbuscli/rbuscli.c, function execute_discover_component_cmd()
  • CID 113: Line 1093 in utils/rbuscli/rbuscli.c, function execute_discover_wildcard_dests_cmd()
  • CID 114: Line 1049 in utils/rbuscli/rbuscli.c, function execute_discover_elements_cmd()

Coverity Checker: RESOURCE_LEAK

Fixes Coverity defects CID 112, 113, 114 (not GitHub issues)
Fix generated by RDKDevPilot AI Bot with pattern validation

Fixes resource leaks in three discovery command functions:
1. execute_discover_component_cmd (line 1004) - Coverity CID 112
2. execute_discover_elements_cmd (line 1049) - Coverity CID 114
3. execute_discover_wildcard_dests_cmd (line 1093) - Coverity CID 113

Changes:
- Initialize pointers to NULL to avoid undefined behavior
- Add free() in error paths to prevent memory leaks
- Add return statements to prevent fall-through

Pattern validation applied (learned from Copilot feedback):
- Pointer initialization pattern (PR #396)
- NULL check before free pattern (PR #396)
- Return after cleanup pattern (PR #396)

Validation scores: Pipeline 95/100, Pattern 70-84/100
@svc-rdkeportal01 svc-rdkeportal01 requested a review from a team as a code owner November 29, 2025 22:42
Copilot AI review requested due to automatic review settings November 29, 2025 22:42
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 fixes three Coverity RESOURCE_LEAK defects in the rbuscli tool by adding proper cleanup in error paths for discovery command functions. The changes add pointer initialization to NULL and cleanup code when RBUS discovery API calls fail, preventing potential memory leaks.

Key changes:

  • Initialize pointer variables to NULL to prevent undefined behavior in error paths
  • Add memory cleanup in error paths for three discovery functions
  • Add explicit return statements after error handling to prevent fall-through

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

Comment on lines 1102 to 1105
if (destinations != NULL)
{
free(destinations);
}
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.

[nitpick] The NULL check before free() is unnecessary and inconsistent with the rest of the codebase. In C, free(NULL) is well-defined and safe. The rest of this file calls free() directly without NULL checks (e.g., lines 953, 955, 991, 993, 1041, 1043, 1090, 1092).

Consider simplifying to:

free(destinations);
Suggested change
if (destinations != NULL)
{
free(destinations);
}
free(destinations);

Copilot uses AI. Check for mistakes.
Comment on lines 1003 to 1006
if (pComponentNames != NULL)
{
free(pComponentNames);
}
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.

[nitpick] The NULL check before free() is unnecessary and inconsistent with the rest of the codebase. In C, free(NULL) is well-defined and safe. The rest of this file calls free() directly without NULL checks (e.g., lines 953, 955, 991, 993, 1041, 1043, 1090, 1092).

Consider simplifying to:

free(pComponentNames);
Suggested change
if (pComponentNames != NULL)
{
free(pComponentNames);
}
free(pComponentNames);

Copilot uses AI. Check for mistakes.
Comment on lines 1053 to 1056
if (pElementNames != NULL)
{
free(pElementNames);
}
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.

[nitpick] The NULL check before free() is unnecessary and inconsistent with the rest of the codebase. In C, free(NULL) is well-defined and safe. The rest of this file calls free() directly without NULL checks (e.g., lines 953, 955, 991, 993, 1041, 1043, 1090, 1092).

Consider simplifying to:

free(pElementNames);
Suggested change
if (pElementNames != NULL)
{
free(pElementNames);
}
free(pElementNames);

Copilot uses AI. Check for mistakes.
Copilot correctly pointed out that:
- free(NULL) is well-defined and safe in C
- The rest of the codebase calls free() directly without NULL checks
- Our NULL checks were inconsistent with codebase style

Changes:
- Simplified all 3 cleanup blocks from 5 lines to 1 line
- Now consistent with rest of rbuscli.c (lines 953, 955, 991, 993, etc.)
- Maintains same safety (free(NULL) is safe)

Addresses Copilot comments on lines 1006, 1056, 1105
@svc-rdkeportal01
Copy link
Author

Response to Copilot Review

Thank you @copilot-pull-request-reviewer for the excellent feedback! 🙏

Changes Made

I've addressed all 3 nitpick comments by removing the unnecessary NULL checks before free().

You're absolutely right:

  • free(NULL) is well-defined and safe in C standard
  • ✅ The rest of rbuscli.c calls free() directly (lines 953, 955, 991, 993, 1041, 1043, 1090, 1092)
  • ✅ Our NULL checks were inconsistent with codebase style

Changes:

// Before (5 lines):
if (pComponentNames != NULL)
{
    free(pComponentNames);
}

// After (1 line):
free(pComponentNames);

Applied to all 3 functions:

  • execute_discover_component_cmd (line 1003)
  • execute_discover_elements_cmd (line 1050)
  • execute_discover_wildcard_dests_cmd (line 1102)

Result:

  • ✅ Simpler code (removed 12 lines)
  • ✅ Consistent with codebase style
  • ✅ Same safety guarantees

The fix now matches the coding style of the rest of the file. Ready for re-review!

Copy link

Copilot AI commented Nov 29, 2025

@svc-rdkeportal01 I've opened a new pull request, #402, to work on those changes. Once the pull request is ready, I'll request review from you.

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.

3 participants