RDK-60308-[tr69hostif, RFC] RDK Coverity Defect Resolution for Device Management #335

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

Jump to bottom

Open

Vismalskumar0 wants to merge 10 commits into develop from topic/RDK-60308

Contributor

Vismalskumar0 commented Jan 14, 2026

Check return value of pthread_cond_timedwait for ETIMEDOUT instead of comparing tv_sec values.

Fix Coverity #2, #3, #4, #5, #20-30, #101: Multiple defects

RDKECMF-216 Fix names in native_full_build workflow #2: INTEGER_OVERFLOW - Add overflow check before pointer arithmetic
1.0.6 release changelog updates #3, 1.0.6 release changelog updates #4: COPY_INSTEAD_OF_MOVE - Use std::move for RValue references
RDK-55468 : tr69hsotif L2 test framework #5: DEADCODE - Remove redundant null check after direct assignment
rebase from develop #20-30: NO_EFFECT - Remove redundant error checks before ERR_CHK calls
1.1.2 release changelog updates #101: RESOURCE_LEAK - Add cleanup for pipe2/fp2 in executeRfcMgr

Fix Coverity #11: LOCK_EVASION

Convert singleton instance to std::atomic for lock-free thread safety.

Fix Coverity #6: FORWARD_NULL

Add TODO comment for potential null pointer dereference issue in external libparodus dependency.

Fix Coverity #7: FORWARD_NULL

Add TODO comment for potential null pointer dereference issue in external libparodus dependency.

Fix Coverity #12: MISSING_LOCK

Protect thread-unsafe fopen calls with mutex lock.

Fix Coverity #13: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #14: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #15: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #16: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #17: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #18: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #19: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #8: MISSING_RETURN

Add default return values to test stub methods.

Fix Coverity #9, #10: MISSING_RETURN

Add default return values to test stub methods.

src/hostif/profiles/DeviceInfo/Device_DeviceInfo_ProcessStatus_Process.cpp Fix Coverity #31-40: NO_EFFECT

Remove 9 redundant error checks before ERR_CHK macro calls.

Fix Coverity #41-45: NO_EFFECT

Remove 5 redundant error checks before ERR_CHK macro calls.

Fix Coverity #46, #47, #106: Multiple defects

RDKEMW-2234: Added new MigrationStatus tr181 ReadOnly Param #46, RDK-55702: Update the MW clients to use Power Manager Plugin #47: NO_EFFECT - Remove redundant error checks before ERR_CHK
RDK-56451 [RDKE] Move tr69hostif L2 binary into common docker repo #106: REVERSE_INULL - Add null check before strcpy_s call

Fix Coverity #48-51: NO_EFFECT

Remove 4 redundant error checks before ERR_CHK macro calls.

Fix Coverity #52-60: NO_EFFECT

Remove 9 redundant error checks before ERR_CHK macro calls.

Fix Coverity #61-69: NO_EFFECT

Remove 9 redundant error checks before ERR_CHK macro calls.

Fix Coverity #70-73: NO_EFFECT

Remove 4 redundant error checks before ERR_CHK macro calls.

Fix Coverity #74-76: NO_EFFECT

Remove 3 redundant error checks before ERR_CHK macro calls.

Fix Coverity #77: NO_EFFECT

Remove redundant error check before ERR_CHK macro call.

Fix Coverity #78-82: NO_EFFECT

Remove 5 redundant error checks before ERR_CHK macro calls.

Fix Coverity #83-86: NO_EFFECT

Remove 4 redundant error checks before ERR_CHK macro calls.

Fix Coverity #100, #103, #104: RESOURCE_LEAK

Free paramValue allocated by getRbusStringParam in all code paths.

Fix Coverity #97: PW.NARROWING_CONVERSION

Use explicit struct initialization with uint32_t cast to avoid implicit narrowing conversion.


          Check return value of pthread_cond_timedwait for ETIMEDOUT instead

731d67a

of comparing tv_sec values.

Fix Coverity #2, #3, #4, #5, #20-30, #101: Multiple defects

