fix(net_connect): improve error handling and memory safety

abhik-roy85 · abhik-roy85 · commit 76a84fa4bd64 · 2025-12-05T21:31:51.000+11:00
Addresses code review feedback from GitHub cursor bot.
diff --git a/components/net_connect/CMakeLists.txt b/components/net_connect/CMakeLists.txt
@@ -33,7 +33,7 @@ endif()
 
 idf_component_register(SRCS "${srcs}"
                        INCLUDE_DIRS "include"
-                       PRIV_REQUIRES esp_netif esp_driver_gpio esp_driver_uart esp_wifi vfs console openthread)
+                       PRIV_REQUIRES esp_netif esp_driver_gpio esp_driver_uart esp_wifi vfs openthread)
 
 if(CONFIG_NET_CONNECT_PROVIDE_WIFI_CONSOLE_CMD)
     idf_component_optional_requires(PRIVATE console)
diff --git a/components/net_connect/addr_from_stdin.c b/components/net_connect/addr_from_stdin.c
@@ -30,18 +30,26 @@ esp_err_t get_addr_from_stdin(int port, int sock_type, int *ip_protocol, int *ad
 
     // ignore empty or LF only string (could receive from DUT class)
     do {
-        fgets(host_ip, HOST_IP_SIZE, stdin);
+        if (fgets(host_ip, HOST_IP_SIZE, stdin) == NULL) {
+            // EOF or error occurred
+            return ESP_FAIL;
+        }
         len = strlen(host_ip);
     } while (len <= 1 && host_ip[0] == '\n');
-    host_ip[len - 1] = '\0';
+
+    // Remove trailing newline if present
+    if (len > 0 && host_ip[len - 1] == '\n') {
+        host_ip[len - 1] = '\0';
+    }
 
     struct addrinfo hints, *addr_list, *cur;
     memset(&hints, 0, sizeof(hints));
 
     // run getaddrinfo() to decide on the IP protocol
     hints.ai_family = AF_UNSPEC;
     hints.ai_socktype = sock_type;
-    hints.ai_protocol = IPPROTO_TCP;
+    // Set protocol based on socket type, or 0 to let getaddrinfo infer it
+    hints.ai_protocol = (sock_type == SOCK_DGRAM) ? IPPROTO_UDP : IPPROTO_TCP;
     if (getaddrinfo(host_ip, NULL, &hints, &addr_list) != 0) {
         return ESP_FAIL;
     }
diff --git a/components/net_connect/console_cmd.c b/components/net_connect/console_cmd.c
@@ -43,7 +43,10 @@ static int cmd_do_wifi_connect(int argc, char **argv)
         wifi_config.sta.channel = (uint8_t)(connect_args.channel->ival[0]);
     }
     const char *ssid = connect_args.ssid->sval[0];
-    const char *pass = connect_args.password->sval[0];
+    const char *pass = NULL;
+    if (connect_args.password->count > 0) {
+        pass = connect_args.password->sval[0];
+    }
     strlcpy((char *) wifi_config.sta.ssid, ssid, sizeof(wifi_config.sta.ssid));
     if (pass) {
         strlcpy((char *) wifi_config.sta.password, pass, sizeof(wifi_config.sta.password));
diff --git a/components/net_connect/eth_connect.c b/components/net_connect/eth_connect.c
@@ -151,7 +151,10 @@ esp_err_t net_connect_ethernet_connect(void)
 #if CONFIG_NET_CONNECT_IPV6
     s_semph_get_ip6_addrs = xSemaphoreCreateBinary();
     if (s_semph_get_ip6_addrs == NULL) {
+#if CONFIG_NET_CONNECT_IPV4
         vSemaphoreDelete(s_semph_get_ip_addrs);
+        s_semph_get_ip_addrs = NULL;
+#endif
         return ESP_ERR_NO_MEM;
     }
 #endif
diff --git a/components/net_connect/ppp_connect.c b/components/net_connect/ppp_connect.c
@@ -115,6 +115,10 @@ static void cdc_rx_callback(int itf, cdcacm_event_t *event)
         // Not our channel
         return;
     }
