fix(sockutls): Per review comments

Author: david-cermak

.github/workflows/sockutls_build.yml

@@ -60,7 +60,7 @@ jobs:
           pip install idf-component-manager idf-build-apps --upgrade
           cd ${TEST_DIR}
           idf.py build
-           ./build/sockutls_host_test.elf
+          ./build/sock_utils_host_test.elf
 
   test_sock_utils:
     # Skip running on forks since it won't have access to secrets

components/sock_utils/README.md

@@ -1,13 +1,17 @@
 # Socket Utilities
 
-This component provides a simplified implementation of various socket utilities using the `lwIP` and `esp_netif` components. It is particularly useful for porting Linux-based libraries where performance and memory constraints are not a primary concern.
+This component provides simplified implementations of common socket-related utilities using `lwIP` and `esp_netif`. It is especially useful for porting Linux-based libraries to ESP32 where performance and memory constraints are secondary considerations.
+
 
 ## Supported Functions
 
-| API            | Description                        | Limitations                                      |
-|----------------|------------------------------------|--------------------------------------------------|
-| `ifaddrs()`    | Uses `esp_netif` internally        | Only supports IPv4 addresses                     |
-| `socketpair()` | Implements using two `lwIP` loopback stream sockets | Only supports IPv4 sockets      |
-| `pipe()`       | Wraps `socketpair()`               | Utilizes bidirectional sockets instead of pipes  |
-| `getnameinfo()`| Uses `lwIP`'s `inet_ntop()`        | Supports only IPv4, and only `NI_NUMERICHOST` and `NI_NUMERICSERV` flags |
-| `gai_strerror()`| Simple `itoa`-like implementation | Only returns the numeric error code as a string   |
+
+| API              | Description                                                 | Limitations                                                       |
+|------------------|-------------------------------------------------------------|-------------------------------------------------------------------|
+| `ifaddrs()`      | Retrieves interface addresses using `esp_netif`             | IPv4 addresses only                                               |
+| `socketpair()`   | Creates a pair of connected sockets using `lwIP` loopback stream sockets | IPv4 sockets only                                    |
+| `pipe()`         | Wraps `socketpair()` to provide unidirectional pipe-like functionality | Uses bidirectional sockets in place of true pipes  |
+| `getnameinfo()`  | Converts IP addresses to human-readable form using `lwIP`'s `inet_ntop()` | IPv4 only; supports `NI_NUMERICHOST` and `NI_NUMERICSERV` flags only |
+| `gai_strerror()` | Returns error code as a string                              | Simple numeric string representation only                         |
+
+**Note**: `socketpair()` and `pipe()` are built on top of `lwIP` TCP sockets, inheriting the same characteristics. For instance, the maximum transmit buffer size is based on the `TCP_SND_BUF` setting.

components/sock_utils/examples/simple/main/app.c

@@ -10,6 +10,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <net/if.h>
 #include <ifaddrs.h>
 
 #ifdef ESP_PLATFORM
