RDKB-64347: Fixing coverity issues#1096
RDKB-64347: Fixing coverity issues#1096bharathivelp wants to merge 1 commit intordkcentral:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses Coverity-reported issues across WiFi monitoring, webconfig consumer logging, and the WiFi blaster active-measurement flow.
Changes:
- Removes an ineffective/null check and simplifies disconnect cleanup logic in associated-client stats collection.
- Adjusts STA connection status log formatting in the webconfig consumer.
- Avoids holding the blaster mutex while collecting (potentially slow) system health stats by restructuring the resource-gathering condition.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
source/stats/wifi_stats_assoc_client.c |
Removes dead/null logic, tweaks disconnect removal condition, and adds a sta_map NULL guard before event building. |
source/sampleapps/webconfig_consumer_apis.c |
Updates STA connect/disconnect printf formats. |
source/apps/blaster/wifi_blaster.c |
Refactors resource collection into a resource_ok boolean and releases the mutex during resource collection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
157bd13 to
13211a6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
a04e6b5 to
6bcb5ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define CSI_TMP_TEMPLATE "/tmp/CSI_tmp.bin" | ||
| WIFI_EVENT_CONSUMER_DGB("Enter %s: %d\n", __FUNCTION__, __LINE__); | ||
| char filename[] = CSI_FILE; | ||
| char filename_tmp[] = CSI_TMP_FILE; | ||
| FILE *csifptr; | ||
| FILE *csifptr_tmp; | ||
| char filename_tmp[] = CSI_TMP_TEMPLATE; | ||
| FILE *csifptr = NULL; | ||
| FILE *csifptr_tmp = NULL; |
There was a problem hiding this comment.
CSI_TMP_TEMPLATE is not actually a template (it’s a fixed filename in /tmp), and creating a predictable temp file in /tmp via fopen(..., "w") is susceptible to symlink/clobber attacks. Please switch to a securely-created temporary file (e.g., mkstemp/open with O_EXCL) and then rename it into place, or otherwise ensure the temp path cannot be pre-created/redirected by another process.
6bcb5ab to
100fb87
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
100fb87 to
78e2ae2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d9fe651 to
f54cf35
Compare
f54cf35 to
0b4e4a6
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 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (fread(&tmp_mac, sizeof(tmp_mac), 1, csifptr) != 1 || | ||
| fread(&tmp_frame_info, sizeof(tmp_frame_info), 1, csifptr) != 1 || | ||
| fread(&tmp_csi_matrix, sizeof(tmp_csi_matrix), 1, csifptr) != 1) { | ||
| WIFI_EVENT_CONSUMER_DGB("Failed to read oldest CSI record\n"); |
There was a problem hiding this comment.
When dropping the oldest record, a partial read (e.g., first fread succeeds but the second/third fails) leaves the file position mid-record, and the subsequent copy loop will read/write misaligned data into the new file. Consider treating this as a hard failure for rotation (e.g., abort copying and start a fresh file, or rewind and copy from the beginning without dropping) instead of continuing after a failed drop.
| WIFI_EVENT_CONSUMER_DGB("Failed to read oldest CSI record\n"); | |
| WIFI_EVENT_CONSUMER_DGB("Failed to read oldest CSI record, copying from the beginning instead\n"); | |
| if (fseek(csifptr, 0, SEEK_SET) != 0) { | |
| WIFI_EVENT_CONSUMER_DGB("Failed to rewind CSI file after partial oldest-record read: %s\n", strerror(errno)); | |
| csifptr = NULL; | |
| } |
0b4e4a6 to
5d5a08c
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 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (sscanf(event->name, "Device.WiFi.STA.%d.InterfaceName", &index) != 1) { | ||
| printf("%s:%d sscanf failed to parse index from event name\n", __FUNCTION__, __LINE__); | ||
| return; | ||
| } |
There was a problem hiding this comment.
index is declared as unsigned int, but the sscanf format string uses %d (expects int*). This is undefined behavior. Use %u (or change index to int and validate it) to make the types consistent.
Reason for change: Fixing Low 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
5d5a08c to
726b90f
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 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| snprintf(str, sizeof(str), "%s", name); | ||
| if (sscanf(str, "Device.WiFi.STA.%u.%s", &index, extension) != 2) { | ||
| wifi_util_error_print(WIFI_CTRL, "%s Invalid property name format %s\n", __FUNCTION__, str); | ||
| return bus_error_invalid_input; | ||
| } |
There was a problem hiding this comment.
snprintf(str, sizeof(str), "%s", name) can truncate long property names without detection; parsing a truncated string can make an invalid/partial property look valid. Also, sscanf(..., "%u.%s", ...) uses an unbounded %s, which can overflow extension[64] if the suffix is longer than 63 bytes. Consider rejecting truncation (check snprintf return) and bounding the scan with a field width (or parse directly from name using bounded specifiers).
Reason for change: Fixing Low 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