+    // Check if netif is still valid (not destroyed during shutdown)
+    if (s_netif == NULL) {
+        return;
+    }
     esp_err_t ret = tinyusb_cdcacm_read(itf, buf, CONFIG_TINYUSB_CDC_RX_BUFSIZE, &rx_size);
     if (ret == ESP_OK) {
         ESP_LOG_BUFFER_HEXDUMP(TAG, buf, rx_size, ESP_LOG_VERBOSE);
@@ -149,22 +153,29 @@ static void ppp_task(void *args)
     ESP_ERROR_CHECK(uart_set_rx_timeout(UART_NUM_1, 1));
 
     char *buffer = (char*)malloc(BUF_SIZE);
+    if (buffer == NULL) {
+        ESP_LOGE(TAG, "Failed to allocate memory for UART buffer");
+        uart_driver_delete(UART_NUM_1);
+        vTaskDelete(NULL);
+        return;
+    }
     uart_event_t event;
     esp_event_handler_register(IP_EVENT, IP_EVENT_PPP_GOT_IP, esp_netif_action_connected, s_netif);
     esp_netif_action_start(s_netif, 0, 0, 0);
     esp_netif_action_connected(s_netif, 0, 0, 0);
     while (!s_stop_task) {
-        xQueueReceive(event_queue, &event, pdMS_TO_TICKS(1000));
-        if (event.type == UART_DATA) {
-            size_t len;
-            uart_get_buffered_data_len(UART_NUM_1, &len);
-            if (len) {
-                len = uart_read_bytes(UART_NUM_1, buffer, BUF_SIZE, 0);
-                ESP_LOG_BUFFER_HEXDUMP(TAG, buffer, len, ESP_LOG_VERBOSE);
-                esp_netif_receive(s_netif, buffer, len, NULL);
+        if (xQueueReceive(event_queue, &event, pdMS_TO_TICKS(1000)) == pdTRUE) {
+            if (event.type == UART_DATA) {
+                size_t len;
+                uart_get_buffered_data_len(UART_NUM_1, &len);
+                if (len) {
+                    len = uart_read_bytes(UART_NUM_1, buffer, BUF_SIZE, 0);
+                    ESP_LOG_BUFFER_HEXDUMP(TAG, buffer, len, ESP_LOG_VERBOSE);
+                    esp_netif_receive(s_netif, buffer, len, NULL);
+                }
+            } else {
+                ESP_LOGW(TAG, "Received UART event: %d", event.type);
             }
-        } else {
-            ESP_LOGW(TAG, "Received UART event: %d", event.type);
         }
     }
     free(buffer);
@@ -243,7 +254,12 @@ esp_err_t net_connect_ppp_connect(void)
 void net_connect_ppp_shutdown(void)
 {
     ESP_ERROR_CHECK(esp_event_handler_unregister(IP_EVENT, ESP_EVENT_ANY_ID, on_ip_event));
-#if CONFIG_NET_CONNECT_PPP_DEVICE_UART
+#if CONFIG_NET_CONNECT_PPP_DEVICE_USB
+    // Unregister CDC line state callback to prevent access to destroyed netif
+    // Note: RX callback is set via config and cannot be unregistered, but cdc_rx_callback
+    // has a NULL check to prevent crashes if called after shutdown
+    tinyusb_cdcacm_register_callback(TINYUSB_CDC_ACM_0, CDC_EVENT_LINE_STATE_CHANGED, NULL);
+#elif CONFIG_NET_CONNECT_PPP_DEVICE_UART
     s_stop_task = true;
     vTaskDelay(pdMS_TO_TICKS(1000)); // wait for the ppp task to stop
 #endif
diff --git a/components/net_connect/stdin_out.c b/components/net_connect/stdin_out.c
@@ -10,7 +10,7 @@
 #include "driver/uart.h"
 #include "sdkconfig.h"
 
-esp_err_t example_configure_stdin_stdout(void)
+esp_err_t net_configure_stdin_stdout(void)
 {
     if (uart_is_driver_installed((uart_port_t)CONFIG_ESP_CONSOLE_UART_NUM)) {
         return ESP_OK;
diff --git a/components/net_connect/thread_connect.c b/components/net_connect/thread_connect.c
@@ -86,7 +86,10 @@ static void ot_task_worker(void *aContext)
 /* tear down connection, release resources */
 void net_connect_thread_shutdown(void)
 {
-    vTaskDelete(s_ot_task_handle);
+    if (s_ot_task_handle != NULL) {
+        vTaskDelete(s_ot_task_handle);
+        s_ot_task_handle = NULL;
+    }
     esp_openthread_netif_glue_deinit();
     esp_netif_destroy(s_openthread_netif);
     esp_vfs_eventfd_unregister();
diff --git a/components/net_connect/wifi_connect.c b/components/net_connect/wifi_connect.c
@@ -185,14 +185,19 @@ void net_connect_wifi_stop(void)
 esp_err_t net_connect_wifi_sta_do_connect(wifi_config_t wifi_config, bool wait)
 {
     if (wait) {
+#if CONFIG_NET_CONNECT_IPV4
         s_semph_get_ip_addrs = xSemaphoreCreateBinary();
         if (s_semph_get_ip_addrs == NULL) {
             return ESP_ERR_NO_MEM;
         }
+#endif
 #if CONFIG_NET_CONNECT_IPV6
         s_semph_get_ip6_addrs = xSemaphoreCreateBinary();
         if (s_semph_get_ip6_addrs == NULL) {
+#if CONFIG_NET_CONNECT_IPV4
             vSemaphoreDelete(s_semph_get_ip_addrs);
+            s_semph_get_ip_addrs = NULL;
+#endif
             return ESP_ERR_NO_MEM;
         }
 #endif
@@ -293,18 +298,29 @@ esp_err_t net_connect_wifi(void)
     net_configure_stdin_stdout();
     char buf[sizeof(wifi_config.sta.ssid) + sizeof(wifi_config.sta.password) + 2] = {0};
     ESP_LOGI(TAG, "Please input ssid password:");
-    fgets(buf, sizeof(buf), stdin);
+    if (fgets(buf, sizeof(buf), stdin) == NULL) {
+        ESP_LOGE(TAG, "Failed to read SSID/password from stdin (EOF or error)");
+        return ESP_ERR_INVALID_STATE;
+    }
     int len = strlen(buf);
-    buf[len - 1] = '\0'; /* removes '\n' */
+    if (len > 0) {
+        buf[len - 1] = '\0'; /* removes '\n' */
+    }
     memset(wifi_config.sta.ssid, 0, sizeof(wifi_config.sta.ssid));
 
     char *rest = NULL;
     char *temp = strtok_r(buf, " ", &rest);
-    strncpy((char*)wifi_config.sta.ssid, temp, sizeof(wifi_config.sta.ssid));
+    if (temp == NULL) {
+        ESP_LOGE(TAG, "SSID is empty or invalid");
+        return ESP_ERR_INVALID_ARG;
+    }
+    strncpy((char*)wifi_config.sta.ssid, temp, sizeof(wifi_config.sta.ssid) - 1);
+    wifi_config.sta.ssid[sizeof(wifi_config.sta.ssid) - 1] = '\0';
     memset(wifi_config.sta.password, 0, sizeof(wifi_config.sta.password));
     temp = strtok_r(NULL, " ", &rest);
     if (temp) {
-        strncpy((char*)wifi_config.sta.password, temp, sizeof(wifi_config.sta.password));
+        strncpy((char*)wifi_config.sta.password, temp, sizeof(wifi_config.sta.password) - 1);
+        wifi_config.sta.password[sizeof(wifi_config.sta.password) - 1] = '\0';
     } else {
         wifi_config.sta.threshold.authmode = WIFI_AUTH_OPEN;
     }

Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@`
`10`	`10`	`#include "driver/uart.h"`
`11`	`11`	`#include "sdkconfig.h"`
`12`	`12`
`13`		`-esp_err_t example_configure_stdin_stdout(void)`
	`13`	`+esp_err_t net_configure_stdin_stdout(void)`
`14`	`14`	`{`
`15`	`15`	`if (uart_is_driver_installed((uart_port_t)CONFIG_ESP_CONSOLE_UART_NUM)) {`
`16`	`16`	`return ESP_OK;`