- #2: INTEGER_OVERFLOW - Add overflow check before pointer arithmetic
- #3, #4: COPY_INSTEAD_OF_MOVE - Use std::move for RValue references
- #5: DEADCODE - Remove redundant null check after direct assignment
- #20-30: NO_EFFECT - Remove redundant error checks before ERR_CHK calls
- #101: RESOURCE_LEAK - Add cleanup for pipe2/fp2 in executeRfcMgr

Fix Coverity #11: LOCK_EVASION

Convert singleton instance to std::atomic for lock-free thread safety.

Fix Coverity #6: FORWARD_NULL

Add TODO comment for potential null pointer dereference issue in
external libparodus dependency.

Fix Coverity #7: FORWARD_NULL

Add TODO comment for potential null pointer dereference issue in
external libparodus dependency.

Fix Coverity #12: MISSING_LOCK

Protect thread-unsafe fopen calls with mutex lock.

Fix Coverity #13: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #14: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #15: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #16: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #17: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #18: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #19: RESOURCE_LEAK

Free allocated memory in notification handler error path.

Fix Coverity #8: MISSING_RETURN

Add default return values to test stub methods.

Fix Coverity #9, #10: MISSING_RETURN

Add default return values to test stub methods.

src/hostif/profiles/DeviceInfo/Device_DeviceInfo_ProcessStatus_Process.cpp
Fix Coverity #31-40: NO_EFFECT

Remove 9 redundant error checks before ERR_CHK macro calls.

Fix Coverity #41-45: NO_EFFECT

Remove 5 redundant error checks before ERR_CHK macro calls.

Fix Coverity #46, #47, #106: Multiple defects

- #46, #47: NO_EFFECT - Remove redundant error checks before ERR_CHK
- #106: REVERSE_INULL - Add null check before strcpy_s call

Fix Coverity #48-51: NO_EFFECT

Remove 4 redundant error checks before ERR_CHK macro calls.

Fix Coverity #52-60: NO_EFFECT

Remove 9 redundant error checks before ERR_CHK macro calls.

Fix Coverity #61-69: NO_EFFECT

Remove 9 redundant error checks before ERR_CHK macro calls.

Fix Coverity #70-73: NO_EFFECT

Remove 4 redundant error checks before ERR_CHK macro calls.

Fix Coverity #74-76: NO_EFFECT

Remove 3 redundant error checks before ERR_CHK macro calls.

Fix Coverity #77: NO_EFFECT

Remove redundant error check before ERR_CHK macro call.

Fix Coverity #78-82: NO_EFFECT

Remove 5 redundant error checks before ERR_CHK macro calls.

Fix Coverity #83-86: NO_EFFECT

Remove 4 redundant error checks before ERR_CHK macro calls.

Fix Coverity #100, #103, #104: RESOURCE_LEAK

Free paramValue allocated by getRbusStringParam in all code paths.

Fix Coverity #97: PW.NARROWING_CONVERSION

Use explicit struct initialization with uint32_t cast to avoid
implicit narrowing conversion.

Vismalskumar0 requested a review from a team as a code owner

January 14, 2026 08:17

Copilot AI review requested due to automatic review settings

January 14, 2026 08:17

Copilot started reviewing on behalf of Vismalskumar0

January 14, 2026 08:17

Contributor

rdkcmf-jenkins commented Jan 14, 2026

Coverity Issue - Parse recovery warning

operator -> or ->* applied to "std::atomic<XBSStore *>" instead of to a pointer type

Low Impact, CWE-none
RW.MEMBER_ACCESS_REQUIRES_POINTER

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp:271

Contributor

rdkcmf-jenkins commented Jan 14, 2026

Coverity Issue - Parse recovery warning

function "XBSStore::getAuthServicePartnerID" not emitted, consider modeling it or review parse diagnostics to improve fidelity

Low Impact, CWE-none
RW.ROUTINE_NOT_EMITTED

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp:122

rdkcmf-jenkins reviewed

View reviewed changes

src/hostif/parodusClient/pal/webpa_attribute.cpp

-                      strncpy(attr[0][i]->value,Param.paramValue, strlen(Param.paramValue));
-                      attr[0][i]->value[strlen(Param.paramValue)] = '\0';
+              	int ret = get_AttribValues_tr69hostIf(&Param);
+                      if(ret == 0 && Param.paramValue != NULL)

