RDKB-64347: Fixing coverity issues#1061
RDKB-64347: Fixing coverity issues#1061bharathivelp wants to merge 1 commit intordkcentral:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Coverity findings by replacing unsafe string operations (e.g., sprintf, strcpy, strcat) with bounded alternatives (snprintf, strncat, _ansc_snprintf) across several WiFi / TR-181 components.
Changes:
- Hardened string formatting/copying in WebPA/TR-181 utilities and WiFi control/analytics paths.
- Updated RBUS handler and DB callback string copies to use bounded writes.
- Minor variable rename in captive portal PSM update flow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| source/dml/tr_181/sbapi/webpa_interface.c | Adds buffer-length plumbing and replaces unbounded string ops when querying component health / ethernet WAN support. |
| source/dml/tr_181/sbapi/cosa_wifi_apis.c | Replaces strcat with bounded strncat in debug logging helper. |
| source/dml/tr_181/ml/cosa_wifi_internal.c | Reworks MAC list string construction to use snprintf with an offset. |
| source/dml/tr_181/ml/cosa_apis_util.c | Replaces _ansc_sprintf with _ansc_snprintf for bounded path/param construction. |
| source/db/wifi_db_apis.c | Uses snprintf instead of strcpy for RFC ID copy from OVSDB record. |
| source/core/wifi_ctrl_rbus_handlers.c | Uses snprintf for RBUS string responses (connected radius endpoint). |
| source/core/wifi_ctrl.c | Uses snprintf for "true"/"false" PSM value and renames local buffer variable. |
| source/apps/analytics/wifi_analytics.c | Uses snprintf instead of sprintf for WPA3 RFC analytics string. |
Comments suppressed due to low confidence (1)
source/dml/tr_181/sbapi/cosa_wifi_apis.c:1408
ccspWifiDbgPrint()still usesvsprintf(&buff[strlen(buff)], ...), which can overflowbuff(4096 bytes) depending on the formatted message length. Since this function is used widely for debug output, this is a real memory-safety issue; please switch tovsnprintfand compute the remaining capacity (taking into account the existing timestamp prefix) so the write is bounded, and handle truncation if needed.
strncat(buff, " ", sizeof(buff)-strlen(buff)-1);
va_start(list, format);
vsprintf(&buff[strlen(buff)], format, list);
va_end(list);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int offset = strlen(maclist); | ||
| for(i = 0; i<numList; i++) { | ||
| if(i > 0) | ||
| strcat(maclist, ","); | ||
| sprintf(macAddr[i], "%02x:%02x:%02x:%02x:%02x:%02x", mac[j], mac[j+1], mac[j+2], mac[j+3], mac[j+4], mac[j+5]); | ||
| strcat(maclist, macAddr[i]); | ||
| if(i > 0) { | ||
| offset += snprintf(maclist + offset, 1024 - offset, ","); | ||
| } | ||
| snprintf(macAddr[i], sizeof(macAddr[i]), "%02x:%02x:%02x:%02x:%02x:%02x", mac[j], mac[j+1], mac[j+2], mac[j+3], mac[j+4], mac[j+5]); | ||
| offset += snprintf(maclist + offset, 1024 - offset, "%s", macAddr[i]); |
Reason for change: Fixing high medium priority coverity issues. Test Procedure: Build should be successful and the regression test should also succeed. Risks: Low Priority: P1 Signed-off-by: Velpula_Bharathi@comcast.com
1d22090 to
aee6ae2
Compare
There was a problem hiding this comment.
Pull request overview
Addresses Coverity-reported string/buffer safety issues across the WiFi/CCSP codepaths by replacing unsafe string APIs with bounded variants.
Changes:
- Update multiple
sprintf/strcpy/strcat/strncpyusages tosnprintf/strncat/_ansc_snprintf. - Extend
checkComponentHealthStatus()to accept an output buffer length and use bounded writes. - Minor variable rename in captive portal PSM update flow.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| source/dml/tr_181/sbapi/webpa_interface.c | Adds status_len to health status helper and switches to bounded string copies. |
| source/dml/tr_181/sbapi/cosa_wifi_apis.c | Replaces strcat with strncat in debug logging path. |
| source/dml/tr_181/ml/cosa_apis_util.c | Replaces _ansc_sprintf with _ansc_snprintf for TR-181 path construction. |
| source/db/wifi_db_apis.c | Uses snprintf to copy RFC id from OVSDB schema record into cached struct. |
| source/core/wifi_ctrl_rbus_handlers.c | Uses snprintf when writing allocated RBUS string return buffers. |
| source/core/wifi_ctrl.c | Uses bounded copies for PSM boolean string and renames local buffer. |
| source/apps/analytics/wifi_analytics.c | Replaces sprintf with snprintf for analytics event formatting. |
Comments suppressed due to low confidence (1)
source/dml/tr_181/sbapi/cosa_wifi_apis.c:1408
vsprintf(&buff[strlen(buff)], ...)can overflowbuffdespite the saferstrncatabove, since it performs an unbounded write starting at the current end of the buffer. Replace withvsnprintfusing the remaining capacity (and handle the return value) to avoid a potential stack buffer overflow.
strncat(buff, " ", sizeof(buff)-strlen(buff)-1);
va_start(list, format);
vsprintf(&buff[strlen(buff)], format, list);
va_end(list);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ret = CcspBaseIf_getParameterValues(bus_handle, str, dbusPath, parameterNames, 1, &val_size, | ||
| ¶meterval); | ||
| if (ret == CCSP_SUCCESS) { | ||
| strcpy(status, parameterval[0]->parameterValue); | ||
| snprintf(status, status_len, "%s", parameterval[0]->parameterValue); | ||
| } |
Reason for change: Fixing high medium priority coverity issues.
Test Procedure: Build should be successful and the regression test should also succeed.
Risks: Low
Priority: P1
Signed-off-by: Velpula_Bharathi@comcast.com