feat: audio sync over wifi #3847
Conversation
TuEmb
force-pushed
the
TuEmb/sync_over_wifi
branch
from
c35d2ca to
6587336
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces WiFi functionality for audio data synchronization, which is a significant feature. The implementation includes a new WiFi state machine, configuration via BLE, and data transfer over UDP. While the overall structure is good, I've found several critical and high-severity issues that must be addressed. These include a buffer overflow vulnerability, race conditions, incorrect state handling, and long blocking calls that can affect system stability and responsiveness. Please review the detailed comments for suggestions on how to resolve these issues.
I am having trouble creating individual review comments. Click here to see my feedback.
omi/firmware/omi/src/lib/core/storage.c (276-301)
There's a critical buffer overflow vulnerability here. The lengths ssid_len, pwd_len, and ip_len are read from the input buffer but are not validated against the sizes of the destination buffers (ssid, pwd, ip) before calling memcpy. A malicious client could provide a large length value, causing a buffer overflow and potentially leading to arbitrary code execution. You should add checks to ensure the lengths do not exceed the buffer sizes minus one (for the null terminator).
if (ssid_len == 0 || ssid_len >= sizeof(ssid) || idx + ssid_len > len) {
LOG_WRN("SSID length invalid: ssid_len=%d, idx=%d, len=%d", ssid_len, idx, len);
result_buffer[0] = 3; break;
}
char ssid[33] = {0};
memcpy(ssid, &((const uint8_t *)buf)[idx], ssid_len);
idx += ssid_len;
LOG_INF("WIFI_SETUP: ssid='%s', idx=%d", ssid, idx);
uint8_t pwd_len = ((const uint8_t *)buf)[idx++];
LOG_INF("WIFI_SETUP: pwd_len=%d, idx=%d, len=%d", pwd_len, idx, len);
if (pwd_len > 0 && (pwd_len >= sizeof(pwd) || idx + pwd_len > len)) {
LOG_WRN("PWD length invalid: pwd_len=%d, idx=%d, len=%d", pwd_len, idx, len);
result_buffer[0] = 4; break;
}
char pwd[65] = {0};
if (pwd_len > 0) memcpy(pwd, &((const uint8_t *)buf)[idx], pwd_len);
idx += pwd_len;
LOG_INF("WIFI_SETUP: pwd='%s', idx=%d", pwd, idx);
uint8_t ip_len = ((const uint8_t *)buf)[idx++];
LOG_INF("WIFI_SETUP: ip_len=%d, idx=%d, len=%d", ip_len, idx, len);
if (ip_len == 0 || ip_len >= sizeof(ip) || idx + ip_len > len) {
LOG_WRN("IP length invalid: ip_len=%d, idx=%d, len=%d", ip_len, idx, len);
result_buffer[0] = 5; break;
}omi/firmware/omi/src/wifi.c (373-389)
This function is critically flawed. It's called when a disconnect event occurs, but it fails to update the connection status by calling wifi_set_connected(false). This will cause the state machine to incorrectly believe it's still connected, leading to failures in reconnection logic. Additionally, it unconditionally sets WIFI_FLAG_DISCONNECT_REQUESTED, which is incorrect for unsolicited disconnections.
static void handle_wifi_disconnect_result(struct net_mgmt_event_callback *cb, struct net_if *iface)
{
if (!wifi_is_connected()) {
return;
}
const struct wifi_status *status = (const struct wifi_status *)cb->info;
if (status && status->status) {
LOG_WRN("Disconnect status: %d", status->status);
}
LOG_WRN("WiFi disconnected, close UDP socket if any");
wifi_set_connected(false);
udp_close_socket();
set_wifi_state(WIFI_STATE_ON);
}omi/firmware/omi/src/wifi.c (886-891)
This function blocks the calling thread by busy-waiting in a while loop for the WiFi state to become WIFI_STATE_OFF. The shutdown process itself includes a 5-second sleep (k_msleep(5000)) in handle_wifi_shutdown, meaning this function will block for at least 5 seconds. This is a critical issue that can lead to an unresponsive system or watchdog timer expirations. The wifi_turn_off function should be asynchronous, or the shutdown logic should be redesigned to avoid long blocking calls.
omi/firmware/omi/src/lib/core/storage.c (340)
The bt_gatt_notify call is using &storage_service.attrs[1], which corresponds to the storage_write_uuid characteristic. However, this handler is for the storage_wifi_uuid characteristic. You should send notifications on the correct characteristic, which is storage_wifi_uuid at index 5 of the storage_service_attr array. Using the wrong attribute will send notifications to clients subscribed to the wrong characteristic. This also applies to other bt_gatt_notify calls in this function.
bt_gatt_notify(conn, &storage_service.attrs[5], &result_buffer, 1);omi/firmware/omi/src/main.c (314)
There is a 10-second blocking sleep (k_sleep(K_SECONDS(10))) during startup before initializing WiFi. This significantly delays the device's readiness without any explanation. If this delay is to wait for hardware to stabilize, it should be replaced with a more robust, event-driven mechanism, such as waiting for a device-ready signal. Unexplained long sleeps in initialization are fragile and hurt user experience.
omi/firmware/omi/src/wifi.c (53)
The global variable current_wifi_state is accessed from multiple threads (the WiFi thread and any thread calling the public API) without any synchronization. This can lead to race conditions. For example, a check-then-act sequence in wifi_turn_on is not atomic. You should use an atomic_t variable and atomic operations (atomic_set, atomic_get) to ensure thread-safe access to the WiFi state.
static atomic_t current_wifi_state = ATOMIC_INIT(WIFI_STATE_OFF);omi/firmware/omi/src/wifi.c (86-89)
The global variables for WiFi credentials and server address (wifi_ssid, wifi_password, tcp_server_addr, tcp_server_port) are written in setup_wifi_credentials and setup_udp_server (called from a BLE callback thread) and read in wifi_connect (called from the WiFi thread) without any synchronization. This is a race condition that could lead to using partially updated or corrupted credentials. You should protect access to these variables with a mutex.
omi/firmware/omi/src/wifi.c (238-267)
This while loop is a busy-wait. It will spin, consuming CPU cycles, until DHCP is bound or the timeout is reached. This is inefficient. You should use k_sem_take with a timeout to wait for the dhcp_bound_sem semaphore, which is a much more efficient way to wait for an event.
static int wifi_wait_for_dhcp(int32_t timeout_ms)
{
struct net_if *iface = net_if_get_wifi_sta();
if (atomic_get(&dhcp_bound)) {
return 0;
}
/* Make sure we don't consume a stale give from previous connections */
k_sem_reset(&dhcp_bound_sem);
if (k_sem_take(&dhcp_bound_sem, K_MSEC(timeout_ms)) != 0) {
/* Timeout occurred, check final state */
if (!atomic_get(&dhcp_bound) && !ipv4_ready(iface)) {
return -ETIMEDOUT;
}
}
return 0;
}omi/firmware/omi/src/wifi.c (631)
Using a long, blocking k_msleep(5000) to wait for WiFi disconnection is unreliable and inefficient. It blocks the WiFi thread for 5 seconds, preventing it from doing other work. A better approach would be to request disconnection and then wait for the NET_EVENT_WIFI_DISCONNECT_RESULT event, for example by using a semaphore. This would make the shutdown process faster and more robust.
omi/firmware/omi/src/wifi.c (1142-1144)
On a UDP sendto error, this code immediately closes the UDP socket and requests a WiFi disconnect. This is too aggressive. Many sendto errors are transient (e.g., buffer full) or related to the remote peer, not the local WiFi link. Tearing down the entire WiFi connection will cause significant disruption. It would be more robust to handle errors like EHOSTUNREACH or ECONNREFUSED by simply logging them and maybe pausing UDP traffic for a short period, without disconnecting from the WiFi network.
omi/firmware/omi/src/wifi.h (31)
The function register_wifi_ready is declared as static, which limits its scope to the current translation unit. Declaring a static function in a public header file is incorrect and will lead to compiler warnings or errors if the header is included in multiple source files. This function should be removed from the header and kept private to wifi.c.
1. Configure WiFi & TCP Server (WIFI_SETUP)Command: 0x01 Payload format:
Example: Response: [0x00] on success, other error codes for invalid format. 2. Enable WiFi (WIFI_START)Command: 0x02 3. Disable WiFi (WIFI_SHUTDOWN)Command: 0x03 |
|
|
Now we can reach ~100KBps in TCP protocol 
|
|
Flow to make an WIFI connection for Omi device 
|
|
Tested with some scenarios:
|
TuEmb
force-pushed
the
TuEmb/sync_over_wifi
branch
from
30d76f6 to
cf5e767
Compare
TuEmb
force-pushed
the
TuEmb/sync_over_wifi
branch
2 times, most recently
from
ebf5cdd to
58f2468
Compare
|
Wifi sync speed: 
|
|
lifetime in offline mode - without wifi feature adding ~7 hours and 10 minutes lifetime in offline mode - with wifi feature adding ~7 hours and 8 minutes |
|
lgtm @TuEmb |
|
-- 2/ Can we automatically create the SSID and password without asking the user? 3/ I’m not sure if the current sync mode is Wi-Fi or BLE. It might be better to let users choose whether they want to use BLE or Wi-Fi for the next sync, or at least allow them to switch. 4/ Can we display the speed after 5 seconds of syncing and recalculate it every 5 seconds? I think it would improve the UX since waiting too long for the first bytes might lead users to think our sync is slow. 5/ Is BLE sync now 17KB/s? I remember it being 30KB/s last time. Just double-checking. 6/ After a while, the app shows "Device disconnected," and the device's LED is still blue, but I cannot find it using either Omi or NRF Connect. I turned the device off by pressing the button, but now I cannot turn it on again. So, red flag. 7/ I also cannot find a way to update my SSID/password for my hotspot. If allowing users to enter it manually is the only way, we need to let users edit it. It would be great if we could run some tests to let users know that their hotspot is good to go. 8/ On the firmware side, please also handle cases where the hotspot credentials are incorrect to protect the device. (updates: add 6, 7, 8) app: testflight 1.0.517 (553) |
2 participants
related issue #2902
This PR includes: