Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes a series of small robustness and cleanup changes across host interface profiles: centralizing SafeC error handling via ERR_CHK, adding null checks and resource handling, tightening some concurrency patterns, and refining a few behaviors (e.g., event publishing and port selection).
Changes:
- Simplified SafeC
strcpy_serror handling by replacing manualif (rc != EOK)checks with theERR_CHK(rc)macro in many profile handlers (Time, STBService, IP, Ethernet, DeviceInfo, ProcessStatus, IP client handler). - Added defensive checks and cleanup: null-parameter guards (e.g., in
getVirtualInterfaceName,getEthernetInterfaceName), safer inotify setup inXBSStore::getAuthServicePartnerID, more robust process/thread logic inXBSStore::getInstance,executeRfcMgr, WebPA readiness monitoring, and Parodus condition waits. - Tweaked specific behaviors: bounded local port search, corrected RBUS
preValuehandling for download status events, and minor JSON-RPC helper usage updates viastd::move.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/hostif/profiles/Time/Device_Time.cpp | Replaced manual strcpy_s error checks with ERR_CHK, keeping time parameter backup/return logic unchanged but cleaner. |
| src/hostif/profiles/STBService/Components_VideoOutput.cpp | Simplified SafeC error handling for video output backup strings using ERR_CHK. |
| src/hostif/profiles/STBService/Components_SPDIF.cpp | Simplified SafeC error handling for SPDIF backup status string. |
| src/hostif/profiles/STBService/Components_HDMI.cpp | Simplified SafeC error handling for HDMI backup strings. |
| src/hostif/profiles/STBService/Components_DisplayDevice.cpp | Simplified SafeC error handling for display device backup strings. |
| src/hostif/profiles/STBService/Components_AudioOutput.cpp | Simplified SafeC error handling for multiple audio backup strings. |
| src/hostif/profiles/IP/Device_IP_Interface_IPv4Address.cpp | Centralized strcpy_s error handling via ERR_CHK in IPv4 address constructor, status, and addressing type getters. |
| src/hostif/profiles/IP/Device_IP_Interface.cpp | Simplified SafeC error handling in interface name/type getters using ERR_CHK. |
| src/hostif/profiles/IP/Device_IP.cpp | Added null check for virtual_if_name in getVirtualInterfaceName, switched to ERR_CHK for IPv4 status strings, and slightly refactored JSON-RPC usage for IP fields. |
| src/hostif/profiles/Ethernet/Device_Ethernet_Interface.cpp | Added null check in getEthernetInterfaceName and simplified SafeC error handling for interface status/name/MAC/duplex backups. |
| src/hostif/profiles/DeviceInfo/XrdkCentralComBSStore.cpp | Adjusted path construction for partner ID monitoring, and simplified XBSStore::getInstance to use a single lock guard without double-checked locking. |
| src/hostif/profiles/DeviceInfo/Device_DeviceInfo_ProcessStatus_Process.cpp | Centralized SafeC error checks via ERR_CHK for process command/state handling and associated getters, preserving existing logic. |
| src/hostif/profiles/DeviceInfo/Device_DeviceInfo.cpp | Various robustness fixes: centralized SafeC error handling, bounded local port search, std::move for JSON-RPC post data, safer RFC manager process/file cleanup, corrected RBUS event preValue, and WebPA readiness polling cleanup. |
| src/hostif/parodusClient/pal/libpd.cpp | Refined parodus_receive_wait to distinguish timeout vs. other errors from pthread_cond_timedwait and log appropriately. |
| src/hostif/handlers/src/hostIf_IPClient_ReqHandler.cpp | Simplified SafeC error handling with ERR_CHK when copying notification keys, but retains existing allocation and hash table logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rc=strcpy_s(notifyKey,strlen(stMsgData->paramName)+1,stMsgData->paramName); | ||
| if(rc!=EOK) | ||
| { | ||
| ERR_CHK(rc); | ||
| } | ||
| g_hash_table_insert(notifyhash,notifyKey,notifyValuePtr); | ||
| ret = OK; | ||
| } |
There was a problem hiding this comment.
notifyKey and notifyValuePtr are allocated, inserted into notifyhash, and then freed at the end of this block (free(notifyKey); free(notifyValuePtr);), even though the GHashTable holds pointers to them for later use. This creates a use-after-free (and likely double-free when the hash table is cleaned up) whenever the notification hash is read or destroyed. These allocations should not be freed immediately after g_hash_table_insert; instead, allow the hash table to own them and free them when notifications are disabled or when the table is destroyed, in line with the comment above.
Coverity Issue - Overflowed integer argumentThe cast of "ifindexes[i]" to a signed type could result in a negative number. High Impact, CWE-190 Issue locationThis issue was discovered outside the diff for this Pull Request. You can find it at: |
2 participants
No description provided.