@@ -60,13 +61,16 @@ static void *reader_thread(void *arg)
     while (addr) {
         if (addr->ifa_addr && addr->ifa_addr->sa_family == AF_INET) {   // look for IP4 addresses
             struct sockaddr_in *sock_addr = (struct sockaddr_in *) addr->ifa_addr;
-            if (getnameinfo((struct sockaddr *)sock_addr, sizeof(*sock_addr),
-                            buffer, sizeof(buffer), NULL, 0, NI_NUMERICHOST) != 0) {
-                ESP_LOGE(TAG, "getnameinfo() failed");
-                freeifaddrs(addresses);
-                return NULL;
+            if ((addr->ifa_flags & IFF_UP) == 0) {
+                ESP_LOGI(TAG, "Network interface \"%s\" is down", addr->ifa_name);
+            } else {
+                if (getnameinfo((struct sockaddr *)sock_addr, sizeof(*sock_addr),
+                                buffer, sizeof(buffer), NULL, 0, NI_NUMERICHOST) != 0) {
+                    freeifaddrs(addresses);
+                    return NULL;
+                }
+                ESP_LOGI(TAG, "IPv4 address of interface \"%s\": %s", addr->ifa_name, buffer);
             }
-            ESP_LOGI(TAG, "IPv4 address of interface \"%s\": %s", addr->ifa_name, buffer);
         }
         addr = addr->ifa_next;
     }

components/sock_utils/examples/simple/pytest_sockutls.py

@@ -10,4 +10,4 @@ def test_sockutls(dut):
     # Signal from the pipe simple implementation
     dut.expect('Received signal: IP4')
     # and the IPv4 address of the connected netif
-    dut.expect('IPv4 address of interface "ETH_DEF"')
+    dut.expect('IPv4 address of interface "en1"')

components/sock_utils/include/gai_strerror.h

@@ -18,7 +18,7 @@ extern "C" {
 #endif
 
 /**
-* @brief Returns a string describing a `getaddrinfo()` error code.
+* @brief Returns a numeric string representing of `getaddrinfo()` error code.
 *
 * @param[in] ecode Error code returned by `getaddrinfo()`.
 *

components/sock_utils/include/ifaddrs.h

@@ -29,7 +29,7 @@ struct ifaddrs {
     struct ifaddrs  *ifa_next;    /* Next item in list */
     char            *ifa_name;    /* Name of interface */
     struct sockaddr *ifa_addr;    /* Address of interface */
-    int ifa_flags;
+    unsigned int ifa_flags;
 };
 
 /**

components/sock_utils/include/poll.h

@@ -0,0 +1,8 @@
+/*
+ * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+// This is a placeholder for poll API
+// Note: some libraries require `poll.h` in public include dirs

components/sock_utils/src/getnameinfo.c

@@ -24,6 +24,9 @@ int getnameinfo(const struct sockaddr *addr, socklen_t addrlen,
     case AF_INET:
         // Handle numeric host address
         if (flags & NI_NUMERICHOST) {
+            if (host == NULL || hostlen == 0) {
+                return 0;
+            }
             if (inet_ntop(AF_INET, &sinp->sin_addr, host, hostlen) == NULL) {
                 return EOVERFLOW;  // Error if address couldn't be converted
             }
@@ -33,6 +36,9 @@ int getnameinfo(const struct sockaddr *addr, socklen_t addrlen,
         if (flags & NI_NUMERICSERV) {
             // Print the port number, but for UDP services (if NI_DGRAM), we treat it the same way
             int port = ntohs(sinp->sin_port);  // Convert port from network byte order
+            if (serv == NULL || servlen == 0) {
+                return 0;
+            }
             if (snprintf(serv, servlen, "%d", port) < 0) {
                 return EOVERFLOW;  // Error if snprintf failed
             }

components/sock_utils/src/ifaddrs.c

@@ -20,8 +20,9 @@ static esp_err_t getifaddrs_unsafe(void *ctx)
     struct ifaddrs **next_addr = NULL;
     struct sockaddr_in *addr_in = NULL;
     esp_netif_t *netif = NULL;
+    char if_name[5];    // lwip netif name buffer (e.g. `st1`, 2 letters + number)
     esp_netif_ip_info_t ip;
-    int ret = ESP_OK;
+    esp_err_t ret = ESP_OK;
 
     while ((netif = esp_netif_next_unsafe(netif)) != NULL) {
         ESP_GOTO_ON_FALSE((ifaddr = (struct ifaddrs *)calloc(1, sizeof(struct ifaddrs))),
@@ -31,16 +32,17 @@ static esp_err_t getifaddrs_unsafe(void *ctx)
         } else {
             *next_addr = ifaddr;    // attach next addr
         }
-        ESP_GOTO_ON_FALSE((ifaddr->ifa_name = strdup(esp_netif_get_ifkey(netif))),
+        ESP_GOTO_ON_ERROR(esp_netif_get_netif_impl_name(netif, if_name), err, TAG, "Failed to get netif name");
+        ESP_GOTO_ON_FALSE((ifaddr->ifa_name = strdup(if_name)),
                           ESP_ERR_NO_MEM, err, TAG, "Failed to allocate if name");
         ESP_GOTO_ON_FALSE((addr_in = (struct sockaddr_in *)calloc(1, sizeof(struct sockaddr_in))),
                           ESP_ERR_NO_MEM, err, TAG, "Failed to allocate addr_in");
-        ESP_GOTO_ON_ERROR(esp_netif_get_ip_info(netif, &ip), err, TAG, "Failed to allocate if name");
+        ifaddr->ifa_addr = (struct sockaddr *)addr_in;
+        ESP_GOTO_ON_ERROR(esp_netif_get_ip_info(netif, &ip), err, TAG, "Failed to get netif IP");
         ESP_LOGD(TAG, "IPv4 address: " IPSTR, IP2STR(&ip.ip));
         addr_in->sin_family = AF_INET;
         addr_in->sin_addr.s_addr = ip.ip.addr;
-        ifaddr->ifa_addr = (struct sockaddr *)addr_in;
-        ifaddr->ifa_flags = IFF_UP; // Mark the interface as UP, add more flags as needed
+        ifaddr->ifa_flags = esp_netif_is_netif_up(netif) ? IFF_UP : 0;
         next_addr = &ifaddr->ifa_next;
     }
     if (next_addr == NULL) {
@@ -51,7 +53,12 @@ static esp_err_t getifaddrs_unsafe(void *ctx)
     return ret;
 
 err:
-    freeifaddrs(ifaddr);
+    if (next_addr) {
+        *next_addr = NULL;
+        freeifaddrs(*ifap);
+    } else {
+        freeifaddrs(ifaddr);
+    }
     *ifap = NULL;
     return ret;
 
@@ -60,22 +67,34 @@ static esp_err_t getifaddrs_unsafe(void *ctx)
 int getifaddrs(struct ifaddrs **ifap)
 {
     if (ifap == NULL) {
+        errno = EINVAL;
         return -1; // Invalid argument
     }
 
-    return esp_netif_tcpip_exec(getifaddrs_unsafe, ifap) == ESP_OK ? 0 : -1;
+    esp_err_t ret = esp_netif_tcpip_exec(getifaddrs_unsafe, ifap);
+    switch (ret) {
+    case ESP_OK:
+        return 0;
+    case ESP_ERR_NO_MEM:
+        errno = ENOMEM;
+        return -1;
+    case ESP_ERR_INVALID_ARG:
+    case ESP_ERR_ESP_NETIF_INVALID_PARAMS:
+        errno = EINVAL;
+        return -1;
+    default:
+    case ESP_FAIL:
+        errno = EIO;    // Generic I/O Error
+        return  -1;
+    }
 }
 
 void freeifaddrs(struct ifaddrs *ifa)
 {
     while (ifa != NULL) {
         struct ifaddrs *next = ifa->ifa_next;
-        if (ifa->ifa_name) {
-            free(ifa->ifa_name);
-        }
-        if (ifa->ifa_addr) {
-            free(ifa->ifa_addr);
-        }
+        free(ifa->ifa_name);
+        free(ifa->ifa_addr);
         free(ifa);
         ifa = next;
     }

components/sock_utils/src/socketpair.c

@@ -12,21 +12,12 @@
 
 static const char *TAG = "socket_helpers";
 
-static int set_nonblocking(int sock)
+int socketpair(int domain, int type, int protocol, int sv[2])
 {
-    int opt;
-    opt = fcntl(sock, F_GETFL, 0);
-    if (opt == -1) {
-        return -1;
-    }
-    if (fcntl(sock, F_SETFL, opt | O_NONBLOCK) == -1) {
+    if (protocol != 0 || type != SOCK_STREAM || domain != AF_UNIX) {
+        errno = ENOSYS; // not implemented
         return -1;
     }
-    return 0;
-}
-
-int socketpair(int domain, int type, int protocol, int sv[2])
-{
     struct sockaddr_storage ss;
     struct sockaddr_in *sa = (struct sockaddr_in *)&ss;
     socklen_t ss_len = sizeof(struct sockaddr_in);
@@ -51,15 +42,9 @@ int socketpair(int domain, int type, int protocol, int sv[2])
 
     fd1 = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
     ESP_GOTO_ON_FALSE(fd1 != INVALID_SOCKET, -1, err, TAG, "Cannot create read side socket");
-    ESP_GOTO_ON_FALSE(set_nonblocking(fd1) == 0, -1, err, TAG, "Failed to set socket to nonblocking mode");
-    if (connect(fd1, (struct sockaddr *)&ss, ss_len) < 0) {
-        ESP_GOTO_ON_FALSE(errno == EINPROGRESS || errno == EWOULDBLOCK, -1, err, TAG, "Failed to connect fd1");
-    }
+    ESP_GOTO_ON_FALSE(connect(fd1, (struct sockaddr *)&ss, ss_len) >= 0, -1, err, TAG, "Failed to connect fd1");
     fd2 = accept(listenfd, NULL, 0);
-    if (fd2 == -1) {
-        ESP_GOTO_ON_FALSE(errno == EINPROGRESS || errno == EWOULDBLOCK, -1, err, TAG, "Failed to accept fd2");
-    }
-    ESP_GOTO_ON_FALSE(set_nonblocking(fd2) == 0, -1, err, TAG, "Failed to set socket to nonblocking mode");
+    ESP_GOTO_ON_FALSE(fd2 != INVALID_SOCKET, -1, err, TAG, "Failed to accept fd2");
 
     close(listenfd);
     sv[0] = fd1;
@@ -73,9 +58,6 @@ int socketpair(int domain, int type, int protocol, int sv[2])
     if (fd1 != INVALID_SOCKET) {
         close(fd1);
     }
-    if (fd2 != INVALID_SOCKET) {
-        close(fd2);
-    }
     return ret;
 }
 
@@ -84,5 +66,11 @@ int pipe(int pipefd[2])
     if (socketpair(AF_UNIX, SOCK_STREAM, 0, pipefd) == -1) {
         return -1;
     }
+    // Close the unwanted ends to make it a unidirectional pipe
+    if (shutdown(pipefd[0], SHUT_WR) == -1 || shutdown(pipefd[1], SHUT_RD) == -1) {
+        close(pipefd[0]);
+        close(pipefd[1]);
+        return -1;
+    }
     return 0;
 }

components/sock_utils/test/host/CMakeLists.txt

@@ -4,4 +4,4 @@ set(COMPONENTS main)
 
 include($ENV{IDF_PATH}/tools/cmake/project.cmake)
 
-project(sockutls_host_test)
+project(sock_utils_host_test)

components/sock_utils/test/host/main/test_sock_utils.cpp

@@ -9,28 +9,50 @@
 #include "catch2/catch_test_macros.hpp"
 #include "catch2/catch_session.hpp"
 
+#define TEST_PORT_NUMBER 3333
+#define TEST_PORT_STRING "3333"
+
 static char buffer[64];
 
-static esp_netif_t *create_test_netif(const char *if_key)
+static esp_err_t dummy_transmit(void *h, void *buffer, size_t len)
+{
+    return ESP_OK;
+}
+
+static esp_err_t dummy_transmit_wrap(void *h, void *buffer, size_t len, void *pbuf)
+{
+    return ESP_OK;
+}
+
+static esp_netif_t *create_test_netif(const char *if_key, uint8_t last_octet)
 {
     esp_netif_inherent_config_t base_cfg = ESP_NETIF_INHERENT_DEFAULT_WIFI_STA();
     esp_netif_ip_info_t ip = { };
-    ip.ip.addr = ESP_IP4TOADDR(1, 2, 3, 4);
+    ip.ip.addr = ESP_IP4TOADDR(1, 2, 3, last_octet);
     base_cfg.ip_info = &ip;
     base_cfg.if_key = if_key;
-    esp_netif_config_t cfg = { .base = &base_cfg, .driver = NULL, .stack = ESP_NETIF_NETSTACK_DEFAULT_WIFI_STA };
-    return esp_netif_new(&cfg);
+    // create a dummy driver, so the netif could be started and set up
+    base_cfg.flags = ESP_NETIF_FLAG_AUTOUP;
+    esp_netif_driver_ifconfig_t driver_cfg = {};
+    driver_cfg.handle = (esp_netif_iodriver_handle)1;
+    driver_cfg.transmit = dummy_transmit;
+    driver_cfg.transmit_wrap = dummy_transmit_wrap;
+    esp_netif_config_t cfg = { .base = &base_cfg, .driver = &driver_cfg, .stack = ESP_NETIF_NETSTACK_DEFAULT_WIFI_STA };
+    esp_netif_t *netif = esp_netif_new(&cfg);
+    // need to start the netif to be considered in tests
+    esp_netif_action_start(netif, nullptr, 0, nullptr);
+    return netif;
 }
 
 TEST_CASE("esp_getnameinfo() for IPv4", "[sock_utils]")
 {
     struct sockaddr_in sock_addr = {};
     sock_addr.sin_family = AF_INET;
-    sock_addr.sin_port = 257; //  (0x0101: same number for LE and BE)
+    sock_addr.sin_port = htons(TEST_PORT_NUMBER); // sock_addr fields are in network byte order
     REQUIRE(esp_getnameinfo((struct sockaddr *)&sock_addr, sizeof(sock_addr), buffer, sizeof(buffer), NULL, 0, NI_NUMERICHOST) == 0);
     CHECK(strcmp("0.0.0.0", buffer) == 0);
     CHECK(esp_getnameinfo((struct sockaddr *)&sock_addr, sizeof(sock_addr), NULL, 0, buffer, sizeof(buffer), NI_NUMERICSERV) == 0);
-    CHECK(strcmp("257", buffer) == 0);
+    CHECK(strcmp(TEST_PORT_STRING, buffer) == 0);
 }
 
 TEST_CASE("esp_getnameinfo() for IPv6", "[sock_utils]")
@@ -58,7 +80,13 @@ static void test_getifaddr(int expected_nr_of_addrs)
                 printf("esp_getnameinfo() failed\n");
             } else {
                 printf("IPv4 address of interface \"%s\": %s\n", addr->ifa_name, buffer);
-                CHECK(strcmp("1.2.3.4", buffer) == 0);
+                if (strcmp(addr->ifa_name, "st1") == 0) {
+                    CHECK(strcmp("1.2.3.1", buffer) == 0);
+                } else if (strcmp(addr->ifa_name, "st2") == 0) {
+                    CHECK(strcmp("1.2.3.2", buffer) == 0);
+                } else {
+                    FAIL("unexpected network interface");
+                }
             }
         }
         addr = addr->ifa_next;
@@ -71,10 +99,10 @@ static void test_getifaddr(int expected_nr_of_addrs)
 TEST_CASE("esp_getifaddrs() with 0, 1, and 2 addresses", "[sock_utils]")
 {
     test_getifaddr(0);
-    esp_netif_t *esp_netif = create_test_netif("station");
+    esp_netif_t *esp_netif = create_test_netif("station", 1);   // st1
     REQUIRE(esp_netif != NULL);
     test_getifaddr(1);
-    esp_netif_t *esp_netif2 = create_test_netif("station2");
+    esp_netif_t *esp_netif2 = create_test_netif("station2", 2); // st2
     REQUIRE(esp_netif2 != NULL);
     test_getifaddr(2);
     esp_netif_destroy(esp_netif);
@@ -85,10 +113,10 @@ static void test_pipe(int read_fd, int write_fd)
 {
     CHECK(read_fd  >= 0);
     CHECK(write_fd >= 0);
-    CHECK(read(read_fd, buffer, sizeof(buffer)) < 0);
     CHECK(write(write_fd, buffer, 1) > 0);
-    CHECK(read(read_fd, buffer, sizeof(buffer)) == 1);
-
+    CHECK(write(write_fd, buffer, 1) > 0);
+    CHECK(read(read_fd, buffer, 1) == 1);
+    CHECK(read(read_fd, buffer, 1) == 1);
 }
 
 TEST_CASE("socketpair()", "[sock_utils]")