Contributor

rdkcmf-jenkins Jan 14, 2026

Coverity Issue - Array compared against 0

Comparing an array to null is not useful: "Param.paramValue != NULL", since the test will always evaluate as true.

Medium Impact, CWE-398
NO_EFFECT

How to fix

Was "Param.paramValue" formerly declared as a pointer?

Contributor

rdkcmf-jenkins commented Jan 14, 2026

Coverity Issue - Parse recovery warning

operator -> or ->* applied to "std::atomic<XBSStore *>" instead of to a pointer type

Low Impact, CWE-none
RW.MEMBER_ACCESS_REQUIRES_POINTER

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp:277

Contributor

rdkcmf-jenkins commented Jan 14, 2026

Coverity Issue - Parse recovery warning

operator -> or ->* applied to "std::atomic<XBSStore *>" instead of to a pointer type

Low Impact, CWE-none
RW.MEMBER_ACCESS_REQUIRES_POINTER

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp:276

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

src/hostif/handlers/src/hostIf_IPClient_ReqHandler.cpp

Comment on lines +492 to +499

+                          hostIf_IPInterface* ipIfInst = hostIf_IPInterface::getInstance(ifindexes[i]);
+                          if (ipIfInst) {
+                              int ipv4AddressNumberOfEntries = ipIfInst->getIPv4AddressNumberOfEntries();
+                              RDK_LOG (RDK_LOG_DEBUG, LOG_TR69HOSTIF, "[%s:%s:%d] ipv4AddressNumberOfEntries = %d, curNumOfInterfaceIPv4Addresses[%d] = %d\n",
+                                  __FILE__, __FUNCTION__, __LINE__, ipv4AddressNumberOfEntries, ifindexes[i], curNumOfInterfaceIPv4Addresses[ifindexes[i]]);
+                              sprintf (objectPath, "Device.IP.Interface.%d.IPv4Address.", ifindexes[i]);
+                              sendAddRemoveEvents (mUpdateCallback, ipv4AddressNumberOfEntries, curNumOfInterfaceIPv4Addresses[ifindexes[i]], objectPath);
+                          }

Copilot AI Jan 14, 2026

The null check is only added for the IPv4 case but not for the IPv6 case. Lines 503 and 509 still call hostIf_IPInterface::getInstance(ifindexes[i]) without checking if the returned pointer is null before calling methods on it. This creates an inconsistency where the IPv4 code path is protected but the IPv6 code path is not.

Copilot uses AI. Check for mistakes.

Contributor Author

Vismalskumar0 Jan 22, 2026

@copilot open a new pull request to apply changes based on this feedback

Vismalskumar0 added 2 commits

January 22, 2026 12:57


          Merge pull request #339 from rdkcentral/develop

Rebase


          Update hostIf_IPClient_ReqHandler.cpp

503c67b

Copilot AI review requested due to automatic review settings

January 22, 2026 07:51

Copilot started reviewing on behalf of Vismalskumar0

January 22, 2026 07:51

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

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

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp Outdated Show resolved Hide resolved

src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp Outdated Show resolved Hide resolved

src/hostif/parodusClient/pal/webpa_attribute.cpp

-                      strncpy(attr[0][i]->value,Param.paramValue, strlen(Param.paramValue));
-                      attr[0][i]->value[strlen(Param.paramValue)] = '\0';
+              	int ret = get_AttribValues_tr69hostIf(&Param);
+                      if(ret == 0 && Param.paramValue != NULL)

Copilot AI Jan 22, 2026

The null check Param.paramValue != NULL is incorrect. The paramValue field is a fixed-size char array in the HOSTIF_MsgData_t structure (line 171 of hostIf_tr69ReqHandler.h), not a pointer, so it can never be NULL. This check will always evaluate to true. Consider checking if the string is empty using Param.paramValue[0] != '\0' instead, or relying solely on the return value check.

Suggested change

      
                    if(ret == 0 && Param.paramValue != NULL)
          
                    if(ret == 0 && Param.paramValue[0] != '\0')

Copilot uses AI. Check for mistakes.


          Apply suggestion from @Copilot

8b76f9f

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings

January 22, 2026 08:45

Copilot started reviewing on behalf of Vismalskumar0

January 22, 2026 08:45

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

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

src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp

    
              XBSStore* XBSStore::xbsInstance = NULL;

              std::atomic<XBSStore*> XBSStore::xbsInstance(nullptr);

Copilot AI Jan 22, 2026

The conversion to std::atomic<XBSStore*> is incomplete and introduces potential race conditions. The getInstance() method on lines 826-836 (not shown in diff but still present) uses double-checked locking with xbsInstance, but now that xbsInstance is atomic, the checks on lines 826 and 829 need to use .load() method. Additionally, the assignment on line 831 should use .store(). Without these changes, the code may not work correctly due to implicit conversions that may not be thread-safe in all scenarios.

Copilot uses AI. Check for mistakes.

src/unittest/stubs/ds/host.hpp

-                  AudioOutputPort &getAudioOutputPort(int id){};
+                  AudioOutputPort &getAudioOutputPort(const std::string &name)
+                  {
+                      static AudioOutputPort aPort(name);

Copilot AI Jan 22, 2026

The static AudioOutputPort instance is constructed with only a name parameter, but AudioOutputPort may require additional initialization parameters. If AudioOutputPort's constructor needs more than just a name, this will cause compilation errors or undefined behavior.

Suggested change

      
                    static AudioOutputPort aPort(name);
          
                    static AudioOutputPort aPort;

Copilot uses AI. Check for mistakes.

Vismalskumar0 and others added 2 commits

January 22, 2026 17:23


          Update Device_DeviceInfo.cpp

00502d4


          Apply suggestion from @Copilot

3ac73d2

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings

January 22, 2026 12:05

Copilot started reviewing on behalf of Vismalskumar0

January 22, 2026 12:05


          Update Device_DeviceInfo.cpp

68ce168

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.

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

src/hostif/handlers/src/hostIf_IPClient_ReqHandler.cpp

Comment on lines +493 to +500

+                          hostIf_IPInterface* ipIfInst = hostIf_IPInterface::getInstance(ifindexes[i]);
+                          if (ipIfInst) {
+                              int ipv4AddressNumberOfEntries = ipIfInst->getIPv4AddressNumberOfEntries();
+                              RDK_LOG (RDK_LOG_DEBUG, LOG_TR69HOSTIF, "[%s:%s:%d] ipv4AddressNumberOfEntries = %d, curNumOfInterfaceIPv4Addresses[%d] = %d\n",
+                                  __FILE__, __FUNCTION__, __LINE__, ipv4AddressNumberOfEntries, ifindexes[i], curNumOfInterfaceIPv4Addresses[ifindexes[i]]);
+                              sprintf (objectPath, "Device.IP.Interface.%d.IPv4Address.", ifindexes[i]);
+                              sendAddRemoveEvents (mUpdateCallback, ipv4AddressNumberOfEntries, curNumOfInterfaceIPv4Addresses[ifindexes[i]], objectPath);
+                          }

Copilot AI Jan 22, 2026

The null check for ipIfInst is added here, but the same pattern is used in the IPv6 code path (lines 504, 510) without null checks. Consider adding the same null check for consistency and safety in the IPv6 sections.

Copilot uses AI. Check for mistakes.

src/hostif/handlers/src/hostIf_DeviceClient_ReqHandler.cpp Outdated Show resolved Hide resolved

src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp Outdated Show resolved Hide resolved


          Update tr69hostif_webpa.py

05db83b

Copilot AI mentioned this pull request

[WIP] Update to address feedback on RDK Coverity defect resolution #340

Draft

Contributor

Copilot AI commented Jan 22, 2026

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


          Update src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp

64cf58e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings

January 22, 2026 14:34


          Update src/hostif/handlers/src/hostIf_DeviceClient_ReqHandler.cpp

1ac5be4

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot started reviewing on behalf of Vismalskumar0

January 22, 2026 14:35

Copilot AI reviewed

View reviewed changes

Contributor

Copilot AI left a comment

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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

Labels

None yet