From 4a89e8c330aedf897a898c7075051723ab4d37be Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 1 Nov 2024 10:05:11 +0100 Subject: [PATCH] fix(sockutls): Per review comments --- .github/workflows/sockutls_build.yml | 2 +- .../sock_utils/examples/simple/main/app.c | 16 +++--- .../examples/simple/pytest_sockutls.py | 2 +- components/sock_utils/include/gai_strerror.h | 2 +- components/sock_utils/include/ifaddrs.h | 2 +- components/sock_utils/include/poll.h | 8 +++ components/sock_utils/src/getnameinfo.c | 6 +++ components/sock_utils/src/ifaddrs.c | 45 +++++++++++----- components/sock_utils/src/socketpair.c | 34 ++++-------- .../sock_utils/test/host/CMakeLists.txt | 2 +- .../test/host/main/test_sock_utils.cpp | 52 ++++++++++++++----- 11 files changed, 112 insertions(+), 59 deletions(-) diff --git a/.github/workflows/sockutls_build.yml b/.github/workflows/sockutls_build.yml index 53199cfccde..1244c1e160a 100644 --- a/.github/workflows/sockutls_build.yml +++ b/.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 diff --git a/components/sock_utils/examples/simple/main/app.c b/components/sock_utils/examples/simple/main/app.c index 8ec83f2ed54..fd21dcf08ce 100644 --- a/components/sock_utils/examples/simple/main/app.c +++ b/components/sock_utils/examples/simple/main/app.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #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; } diff --git a/components/sock_utils/examples/simple/pytest_sockutls.py b/components/sock_utils/examples/simple/pytest_sockutls.py index a5427f58c86..08f267cd065 100644 --- a/components/sock_utils/examples/simple/pytest_sockutls.py +++ b/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"') diff --git a/components/sock_utils/include/gai_strerror.h b/components/sock_utils/include/gai_strerror.h index 2180ba1b308..ad872b982f4 100644 --- a/components/sock_utils/include/gai_strerror.h +++ b/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()`. * diff --git a/components/sock_utils/include/ifaddrs.h b/components/sock_utils/include/ifaddrs.h index 74d13ee1869..4c6cd9e3bc8 100644 --- a/components/sock_utils/include/ifaddrs.h +++ b/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; }; /** diff --git a/components/sock_utils/include/poll.h b/components/sock_utils/include/poll.h index e69de29bb2d..cdd5b79dffa 100644 --- a/components/sock_utils/include/poll.h +++ b/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 diff --git a/components/sock_utils/src/getnameinfo.c b/components/sock_utils/src/getnameinfo.c index a0d883746a3..0b939c5313b 100644 --- a/components/sock_utils/src/getnameinfo.c +++ b/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 } diff --git a/components/sock_utils/src/ifaddrs.c b/components/sock_utils/src/ifaddrs.c index 01c766f9784..bbb37814e83 100644 --- a/components/sock_utils/src/ifaddrs.c +++ b/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; } diff --git a/components/sock_utils/src/socketpair.c b/components/sock_utils/src/socketpair.c index f9bb91726f3..6573e50ef0e 100644 --- a/components/sock_utils/src/socketpair.c +++ b/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; } diff --git a/components/sock_utils/test/host/CMakeLists.txt b/components/sock_utils/test/host/CMakeLists.txt index e6ec6b7fb20..2c0886a818c 100644 --- a/components/sock_utils/test/host/CMakeLists.txt +++ b/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) diff --git a/components/sock_utils/test/host/main/test_sock_utils.cpp b/components/sock_utils/test/host/main/test_sock_utils.cpp index 934f4a56db1..e1d3facf6d7 100644 --- a/components/sock_utils/test/host/main/test_sock_utils.cpp +++ b/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]")