RDKB-64347: Fixing coverity issues#1056
RDKB-64347: Fixing coverity issues#1056bharathivelp wants to merge 2 commits intordkcentral:developfrom
Conversation
a0a1422 to
5ca3cc4
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses high/medium Coverity findings primarily by replacing unbounded string operations (strcpy, sprintf, %s in sscanf) with bounded alternatives and adding a small guard against unsigned underflow in a log message.
Changes:
- Replace multiple
strcpy/sprintfuses withsnprintf(and onestrcpywithstrncpy+ explicit NUL termination). - Add field-width limits to
sscanfto prevent buffer overreads/writes. - Prevent
index - 1underflow in station connection-status logging.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/stats/wifi_monitor.c | Use snprintf instead of strcpy when updating RADIUS connected endpoint (non-PHASE2). |
| source/sampleapps/webconfig_consumer_apis.c | Clamp (index - 1) in log output to avoid underflow when index == 0. |
| source/dml/tr_181/sbapi/cosa_wifi_apis.c | Replace strcat/strcpy with bounded concatenation/copy for security-mode string building. |
| source/db/wifi_db_apis.c | Bound copy of last reboot reason from PSM to a fixed maximum length with explicit terminator. |
| source/core/wifi_mgr.c | Replace strcpy/sprintf with snprintf when populating VAP name and DB path. |
| source/core/wifi_ctrl_rbus_handlers.c | Add %63s limit to sscanf for extension parsing. |
| source/core/wifi_ctrl.c | Replace strcpy with snprintf when copying STA SSID out of the config map. |
| lib/pktgen/pktgen.c | Add %31s limit to sscanf for run_if parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5ca3cc4 to
d0dd67a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| len = strlen(securityName); | ||
| if (len > 0) | ||
| { | ||
| strcat(securityName, ","); | ||
| strcat(securityName, wifiSecMap[i].wifiSecType); | ||
| snprintf(securityName + len, bufSize - len, ",%s", wifiSecMap[i].wifiSecType); | ||
| } |
There was a problem hiding this comment.
getSecurityStringFromInt() now takes bufSize, but it still uses unbounded strlen(securityName). If securityName is not guaranteed to be NUL-terminated within bufSize, this can read past the provided buffer, and can also make bufSize - len incorrect. Use a bounded length calculation (e.g., strnlen(securityName, bufSize)) and guard len < bufSize before appending so the function is safe for all callers.
| strncpy(last_reboot_reason, str, 31); | ||
| last_reboot_reason[31] = '\0'; |
There was a problem hiding this comment.
This function hard-codes a 32-byte destination (strncpy(..., 31) + last_reboot_reason[31] = '\0'), but the API only accepts char *last_reboot_reason with no size. That creates an implicit contract that callers must always provide at least 32 bytes. Consider changing the signature to accept a buffer length (and using it), or at least using a named constant for the required size and documenting/enforcing it.
d0dd67a to
3fc2caa
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| conn_status = (sta_conn_info.connect_status == wifi_connection_status_connected) ? true:false; | ||
| if (conn_status == true) { | ||
| printf("%s:%d: Station successfully connected with external AP radio:%d\r\n", | ||
| __func__, __LINE__, index - 1); | ||
| __func__, __LINE__, (index > 0) ? (index - 1) : 0); | ||
| if (index == 1) { |
There was a problem hiding this comment.
index is declared as unsigned int, but it is populated via sscanf(event->name, "...%d...", &index) earlier in this function. Passing an unsigned int* for a %d conversion is undefined behavior and can also leave index at 0 if parsing fails. Consider switching the parse to an unsigned/signed conversion that matches the variable type, checking sscanf’s return value, and treating index == 0/parse failure as an invalid event name rather than silently logging radio 0 via the new ternary.
| if (sscanf(name, "Device.WiFi.STA.%u.%63s", &index, extension) != 2 || index < 1) { | ||
| wifi_util_error_print(WIFI_CTRL, "%s Invalid property name format: %s\n", __FUNCTION__, name); | ||
| return bus_error_invalid_input; | ||
| } | ||
| if (index > getNumberRadios()) { | ||
| wifi_util_error_print(WIFI_CTRL, "%s Invalid index %d\n", __FUNCTION__, index); |
There was a problem hiding this comment.
Parsing STA properties with %u into an unsigned int is fine, but note that %u will accept a leading '-' and convert it to a very large unsigned value (e.g., "-1" -> UINT_MAX). If you want negative indexes to be treated as invalid input (not just "index > getNumberRadios"), parse into a signed temporary and validate > 0 before casting. Also, the later error log uses %d for index even though it’s unsigned; the format specifier should match the variable type to avoid undefined behavior / misleading logs.
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
3fc2caa to
a310821
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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