RDKB-65853: Fixing coverity issue - #1283
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses several Coverity-reported issues across webconfig decoding/translation, PSM MAC filter handling, and WiFi event cloning/copying—primarily by adding null checks, fixing a use-after-free log path, and tightening monitor event payload sizing/copying.
Changes:
- Prevent null dereference when translating associated-device stats to EasyMesh when stats are absent.
- Fix a use-after-free in assoc-device stats decode logging by logging
sizeinstead of dereferencing a freed pointer. - Improve monitor event memory handling by tracking/copying the actual monitor payload size (
mon_data_len) and allocating/copying only what’s needed for specific monitor subtypes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| source/webconfig/wifi_easymesh_translator.c | Adds guard for missing stats input to avoid null dereference during translation. |
| source/webconfig/wifi_decoder.c | Fixes use-after-free in logging when assoc-device stats array size is 0. |
| source/dml/wifi_ssp/ssp_loop.c | Avoids freeing a hash-map key after insertion by nulling the pointer before cleanup. |
| source/core/wifi_events.c | Adds monitor payload sizing, stores mon_data_len, and uses it for cloning/copying monitor events. |
| source/apps/wifi_apps_mgr.c | Adjusts link-quality event creation to avoid unnecessary allocation when handing off owned buffers. |
| include/wifi_events.h | Extends wifi_event_t with mon_data_len to support variable-sized monitor payloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
source/dml/wifi_ssp/ssp_loop.c:1224
mcfg_macis allocated withstrdup()and then the function canreturnbefore the finalfree(mcfg_mac);(e.g., when the MAC entry already exists ormalloc()fortemp_mac_entryfails). This leaks memory. Sincehash_map_put()takes ownership of the key on success (collection.c:172-184), freeingmcfg_macis only needed on these early-return paths before insertion.
if (hash_map_put(psm_mac_map, mcfg_mac, temp_mac_entry) != 0)
{
free(mcfg_mac);
free(temp_mac_entry);
return;
}
mcfg_mac = NULL;
source/apps/wifi_apps_mgr.c:242
get_app_by_inst()can return NULL (other call sites check for this), but this function unconditionally dereferencesapp->desc.event_fn, which can crash if the link-quality app wasn’t registered/initialized. Add a NULL check and clean up the allocated event on error.
event = (wifi_event_t *)create_wifi_event(0, type, sub_type);
if (event == NULL) {
wifi_util_error_print(WIFI_APPS, "%s %d failed to allocate memory to event\n",__FUNCTION__, __LINE__);
return RETURN_ERR;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
source/apps/wifi_apps_mgr.c:253
- In
apps_mgr_link_quality_event, the event is created withmsg_len= 0 (socore_data.msgis not allocated), but whenarg != NULLthe code assignsevent->u.core_data.msg = arg.destroy_wifi_event()will laterfree(event->u.core_data.msg), which will attempt to free memory owned by the caller (or even stack memory), causing crashes/double-frees. The event should allocate its own buffer and copyarg, consistent with other event helpers in this file.
event = (wifi_event_t *)create_wifi_event(0, type, sub_type);
if (event == NULL) {
wifi_util_error_print(WIFI_APPS, "%s %d failed to allocate memory to event\n",__FUNCTION__, __LINE__);
return RETURN_ERR;
}
if (arg == NULL) {
event->u.core_data.msg = NULL;
event->u.core_data.len = 0;
} else {
/* copy msg to data */
event->u.core_data.msg = arg;
event->u.core_data.len = len;
event->event_type = type;
event->sub_type = sub_type;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
source/dml/wifi_ssp/ssp_loop.c:1263
hash_map_put(psm_mac_map, strdup(mcfg->mac), temp_mac_entry)failure path returns without freeingtemp_mac_entry, and also leaks the duplicated key since it is created inline. Allocate the key into a variable so you can free both the key and value on failure.
if (hash_map_put(psm_mac_map, strdup(mcfg->mac), temp_mac_entry) != 0) {
wifi_util_error_print(WIFI_PSM, "%s:%d hash_map_put failed for mac filter entry\r\n", __func__, __LINE__);
return;
}
source/apps/wifi_apps_mgr.c:261
- In this function
create_wifi_event(len, ...)allocatescore_data.msg, but that pointer is immediately overwritten witharg(ownership transfer), leaking the allocation. This becomes more visible on the newapp == NULLerror path, wheredestroy_wifi_event(event)will also freearg(intended) but still cannot free the lost allocation. Create the event withmsg_len=0since you are not copying into the internal buffer.
if (app == NULL) {
wifi_util_error_print(WIFI_APPS, "%s %d assert - NULL Pointer\n", __FUNCTION__, __LINE__);
destroy_wifi_event(event);
return RETURN_ERR;
}
source/dml/wifi_ssp/ssp_loop.c:1224
hash_map_put(psm_mac_map, mcfg_mac, temp_mac_entry)failure path returns without freeingtemp_mac_entryandmcfg_mac, leaking memory and leaving a partially-applied update (PSM record may have been written but map not updated). Free the allocated objects before returning on failure.
This issue also appears on line 1260 of the same file.
if (hash_map_put(psm_mac_map, mcfg_mac, temp_mac_entry) != 0)
{
wifi_util_error_print(WIFI_PSM, "%s:%d hash_map_put failed for mac filter entry\r\n", __func__, __LINE__);
return;
}
Reason for change: Fixing 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
source/dml/wifi_ssp/ssp_loop.c:1262
- This passes strdup(mcfg->mac) directly into hash_map_put() and on failure returns immediately. If hash_map_put() fails before queue_push() (e.g. internal malloc failure), the strdup key and temp_mac_entry are leaked and the key pointer is lost, making cleanup impossible here without changing the ownership contract (see source/utils/collection.c:158-186).
if (hash_map_put(psm_mac_map, strdup(mcfg->mac), temp_mac_entry) != 0) {
wifi_util_error_print(WIFI_PSM, "%s:%d hash_map_put failed for mac filter entry\r\n", __func__, __LINE__);
return;
source/webconfig/wifi_easymesh_translator.c:1549
- When the AssociatedDeviceStats array is empty, decode_assocdev_stats_object() returns webconfig_error_none and leaves collect_stats.stats as NULL. Returning webconfig_error_translate_to_easymesh here will turn the valid "no associated clients" case into a translation failure for webconfig_subdoc_type_assocdev_stats.
if (params->collect_stats.stats == NULL) {
wifi_util_info_print(WIFI_WEBCONFIG,
"%s:%d: no associated device stats to translate\n", __func__, __LINE__);
return webconfig_error_translate_to_easymesh;
}
source/dml/wifi_ssp/ssp_loop.c:1224
- On hash_map_put() failure this returns without freeing mcfg_mac/temp_mac_entry. hash_map_put() only frees key/data when queue_push() fails; it does not free them on earlier failures (e.g. malloc(sizeof(hash_element_t)) failure), so this path can leak allocations under OOM conditions (see source/utils/collection.c:158-186).
This issue also appears on line 1260 of the same file.
if (hash_map_put(psm_mac_map, mcfg_mac, temp_mac_entry) != 0)
{
wifi_util_error_print(WIFI_PSM, "%s:%d hash_map_put failed for mac filter entry\r\n", __func__, __LINE__);
return;
}
Reason for change: Fixing 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