Release 2.3.0#49
Conversation
Release 2.2.0 2.2.0
… Failure (#32) 1. dhcpsnooper in hotspot component monitors the DHCP exchange process for newly associated hotspot clients. If a client successfully associates with the Access Point (AP) but does not receive a valid IP address assignment from the DHCP server within a predefined period and automatically trigger a de-authentication for that client. 2. Implement a data model( Device.X_COMCAST-COM_GRE.Hotspot.RejectAssociatedClient)for the Rbus event, which will be triggered whenever a client's DHCP transaction fails to assign an IP address within a specified period (i.e., a timeout). --------- Signed-off-by: raghavendra.hegde2@comcast.com
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Release 2.3.0 adds Hotspot client deauthentication support on DHCP failure by tracking per-client DHCP state and publishing an RBUS event to OneWifi, along with accompanying unit tests and changelog updates.
Changes:
- Added DHCP-state tracking per client (linked list) and timeout-based deauth trigger that publishes an RBUS event.
- Extended client bookkeeping to carry
vapIndexand updated RSSI update API accordingly. - Added unit tests for client-state helpers, RBUS publish helper, and DHCP timer state logic; updated
CHANGELOG.mdfor 2.3.0.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| source/test/mocks/hotspotfd_internal_mock.h | Exposes new global client_list for unit tests. |
| source/test/HotspotFdTest/HotspotFdTest.cpp | Adds unit tests and shared cleanup helper for DHCP client-state logic and RBUS publishing. |
| source/hotspotfd/include/dhcpsnooper.h | Introduces DHCP client state structs + APIs, adds vapIndex, updates function signatures. |
| source/hotspotfd/hotspotfd.c | Adds RBUS data element registration + init for the RejectAssociatedClient event. |
| source/hotspotfd/dhcpsnooper.c | Implements RBUS publish helper, client-state list management, and DHCP timer/deauth logic in packet handler. |
| source/hotspotfd/cosa_hotspot_dml.c | Updates caller to pass vapIndex into RSSI update API. |
| CHANGELOG.md | Adds 2.3.0 release notes entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| msg_debug("Command string: %s - %s\n", cmdStr, __FUNCTION__); | ||
|
|
||
| if (cmdStr == NULL || strlen(cmdStr) == 0) { | ||
| msg_debug("mac address empty and not sending rbus event to onewifi: %d\n", __LINE__); | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
cmdStr is logged with %s before it is null-checked; calling publish_to_onewifi(NULL) will dereference a NULL pointer in the log call. Move the log after the NULL/empty check or log safely (e.g., conditional string or %p).
| msg_debug("Command string: %s - %s\n", cmdStr, __FUNCTION__); | |
| if (cmdStr == NULL || strlen(cmdStr) == 0) { | |
| msg_debug("mac address empty and not sending rbus event to onewifi: %d\n", __LINE__); | |
| return 0; | |
| } | |
| if (cmdStr == NULL || strlen(cmdStr) == 0) { | |
| msg_debug("mac address empty and not sending rbus event to onewifi: %d\n", __LINE__); | |
| return 0; | |
| } | |
| msg_debug("Command string: %s - %s\n", cmdStr, __FUNCTION__); |
| } | ||
| } | ||
|
|
||
| static void constructCommand(char *macaddr, char *cmd) |
There was a problem hiding this comment.
constructCommand copies into cmd using a fixed length derived from macaddr_with_index (63 bytes) without knowing the capacity of cmd, and it doesn’t guarantee cmd is NUL-terminated. This can overflow the caller’s buffer and/or produce unterminated strings. Update the API to accept cmd length (e.g., constructCommand(..., char* cmd, size_t cmd_len)) and use bounded formatting/copying into cmd with explicit termination.
| snprintf(macaddr_with_index, sizeof(macaddr_with_index), "%s_%d", macaddr, pNewClient->client.vapIndex); | ||
| macaddr_with_index[sizeof(macaddr_with_index) - 1] = '\0'; | ||
| } | ||
| strncpy(cmd, macaddr_with_index, sizeof(macaddr_with_index) - 1); | ||
| msg_debug("constructCommand :%s\n", cmd); | ||
| pthread_mutex_unlock(&global_stats_mutex); |
There was a problem hiding this comment.
constructCommand copies into cmd using a fixed length derived from macaddr_with_index (63 bytes) without knowing the capacity of cmd, and it doesn’t guarantee cmd is NUL-terminated. This can overflow the caller’s buffer and/or produce unterminated strings. Update the API to accept cmd length (e.g., constructCommand(..., char* cmd, size_t cmd_len)) and use bounded formatting/copying into cmd with explicit termination.
| clock_gettime(CLOCK_MONOTONIC, &now); | ||
| return (now.tv_sec - start.tv_sec) * 1000 + (now.tv_nsec - start.tv_nsec) / 1000000; |
There was a problem hiding this comment.
This elapsed-time calculation is incorrect when now.tv_nsec < start.tv_nsec (common across second boundaries), producing a negative millisecond component and inaccurate totals. Normalize the timespec delta (borrow 1s when nsec diff is negative) or compute using a single nanosecond total with proper signed arithmetic before converting to ms.
| clock_gettime(CLOCK_MONOTONIC, &now); | |
| return (now.tv_sec - start.tv_sec) * 1000 + (now.tv_nsec - start.tv_nsec) / 1000000; | |
| time_t sec_diff; | |
| long nsec_diff; | |
| clock_gettime(CLOCK_MONOTONIC, &now); | |
| sec_diff = now.tv_sec - start.tv_sec; | |
| nsec_diff = now.tv_nsec - start.tv_nsec; | |
| if (nsec_diff < 0) { | |
| sec_diff--; | |
| nsec_diff += 1000000000L; | |
| } | |
| return sec_diff * 1000 + nsec_diff / 1000000; |
| msg_debug("All DHCP states completed with ACK IP 172.20.20.20 in %ld ms. Timer stopped.\n", elapsed_time); | ||
| state->timer_running = false; | ||
| state->dhcp_discover = state->dhcp_request = state->dhcp_offer = state->dhcp_ack = false; | ||
| remove_client_state(mac_str); |
There was a problem hiding this comment.
remove_client_state(mac_str) frees the list node backing state, leaving state as a dangling pointer for the remainder of snoop_packetHandler. Even if it’s not currently used afterward, this is fragile and can easily become a use-after-free with future edits. Set state = NULL immediately after removal (and avoid any subsequent reads), or defer removal until the end of the handler.
| remove_client_state(mac_str); | |
| remove_client_state(mac_str); | |
| state = NULL; |
| event.data = data; | ||
| event.type = RBUS_EVENT_GENERAL; | ||
|
|
||
| rc = rbusEvent_Publish(disassoc_rbus_handle, &event); |
There was a problem hiding this comment.
disassoc_rbus_handle is used unconditionally here, but RBUS initialization in hotspotDisassocClientRbusInit() can fail (RBUS not available / open failure / reg failure). Publishing with an uninitialized/invalid handle can fail unpredictably (and may crash depending on RBUS implementation). Add a guard in publish_to_onewifi (e.g., check handle validity and/or a “rbus ready” flag set only on successful init) and return a failure code early if RBUS isn’t initialized.
| // Helper to clean up the client_list linked list between tests | ||
| static void cleanup_client_list() { | ||
| client_node_t *cur = client_list; | ||
| while (cur) { | ||
| client_node_t *next = cur->next; | ||
| free(cur); | ||
| cur = next; | ||
| } | ||
| client_list = NULL; | ||
| } |
There was a problem hiding this comment.
Many tests call cleanup_client_list() manually at the end. If any test hits an ASSERT_* and returns early, cleanup won’t run, leaking global state into subsequent tests and causing order-dependent failures. Prefer moving this cleanup into the fixture’s TearDown() (or using a scoped cleanup guard) so it always executes, and remove per-test cleanup calls.
Release 2.3.0