From 42fbdec4e0209bc4eee2f2639fac3b3d2f1555af Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Wed, 11 Jun 2025 23:36:23 -0700 Subject: [PATCH 01/15] Refactor UdpSocket to remove SocketState and the use of dynamic memory allocation. --- Drv/Ip/UdpSocket.cpp | 43 ++++++++++++++++++------------------------- Drv/Ip/UdpSocket.hpp | 16 +++++++++++++--- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index 923f6589d92..cc9bba426a2 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -40,23 +40,13 @@ namespace Drv { -struct SocketState { - struct sockaddr_in m_addr_send; //!< UDP server address, maybe unused - struct sockaddr_in m_addr_recv; //!< UDP server address, maybe unused - - SocketState() { - ::memset(&m_addr_send, 0, sizeof(m_addr_send)); - ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); - } -}; - -UdpSocket::UdpSocket() : IpSocket(), m_state(new(std::nothrow) SocketState), m_recv_port(0), m_recv_configured(false) { - FW_ASSERT(m_state != nullptr); +UdpSocket::UdpSocket() : IpSocket(), m_recv_port(0), m_recv_configured(false) { + ::memset(&m_addr_send, 0, sizeof(m_addr_send)); + ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); } UdpSocket::~UdpSocket() { - FW_ASSERT(m_state); - delete m_state; + // No dynamic memory to clean up } SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) { @@ -68,10 +58,12 @@ SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, SocketIpStatus UdpSocket::configureSend(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) { //Timeout is for the send, so configure send will work with the base class FW_ASSERT(hostname != nullptr); - return this->IpSocket::configure(hostname, port, timeout_seconds, timeout_microseconds); + // Call the base class configure method directly + return IpSocket::configure(hostname, port, timeout_seconds, timeout_microseconds); } SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) { + // Call isValidPort directly as a member of this class FW_ASSERT(this->isValidPort(port)); FW_ASSERT(hostname != nullptr); this->m_recv_port = port; @@ -115,8 +107,8 @@ SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { // Update m_recv_port with the actual port assigned (for ephemeral port support) this->m_recv_port = ntohs(address.sin_port); - FW_ASSERT(sizeof(this->m_state->m_addr_recv) == sizeof(address), static_cast(sizeof(this->m_state->m_addr_recv)), static_cast(sizeof(address))); - memcpy(&this->m_state->m_addr_recv, &address, sizeof(this->m_state->m_addr_recv)); + FW_ASSERT(sizeof(this->m_addr_recv) == sizeof(address), static_cast(sizeof(this->m_addr_recv)), static_cast(sizeof(address))); + memcpy(&this->m_addr_recv, &address, sizeof(this->m_addr_recv)); return SOCK_SUCCESS; } @@ -152,13 +144,14 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { }; // Now apply timeouts - if ((status = IpSocket::setupTimeouts(socketFd)) != SOCK_SUCCESS) { + // Call setupTimeouts directly as a member of this class + if ((status = this->setupTimeouts(socketFd)) != SOCK_SUCCESS) { ::close(socketFd); return status; } - FW_ASSERT(sizeof(this->m_state->m_addr_send) == sizeof(address), static_cast(sizeof(this->m_state->m_addr_send)), + FW_ASSERT(sizeof(this->m_addr_send) == sizeof(address), static_cast(sizeof(this->m_addr_send)), static_cast(sizeof(address))); - memcpy(&this->m_state->m_addr_send, &address, sizeof(this->m_state->m_addr_send)); + memcpy(&this->m_addr_send, &address, sizeof(this->m_addr_send)); } // Only bind if configureRecv was called (including ephemeral) @@ -186,21 +179,21 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { } I32 UdpSocket::sendProtocol(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { - FW_ASSERT(this->m_state->m_addr_send.sin_family != 0); // Make sure the address was previously setup + FW_ASSERT(this->m_addr_send.sin_family != 0); // Make sure the address was previously setup return static_cast(::sendto(socketDescriptor.fd, data, size, SOCKET_IP_SEND_FLAGS, - reinterpret_cast(&this->m_state->m_addr_send), sizeof(this->m_state->m_addr_send))); + reinterpret_cast(&this->m_addr_send), sizeof(this->m_addr_send))); } I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) { - FW_ASSERT(this->m_state->m_addr_recv.sin_family != 0); // Make sure the address was previously setup + FW_ASSERT(this->m_addr_recv.sin_family != 0); // Make sure the address was previously setup struct sockaddr_in sender_addr; socklen_t sender_addr_len = sizeof(sender_addr); I32 received = static_cast(::recvfrom(socketDescriptor.fd, data, size, SOCKET_IP_RECV_FLAGS, reinterpret_cast(&sender_addr), &sender_addr_len)); // If we have not configured a send port, set it to the source of the last received packet - if (received > 0 && this->m_state->m_addr_send.sin_port == 0) { - this->m_state->m_addr_send = sender_addr; + if (received > 0 && this->m_addr_send.sin_port == 0) { + this->m_addr_send = sender_addr; this->m_port = ntohs(sender_addr.sin_port); Fw::Logger::log("Configured send port to %hu as specified by the last received packet.\n", this->m_port); } diff --git a/Drv/Ip/UdpSocket.hpp b/Drv/Ip/UdpSocket.hpp index 98ef4929d45..8ca03f98530 100644 --- a/Drv/Ip/UdpSocket.hpp +++ b/Drv/Ip/UdpSocket.hpp @@ -16,9 +16,18 @@ #include #include -namespace Drv { +// Include system headers for sockaddr_in +#ifdef TGT_OS_TYPE_VXWORKS + #include + #include +#elif defined TGT_OS_TYPE_LINUX || TGT_OS_TYPE_DARWIN + #include + #include +#else + #error OS not supported for IP Socket Communications +#endif -struct SocketState; +namespace Drv { /** * \brief Helper for setting up Udp using Berkeley sockets as a client @@ -122,7 +131,8 @@ class UdpSocket : public IpSocket { */ I32 recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) override; private: - SocketState* m_state; //!< State storage + struct sockaddr_in m_addr_send; //!< UDP server address for sending + struct sockaddr_in m_addr_recv; //!< UDP server address for receiving U16 m_recv_port; //!< Port to receive on CHAR m_recv_hostname[SOCKET_MAX_HOSTNAME_SIZE]; //!< Hostname to receive on bool m_recv_configured; //!< True if configureRecv was called From 85b4568abd39dfb856dd72266b3c0c0939c75ea4 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Thu, 12 Jun 2025 00:10:15 -0700 Subject: [PATCH 02/15] Replacing some ASSERTs with proper SocketIpStatus. Adding some input argument validation. Adding some better logging. --- Drv/Ip/UdpSocket.cpp | 50 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index cc9bba426a2..dd28d7aafd1 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -45,9 +45,7 @@ UdpSocket::UdpSocket() : IpSocket(), m_recv_port(0), m_recv_configured(false) { ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); } -UdpSocket::~UdpSocket() { - // No dynamic memory to clean up -} +UdpSocket::~UdpSocket() = default; SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) { FW_ASSERT(0); // Must use configureSend and/or configureRecv @@ -57,15 +55,28 @@ SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, SocketIpStatus UdpSocket::configureSend(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) { //Timeout is for the send, so configure send will work with the base class - FW_ASSERT(hostname != nullptr); + if (hostname == nullptr) { + return SOCK_INVALID_IP_ADDRESS; + } + if (timeout_microseconds >= 1000000) { + return SOCK_FAILED_TO_SET_SOCKET_OPTIONS; + } // Call the base class configure method directly return IpSocket::configure(hostname, port, timeout_seconds, timeout_microseconds); } SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) { - // Call isValidPort directly as a member of this class - FW_ASSERT(this->isValidPort(port)); - FW_ASSERT(hostname != nullptr); + if (hostname == nullptr) { + return SOCK_INVALID_IP_ADDRESS; + } + if (!this->isValidPort(port)) { + return SOCK_INVALID_CALL; + } + // Check hostname length to prevent potential buffer overflow + if (strlen(hostname) >= SOCKET_MAX_HOSTNAME_SIZE) { + return SOCK_INVALID_IP_ADDRESS; + } + this->m_recv_port = port; (void) Fw::StringUtils::string_copy(this->m_recv_hostname, hostname, static_cast(SOCKET_MAX_HOSTNAME_SIZE)); this->m_recv_configured = true; @@ -73,14 +84,18 @@ SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) { } U16 UdpSocket::getRecvPort() { - U16 port = this->m_recv_port; - return port; + return this->m_recv_port; } SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { + if (fd < 0) { + return SOCK_FAILED_TO_BIND; + } + + // Initialize address structure to zero before use struct sockaddr_in address; - FW_ASSERT(fd != -1); + ::memset(&address, 0, sizeof(address)); // Set up the address port and name address.sin_family = AF_INET; @@ -114,9 +129,17 @@ SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { } SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { + // Add validation for edge case where neither send nor receive is configured + if (this->m_port == 0 && !this->m_recv_configured) { + return SOCK_INVALID_CALL; // Neither send nor receive is configured + } + SocketIpStatus status = SOCK_SUCCESS; PlatformIntType socketFd = -1; + + // Initialize address structure to zero before use struct sockaddr_in address; + ::memset(&address, 0, sizeof(address)); U16 port = this->m_port; U16 recv_port = this->m_recv_port; @@ -139,12 +162,12 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { // First IP address to socket sin_addr if ((status = IpSocket::addressToIp4(m_hostname, &(address.sin_addr))) != SOCK_SUCCESS) { + Fw::Logger::log("Failed to resolve hostname %s: %d\n", m_hostname, static_cast(status)); ::close(socketFd); return status; }; // Now apply timeouts - // Call setupTimeouts directly as a member of this class if ((status = this->setupTimeouts(socketFd)) != SOCK_SUCCESS) { ::close(socketFd); return status; @@ -180,14 +203,19 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { I32 UdpSocket::sendProtocol(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { FW_ASSERT(this->m_addr_send.sin_family != 0); // Make sure the address was previously setup + FW_ASSERT(socketDescriptor.fd < 0 || data == nullptr || size == 0); + return static_cast(::sendto(socketDescriptor.fd, data, size, SOCKET_IP_SEND_FLAGS, reinterpret_cast(&this->m_addr_send), sizeof(this->m_addr_send))); } I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) { FW_ASSERT(this->m_addr_recv.sin_family != 0); // Make sure the address was previously setup + FW_ASSERT(socketDescriptor.fd < 0 || data == nullptr || size == 0); + // Initialize sender address structure to zero struct sockaddr_in sender_addr; + ::memset(&sender_addr, 0, sizeof(sender_addr)); socklen_t sender_addr_len = sizeof(sender_addr); I32 received = static_cast(::recvfrom(socketDescriptor.fd, data, size, SOCKET_IP_RECV_FLAGS, reinterpret_cast(&sender_addr), &sender_addr_len)); From 5d00932150ef18917d643ce666c58331d0377ea7 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Thu, 12 Jun 2025 00:22:44 -0700 Subject: [PATCH 03/15] Refactoring UdpSocket to remove redundant m_recv_port and m_recv_hostname members --- Drv/Ip/UdpSocket.cpp | 57 ++++++++++++++++++++++++-------------------- Drv/Ip/UdpSocket.hpp | 2 -- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index dd28d7aafd1..ea246bf42b5 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -40,7 +40,7 @@ namespace Drv { -UdpSocket::UdpSocket() : IpSocket(), m_recv_port(0), m_recv_configured(false) { +UdpSocket::UdpSocket() : IpSocket(), m_recv_configured(false) { ::memset(&m_addr_send, 0, sizeof(m_addr_send)); ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); } @@ -77,14 +77,22 @@ SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) { return SOCK_INVALID_IP_ADDRESS; } - this->m_recv_port = port; - (void) Fw::StringUtils::string_copy(this->m_recv_hostname, hostname, static_cast(SOCKET_MAX_HOSTNAME_SIZE)); + // Initialize the receive address structure + ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); + m_addr_recv.sin_family = AF_INET; + m_addr_recv.sin_port = htons(port); + + // Convert hostname to IP address + if (IpSocket::addressToIp4(hostname, &m_addr_recv.sin_addr) != SOCK_SUCCESS) { + return SOCK_INVALID_IP_ADDRESS; + } + this->m_recv_configured = true; return SOCK_SUCCESS; } U16 UdpSocket::getRecvPort() { - return this->m_recv_port; + return ntohs(this->m_addr_recv.sin_port); } @@ -93,22 +101,13 @@ SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { return SOCK_FAILED_TO_BIND; } - // Initialize address structure to zero before use - struct sockaddr_in address; - ::memset(&address, 0, sizeof(address)); - - // Set up the address port and name - address.sin_family = AF_INET; - address.sin_port = htons(this->m_recv_port); + // Use a local copy of the address structure + struct sockaddr_in address = this->m_addr_recv; + // OS specific settings #if defined TGT_OS_TYPE_VXWORKS || TGT_OS_TYPE_DARWIN address.sin_len = static_cast(sizeof(struct sockaddr_in)); #endif - - // First IP address to socket sin_addr - if (IpSocket::addressToIp4(m_recv_hostname, &address.sin_addr) != SOCK_SUCCESS) { - return SOCK_INVALID_IP_ADDRESS; - }; // UDP (for receiving) requires bind to an address to the socket if (::bind(fd, reinterpret_cast(&address), sizeof(address)) < 0) { return SOCK_FAILED_TO_BIND; @@ -119,17 +118,15 @@ SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { return SOCK_FAILED_TO_READ_BACK_PORT; } - // Update m_recv_port with the actual port assigned (for ephemeral port support) - this->m_recv_port = ntohs(address.sin_port); + // Update m_addr_recv with the actual port assigned (for ephemeral port support) + this->m_addr_recv.sin_port = address.sin_port; - FW_ASSERT(sizeof(this->m_addr_recv) == sizeof(address), static_cast(sizeof(this->m_addr_recv)), static_cast(sizeof(address))); - memcpy(&this->m_addr_recv, &address, sizeof(this->m_addr_recv)); + // No need to copy the entire structure since we only updated the port return SOCK_SUCCESS; } SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { - // Add validation for edge case where neither send nor receive is configured if (this->m_port == 0 && !this->m_recv_configured) { return SOCK_INVALID_CALL; // Neither send nor receive is configured } @@ -142,7 +139,7 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { ::memset(&address, 0, sizeof(address)); U16 port = this->m_port; - U16 recv_port = this->m_recv_port; + U16 recv_port = ntohs(this->m_addr_recv.sin_port); // Acquire a socket, or return error if ((socketFd = ::socket(AF_INET, SOCK_DGRAM, 0)) == -1) { @@ -188,12 +185,15 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { } // Log message for UDP + char recv_ip[INET_ADDRSTRLEN]; + inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_ip, INET_ADDRSTRLEN); + if ((port == 0) && (recv_port > 0)) { - Fw::Logger::log("Setup to only receive udp at %s:%hu\n", m_recv_hostname, recv_port); + Fw::Logger::log("Setup to only receive udp at %s:%hu\n", recv_ip, recv_port); } else if ((port > 0) && (recv_port == 0)) { Fw::Logger::log("Setup to only send udp at %s:%hu\n", m_hostname, port); } else if ((port > 0) && (recv_port > 0)) { - Fw::Logger::log("Setup to receive udp at %s:%hu and send to %s:%hu\n", m_recv_hostname, recv_port, m_hostname, port); + Fw::Logger::log("Setup to receive udp at %s:%hu and send to %s:%hu\n", recv_ip, recv_port, m_hostname, port); } FW_ASSERT(status == SOCK_SUCCESS, static_cast(status)); @@ -203,7 +203,9 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { I32 UdpSocket::sendProtocol(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { FW_ASSERT(this->m_addr_send.sin_family != 0); // Make sure the address was previously setup - FW_ASSERT(socketDescriptor.fd < 0 || data == nullptr || size == 0); + FW_ASSERT(socketDescriptor.fd >= 0); // File descriptor should be valid + FW_ASSERT(data != nullptr); // Data pointer should not be null + FW_ASSERT(size > 0); // Size should be positive return static_cast(::sendto(socketDescriptor.fd, data, size, SOCKET_IP_SEND_FLAGS, reinterpret_cast(&this->m_addr_send), sizeof(this->m_addr_send))); @@ -211,11 +213,14 @@ I32 UdpSocket::sendProtocol(const SocketDescriptor& socketDescriptor, const U8* I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) { FW_ASSERT(this->m_addr_recv.sin_family != 0); // Make sure the address was previously setup - FW_ASSERT(socketDescriptor.fd < 0 || data == nullptr || size == 0); + FW_ASSERT(socketDescriptor.fd >= 0); // File descriptor should be valid + FW_ASSERT(data != nullptr); // Data pointer should not be null + FW_ASSERT(size > 0); // Size should be positive // Initialize sender address structure to zero struct sockaddr_in sender_addr; ::memset(&sender_addr, 0, sizeof(sender_addr)); + socklen_t sender_addr_len = sizeof(sender_addr); I32 received = static_cast(::recvfrom(socketDescriptor.fd, data, size, SOCKET_IP_RECV_FLAGS, reinterpret_cast(&sender_addr), &sender_addr_len)); diff --git a/Drv/Ip/UdpSocket.hpp b/Drv/Ip/UdpSocket.hpp index 8ca03f98530..855efbd7b35 100644 --- a/Drv/Ip/UdpSocket.hpp +++ b/Drv/Ip/UdpSocket.hpp @@ -133,8 +133,6 @@ class UdpSocket : public IpSocket { private: struct sockaddr_in m_addr_send; //!< UDP server address for sending struct sockaddr_in m_addr_recv; //!< UDP server address for receiving - U16 m_recv_port; //!< Port to receive on - CHAR m_recv_hostname[SOCKET_MAX_HOSTNAME_SIZE]; //!< Hostname to receive on bool m_recv_configured; //!< True if configureRecv was called }; } // namespace Drv From 006f2f6b2d6d8532c87e49b666b844496aada343 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Thu, 12 Jun 2025 00:44:08 -0700 Subject: [PATCH 04/15] Fixing some documentation and renaming a variable to be more consistent --- Drv/Ip/UdpSocket.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index ea246bf42b5..d4ca07e24ca 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -54,14 +54,12 @@ SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, SocketIpStatus UdpSocket::configureSend(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) { - //Timeout is for the send, so configure send will work with the base class if (hostname == nullptr) { return SOCK_INVALID_IP_ADDRESS; } if (timeout_microseconds >= 1000000) { return SOCK_FAILED_TO_SET_SOCKET_OPTIONS; } - // Call the base class configure method directly return IpSocket::configure(hostname, port, timeout_seconds, timeout_microseconds); } @@ -101,7 +99,6 @@ SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { return SOCK_FAILED_TO_BIND; } - // Use a local copy of the address structure struct sockaddr_in address = this->m_addr_recv; // OS specific settings @@ -121,7 +118,6 @@ SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { // Update m_addr_recv with the actual port assigned (for ephemeral port support) this->m_addr_recv.sin_port = address.sin_port; - // No need to copy the entire structure since we only updated the port return SOCK_SUCCESS; } @@ -185,15 +181,15 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { } // Log message for UDP - char recv_ip[INET_ADDRSTRLEN]; - inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_ip, INET_ADDRSTRLEN); + char recv_addr[INET_ADDRSTRLEN]; + inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_addr, INET_ADDRSTRLEN); if ((port == 0) && (recv_port > 0)) { - Fw::Logger::log("Setup to only receive udp at %s:%hu\n", recv_ip, recv_port); + Fw::Logger::log("Setup to only receive udp at %s:%hu\n", recv_addr, recv_port); } else if ((port > 0) && (recv_port == 0)) { Fw::Logger::log("Setup to only send udp at %s:%hu\n", m_hostname, port); } else if ((port > 0) && (recv_port > 0)) { - Fw::Logger::log("Setup to receive udp at %s:%hu and send to %s:%hu\n", recv_ip, recv_port, m_hostname, port); + Fw::Logger::log("Setup to receive udp at %s:%hu and send to %s:%hu\n", recv_addr, recv_port, m_hostname, port); } FW_ASSERT(status == SOCK_SUCCESS, static_cast(status)); From 574dba437ec551d08156165c8b77b117bc32410e Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Thu, 12 Jun 2025 09:38:45 -0700 Subject: [PATCH 05/15] Adding ADDRSTRLEN to spelling expect --- .github/actions/spelling/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 00a65c8ec96..0ae5f1ad76f 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -8,6 +8,7 @@ ACTIVERATEGROUP ACTIVERATEGROUPCFG ACTIVERATEGROUPIMPLTESTER ACTIVETEXTLOGGERIMPL +ADDRSTRLEN adminlist ALLEXTERNALS alphanums From 484c5b95015333ea686f016758374e0e36083e91 Mon Sep 17 00:00:00 2001 From: vincewoo Date: Fri, 13 Jun 2025 20:42:22 -0700 Subject: [PATCH 06/15] Incorporating PR fixes and adding a new UT for zero length datagram --- Drv/Ip/UdpSocket.cpp | 49 ++++++++++++++----------------------- Drv/Ip/UdpSocket.hpp | 1 - Drv/Ip/test/ut/TestUdp.cpp | 50 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 32 deletions(-) diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index d4ca07e24ca..15a36b1c177 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -41,8 +41,8 @@ namespace Drv { UdpSocket::UdpSocket() : IpSocket(), m_recv_configured(false) { - ::memset(&m_addr_send, 0, sizeof(m_addr_send)); - ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); + (void)::memset(&m_addr_send, 0, sizeof(m_addr_send)); + (void)::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); } UdpSocket::~UdpSocket() = default; @@ -54,29 +54,19 @@ SocketIpStatus UdpSocket::configure(const char* const hostname, const U16 port, SocketIpStatus UdpSocket::configureSend(const char* const hostname, const U16 port, const U32 timeout_seconds, const U32 timeout_microseconds) { - if (hostname == nullptr) { - return SOCK_INVALID_IP_ADDRESS; - } - if (timeout_microseconds >= 1000000) { - return SOCK_FAILED_TO_SET_SOCKET_OPTIONS; - } + FW_ASSERT(hostname != nullptr); + FW_ASSERT(this->isValidPort(port)); + FW_ASSERT(timeout_microseconds < 1000000); return IpSocket::configure(hostname, port, timeout_seconds, timeout_microseconds); } SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) { - if (hostname == nullptr) { - return SOCK_INVALID_IP_ADDRESS; - } - if (!this->isValidPort(port)) { - return SOCK_INVALID_CALL; - } - // Check hostname length to prevent potential buffer overflow - if (strlen(hostname) >= SOCKET_MAX_HOSTNAME_SIZE) { - return SOCK_INVALID_IP_ADDRESS; - } + FW_ASSERT(hostname != nullptr); + FW_ASSERT(this->isValidPort(port)); + FW_ASSERT(Fw::StringUtils::string_length(hostname, SOCKET_MAX_HOSTNAME_SIZE) < SOCKET_MAX_HOSTNAME_SIZE); // Initialize the receive address structure - ::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); + (void)::memset(&m_addr_recv, 0, sizeof(m_addr_recv)); m_addr_recv.sin_family = AF_INET; m_addr_recv.sin_port = htons(port); @@ -93,12 +83,8 @@ U16 UdpSocket::getRecvPort() { return ntohs(this->m_addr_recv.sin_port); } - SocketIpStatus UdpSocket::bind(const PlatformIntType fd) { - if (fd < 0) { - return SOCK_FAILED_TO_BIND; - } - + FW_ASSERT(fd != -1); struct sockaddr_in address = this->m_addr_recv; // OS specific settings @@ -132,7 +118,7 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { // Initialize address structure to zero before use struct sockaddr_in address; - ::memset(&address, 0, sizeof(address)); + (void)::memset(&address, 0, sizeof(address)); U16 port = this->m_port; U16 recv_port = ntohs(this->m_addr_recv.sin_port); @@ -167,7 +153,7 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { } FW_ASSERT(sizeof(this->m_addr_send) == sizeof(address), static_cast(sizeof(this->m_addr_send)), static_cast(sizeof(address))); - memcpy(&this->m_addr_send, &address, sizeof(this->m_addr_send)); + (void)memcpy(&this->m_addr_send, &address, sizeof(this->m_addr_send)); } // Only bind if configureRecv was called (including ephemeral) @@ -182,7 +168,10 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { // Log message for UDP char recv_addr[INET_ADDRSTRLEN]; - inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_addr, INET_ADDRSTRLEN); + if (inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_addr, INET_ADDRSTRLEN) == nullptr) { + strncpy(recv_addr, "INVALID_ADDR", INET_ADDRSTRLEN); + recv_addr[INET_ADDRSTRLEN - 1] = '\0'; + } if ((port == 0) && (recv_port > 0)) { Fw::Logger::log("Setup to only receive udp at %s:%hu\n", recv_addr, recv_port); @@ -201,7 +190,6 @@ I32 UdpSocket::sendProtocol(const SocketDescriptor& socketDescriptor, const U8* FW_ASSERT(this->m_addr_send.sin_family != 0); // Make sure the address was previously setup FW_ASSERT(socketDescriptor.fd >= 0); // File descriptor should be valid FW_ASSERT(data != nullptr); // Data pointer should not be null - FW_ASSERT(size > 0); // Size should be positive return static_cast(::sendto(socketDescriptor.fd, data, size, SOCKET_IP_SEND_FLAGS, reinterpret_cast(&this->m_addr_send), sizeof(this->m_addr_send))); @@ -211,17 +199,16 @@ I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const FW_ASSERT(this->m_addr_recv.sin_family != 0); // Make sure the address was previously setup FW_ASSERT(socketDescriptor.fd >= 0); // File descriptor should be valid FW_ASSERT(data != nullptr); // Data pointer should not be null - FW_ASSERT(size > 0); // Size should be positive // Initialize sender address structure to zero struct sockaddr_in sender_addr; - ::memset(&sender_addr, 0, sizeof(sender_addr)); + (void)::memset(&sender_addr, 0, sizeof(sender_addr)); socklen_t sender_addr_len = sizeof(sender_addr); I32 received = static_cast(::recvfrom(socketDescriptor.fd, data, size, SOCKET_IP_RECV_FLAGS, reinterpret_cast(&sender_addr), &sender_addr_len)); // If we have not configured a send port, set it to the source of the last received packet - if (received > 0 && this->m_addr_send.sin_port == 0) { + if (received >= 0 && this->m_addr_send.sin_port == 0) { this->m_addr_send = sender_addr; this->m_port = ntohs(sender_addr.sin_port); Fw::Logger::log("Configured send port to %hu as specified by the last received packet.\n", this->m_port); diff --git a/Drv/Ip/UdpSocket.hpp b/Drv/Ip/UdpSocket.hpp index 855efbd7b35..169b8e756fd 100644 --- a/Drv/Ip/UdpSocket.hpp +++ b/Drv/Ip/UdpSocket.hpp @@ -101,7 +101,6 @@ class UdpSocket : public IpSocket { U16 getRecvPort(); protected: - /** * \brief bind the UDP to a port such that it can receive packets at the previously configured port * \param socketDescriptor: socket descriptor used in bind diff --git a/Drv/Ip/test/ut/TestUdp.cpp b/Drv/Ip/test/ut/TestUdp.cpp index 54d9c2d026e..9b8d1d847b4 100644 --- a/Drv/Ip/test/ut/TestUdp.cpp +++ b/Drv/Ip/test/ut/TestUdp.cpp @@ -3,10 +3,19 @@ // #include #include +#include #include + +#ifdef __VXWORKS__ +#include +#include +#include +#else #include #include #include +#endif + #include #include #include @@ -100,6 +109,47 @@ TEST(SingleSide, TestSingleSideSendUdp) { test_with_loop(1, SEND); } +TEST(UdpZeroLength, TestZeroLengthUdpDatagram) { + Drv::UdpSocket sender; + Drv::UdpSocket receiver; + Drv::SocketDescriptor send_fd; + Drv::SocketDescriptor recv_fd; + U16 port = Drv::Test::get_free_port(true); + ASSERT_NE(0, port); + + // Configure receiver and sender + ASSERT_EQ(receiver.configureRecv("127.0.0.1", port), Drv::SOCK_SUCCESS); + ASSERT_EQ(receiver.open(recv_fd), Drv::SOCK_SUCCESS); + + // Set a short receive timeout to avoid hanging + Drv::Test::force_recv_timeout(recv_fd.fd, receiver); + ASSERT_EQ(sender.configureSend("127.0.0.1", port, 0, 100), Drv::SOCK_SUCCESS); + ASSERT_EQ(sender.open(send_fd), Drv::SOCK_SUCCESS); + + // Send a zero-length datagram + U8 empty_data[1] = {0}; // Buffer is required, but size is 0 + ASSERT_EQ(sender.send(send_fd, empty_data, 0), Drv::SOCK_SUCCESS); + + // Receive the zero-length datagram + U8 recv_buf[1] = {0xFF}; + U32 recv_buf_len = 1; + I32 recv_status = receiver.recv(recv_fd, recv_buf, recv_buf_len); + + // Accept 0 (success), -EAGAIN, or -EWOULDBLOCK (timeout/no data) + bool valid_status = (recv_status == 0 || + recv_status == -EAGAIN || +#ifdef EWOULDBLOCK + recv_status == -EWOULDBLOCK || +#endif + recv_status == -16 // fallback for platforms where EWOULDBLOCK is not defined but -16 is returned + ); + ASSERT_TRUE(valid_status) + << "recv_status=" << recv_status << ", errno=" << errno; + + sender.close(send_fd); + receiver.close(recv_fd); +} + TEST(SingleSide, TestSingleSideMultipleSendUdp) { test_with_loop(100, SEND); } From 70be688113fc61e5002092c20b610b984475cc7a Mon Sep 17 00:00:00 2001 From: vincewoo Date: Fri, 13 Jun 2025 21:04:27 -0700 Subject: [PATCH 07/15] Replacing a strcpy with a Fw::StringUtils::string_copy --- Drv/Ip/UdpSocket.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index 15a36b1c177..c4c0ea4902d 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -169,8 +169,7 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { // Log message for UDP char recv_addr[INET_ADDRSTRLEN]; if (inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_addr, INET_ADDRSTRLEN) == nullptr) { - strncpy(recv_addr, "INVALID_ADDR", INET_ADDRSTRLEN); - recv_addr[INET_ADDRSTRLEN - 1] = '\0'; + (void) Fw::StringUtils::string_copy(recv_addr, "INVALID_ADDR", INET_ADDRSTRLEN); } if ((port == 0) && (recv_port > 0)) { From 89873946d58f17bc85d6ea3cd9a7effd142b2089 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Tue, 17 Jun 2025 16:07:47 -0700 Subject: [PATCH 08/15] Removing incorrect UDP statuses in empty datagram test --- Drv/Ip/test/ut/SocketTestHelper.cpp | 23 ++++++++++++++++++----- Drv/Ip/test/ut/TestUdp.cpp | 15 ++++----------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/Drv/Ip/test/ut/SocketTestHelper.cpp b/Drv/Ip/test/ut/SocketTestHelper.cpp index 0ff8ba97e90..81c5d6bec39 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.cpp +++ b/Drv/Ip/test/ut/SocketTestHelper.cpp @@ -21,7 +21,7 @@ void force_recv_timeout(PlatformIntType fd, Drv::IpSocket& socket) { // Set timeout socket option struct timeval timeout; timeout.tv_sec = 0; - timeout.tv_usec = 50; // 50ms max before test failure + timeout.tv_usec = 100000; // 100ms max before test failure setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, reinterpret_cast(&timeout), sizeof(timeout)); } @@ -65,10 +65,23 @@ void receive_all(Drv::IpSocket& receiver, Drv::SocketDescriptor& receiver_fd, U8 U32 received_size = 0; Drv::SocketIpStatus status; do { - U32 size_in_out = size - received_size; - status = receiver.recv(receiver_fd, buffer + received_size, size_in_out); - ASSERT_TRUE((status == Drv::SOCK_NO_DATA_AVAILABLE || status == Drv::SOCK_SUCCESS)); - received_size += size_in_out; + U32 bytes_to_request = size - received_size; + U32 bytes_actually_received = bytes_to_request; // Will be updated by receiver.recv + status = receiver.recv(receiver_fd, buffer + received_size, bytes_actually_received); + + if (status == Drv::SOCK_SUCCESS) { + received_size += bytes_actually_received; + } else if (status == Drv::SOCK_NO_DATA_AVAILABLE) { + // Socket timeout occurred before all expected data was received. + FAIL() << "Drv::Test::receive_all timed out (SOCK_NO_DATA_AVAILABLE) expecting " << size + << " bytes, but received only " << received_size << " bytes so far."; + return; // Exit to prevent infinite loop and mark test as failed + } else { + // Other unexpected socket error + FAIL() << "Drv::Test::receive_all encountered an unexpected socket error: " << status + << " while expecting " << size << " bytes. Received " << received_size << " bytes so far."; + return; // Exit to prevent infinite loop and mark test as failed + } } while (size > received_size); EXPECT_EQ(received_size, size); } diff --git a/Drv/Ip/test/ut/TestUdp.cpp b/Drv/Ip/test/ut/TestUdp.cpp index 9b8d1d847b4..a5c4de89d48 100644 --- a/Drv/Ip/test/ut/TestUdp.cpp +++ b/Drv/Ip/test/ut/TestUdp.cpp @@ -121,7 +121,7 @@ TEST(UdpZeroLength, TestZeroLengthUdpDatagram) { ASSERT_EQ(receiver.configureRecv("127.0.0.1", port), Drv::SOCK_SUCCESS); ASSERT_EQ(receiver.open(recv_fd), Drv::SOCK_SUCCESS); - // Set a short receive timeout to avoid hanging + // Set a receive timeout to avoid hanging (now 100ms) Drv::Test::force_recv_timeout(recv_fd.fd, receiver); ASSERT_EQ(sender.configureSend("127.0.0.1", port, 0, 100), Drv::SOCK_SUCCESS); ASSERT_EQ(sender.open(send_fd), Drv::SOCK_SUCCESS); @@ -135,16 +135,9 @@ TEST(UdpZeroLength, TestZeroLengthUdpDatagram) { U32 recv_buf_len = 1; I32 recv_status = receiver.recv(recv_fd, recv_buf, recv_buf_len); - // Accept 0 (success), -EAGAIN, or -EWOULDBLOCK (timeout/no data) - bool valid_status = (recv_status == 0 || - recv_status == -EAGAIN || -#ifdef EWOULDBLOCK - recv_status == -EWOULDBLOCK || -#endif - recv_status == -16 // fallback for platforms where EWOULDBLOCK is not defined but -16 is returned - ); - ASSERT_TRUE(valid_status) - << "recv_status=" << recv_status << ", errno=" << errno; + // Expect 0 (success) for a zero-length datagram. + ASSERT_EQ(recv_status, 0) + << "Expected recv_status 0 for zero-length datagram, but got " << recv_status << " with errno=" << errno; sender.close(send_fd); receiver.close(recv_fd); From 401908a2fc16775cdbc896bea7eb3c62f0ca92c1 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Tue, 17 Jun 2025 21:03:32 -0700 Subject: [PATCH 09/15] Fixing bug in implementation of UdpSocket by overriding IpSocket implementation of send and recv to properly handle 0-length datagrams --- Drv/Ip/IpSocket.cpp | 68 ++++++++++++--------- Drv/Ip/IpSocket.hpp | 4 +- Drv/Ip/UdpSocket.cpp | 95 ++++++++++++++++++++++++++--- Drv/Ip/UdpSocket.hpp | 22 ++++++- Drv/Ip/test/ut/SocketTestHelper.cpp | 12 +++- Drv/Ip/test/ut/SocketTestHelper.hpp | 10 ++- Drv/Ip/test/ut/TestUdp.cpp | 33 +++++----- 7 files changed, 182 insertions(+), 62 deletions(-) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index 5f80a495137..ab1fd2b35a4 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -161,40 +161,50 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8 } SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { - I32 size = 0; - // Try to read until we fail to receive data - for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (size <= 0); i++) { - errno = 0; - // Attempt to recv out data - size = this->recvProtocol(socketDescriptor, data, req_read); + I32 bytes_received_or_status; // Stores the return value from recvProtocol - // Nothing to be received - if ((size == -1) && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) { + // Loop primarily for EINTR. Other conditions should lead to an earlier exit. + for (U32 i = 0; i < SOCKET_MAX_ITERATIONS; i++) { + errno = 0; + // Pass the current value of req_read (max buffer size) to recvProtocol. + // recvProtocol returns bytes read or -1 on error. + bytes_received_or_status = this->recvProtocol(socketDescriptor, data, req_read); + + if (bytes_received_or_status > 0) { + // Successfully read data + req_read = static_cast(bytes_received_or_status); + return SOCK_SUCCESS; + } else if (bytes_received_or_status == 0) { + // For TCP (which IpSocket primarily serves as a base for, or when not overridden), + // a return of 0 from ::recv means the peer has performed an orderly shutdown. req_read = 0; - return SOCK_NO_DATA_AVAILABLE; - } - - // Error is EINTR, just try again - if ((size == -1) && (errno == EINTR)) { - continue; - } - // Zero bytes read reset or bad ef means we've disconnected - else if (size == 0 || ((size == -1) && ((errno == ECONNRESET) || (errno == EBADF)))) { - req_read = static_cast(size); return SOCK_DISCONNECTED; - } - // Error returned, and it wasn't an interrupt, nor a disconnect - else if (size == -1) { - req_read = static_cast(size); - return SOCK_READ_ERROR; // Stop recv task on error + } else { // bytes_received_or_status == -1, an error occurred + if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) { + // Non-blocking socket would block, or SO_RCVTIMEO timeout occurred. + req_read = 0; + return SOCK_NO_DATA_AVAILABLE; + } else if (errno == EINTR) { + // Interrupted system call. Retry if not the last iteration. + if (i == (SOCKET_MAX_ITERATIONS - 1)) { + req_read = 0; + return SOCK_INTERRUPTED_TRY_AGAIN; // Max retries for EINTR reached + } + // Loop will continue for EINTR. The 'continue' is implicit here. + } else if ((errno == ECONNRESET) || (errno == EBADF)) { + // Connection reset or bad file descriptor. + req_read = 0; + return SOCK_DISCONNECTED; // Or a more specific error like SOCK_READ_ERROR + } else { + // Other socket read error. + req_read = 0; + return SOCK_READ_ERROR; + } } } - req_read = static_cast(size); - // Prevent interrupted socket being viewed as success - if (size == -1) { - return SOCK_INTERRUPTED_TRY_AGAIN; - } - return SOCK_SUCCESS; + // If the loop completes, it means SOCKET_MAX_ITERATIONS of EINTR occurred. + req_read = 0; + return SOCK_INTERRUPTED_TRY_AGAIN; } } // namespace Drv diff --git a/Drv/Ip/IpSocket.hpp b/Drv/Ip/IpSocket.hpp index 5a3784c8cf5..4357fcb77d5 100644 --- a/Drv/Ip/IpSocket.hpp +++ b/Drv/Ip/IpSocket.hpp @@ -114,7 +114,7 @@ class IpSocket { * \param size: size of data to send * \return status of the send, SOCK_DISCONNECTED to reopen, SOCK_SUCCESS on success, something else on error */ - SocketIpStatus send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size); + virtual SocketIpStatus send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size); /** * \brief receive data from the IP socket from the given buffer * @@ -131,7 +131,7 @@ class IpSocket { * \param size: maximum size of data buffer to fill * \return status of the send, SOCK_DISCONNECTED to reopen, SOCK_SUCCESS on success, something else on error */ - SocketIpStatus recv(const SocketDescriptor& fd, U8* const data, U32& size); + virtual SocketIpStatus recv(const SocketDescriptor& fd, U8* const data, U32& size); /** * \brief closes the socket diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index c4c0ea4902d..2eb80f03378 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -27,16 +27,15 @@ #include #include #include -#elif defined TGT_OS_TYPE_LINUX || TGT_OS_TYPE_DARWIN +#else #include #include #include -#else - #error OS not supported for IP Socket Communications #endif #include #include +#include namespace Drv { @@ -71,8 +70,9 @@ SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) { m_addr_recv.sin_port = htons(port); // Convert hostname to IP address - if (IpSocket::addressToIp4(hostname, &m_addr_recv.sin_addr) != SOCK_SUCCESS) { - return SOCK_INVALID_IP_ADDRESS; + SocketIpStatus status = IpSocket::addressToIp4(hostname, &m_addr_recv.sin_addr); + if (status != SOCK_SUCCESS) { + return status; } this->m_recv_configured = true; @@ -140,14 +140,16 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { #endif // First IP address to socket sin_addr - if ((status = IpSocket::addressToIp4(m_hostname, &(address.sin_addr))) != SOCK_SUCCESS) { + status = IpSocket::addressToIp4(m_hostname, &(address.sin_addr)); + if (status != SOCK_SUCCESS) { Fw::Logger::log("Failed to resolve hostname %s: %d\n", m_hostname, static_cast(status)); ::close(socketFd); return status; }; // Now apply timeouts - if ((status = this->setupTimeouts(socketFd)) != SOCK_SUCCESS) { + status = this->setupTimeouts(socketFd); + if (status != SOCK_SUCCESS) { ::close(socketFd); return status; } @@ -168,7 +170,8 @@ SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) { // Log message for UDP char recv_addr[INET_ADDRSTRLEN]; - if (inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_addr, INET_ADDRSTRLEN) == nullptr) { + const char* recv_addr_str = inet_ntop(AF_INET, &(this->m_addr_recv.sin_addr), recv_addr, INET_ADDRSTRLEN); + if (recv_addr_str == nullptr) { (void) Fw::StringUtils::string_copy(recv_addr, "INVALID_ADDR", INET_ADDRSTRLEN); } @@ -215,4 +218,80 @@ I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const return received; } +SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { + // Special case for zero-length datagrams in UDP + if (size == 0) { + errno = 0; + I32 sent = this->sendProtocol(socketDescriptor, data, 0); + if (sent == -1) { + if (errno == EINTR) { + // For zero-length datagrams, we'll just try once more if interrupted + errno = 0; + sent = this->sendProtocol(socketDescriptor, data, 0); + } + + if (sent == -1) { + if ((errno == EBADF) || (errno == ECONNRESET)) { + return SOCK_DISCONNECTED; + } else { + return SOCK_SEND_ERROR; + } + } + } + // For zero-length datagrams in UDP, success is either 0 or a non-negative value + return SOCK_SUCCESS; + } + + // For non-zero-length data, delegate to the base class implementation + return IpSocket::send(socketDescriptor, data, size); +} + +SocketIpStatus UdpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { + I32 bytes_received_or_status; // Stores the return from recvProtocol, which is byte count or -1 + + // Loop primarily for EINTR. Other conditions should lead to an earlier exit. + for (U32 i = 0; i < SOCKET_MAX_ITERATIONS; i++) { + errno = 0; + // Note: recvProtocol for UdpSocket takes 'size' as const U32, + // so it won't modify 'req_read' directly. We pass req_read. + // The actual number of bytes read will be the return value 'bytes_received_or_status'. + bytes_received_or_status = this->recvProtocol(socketDescriptor, data, req_read); + + if (bytes_received_or_status > 0) { + // Successfully read data + req_read = static_cast(bytes_received_or_status); + return SOCK_SUCCESS; + } else if (bytes_received_or_status == 0) { + // For UDP, a return of 0 from recvfrom means a 0-byte datagram was received. + // This is a success case for UDP, not a disconnection. + req_read = 0; + return SOCK_SUCCESS; + } else { // bytes_received_or_status == -1, an error occurred + if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) { + // Timeout or would block (non-blocking socket) + req_read = 0; // No data read + return SOCK_NO_DATA_AVAILABLE; + } else if (errno == EINTR) { + // Interrupted system call. Retry if not the last iteration. + if (i == (SOCKET_MAX_ITERATIONS - 1)) { // Last attempt + req_read = 0; + return SOCK_INTERRUPTED_TRY_AGAIN; // Max retries for EINTR reached + } + // Loop will continue for EINTR. The 'continue' is implicit here. + } else if ((errno == ECONNRESET) || (errno == EBADF)) { + // Connection reset or bad file descriptor. + req_read = 0; + return SOCK_DISCONNECTED; + } else { + // Other unhandled socket read error. + req_read = 0; + return SOCK_READ_ERROR; + } + } + } + // If the loop completes, it means SOCKET_MAX_ITERATIONS of EINTR occurred. + req_read = 0; + return SOCK_INTERRUPTED_TRY_AGAIN; +} + } // namespace Drv diff --git a/Drv/Ip/UdpSocket.hpp b/Drv/Ip/UdpSocket.hpp index 169b8e756fd..141951828fb 100644 --- a/Drv/Ip/UdpSocket.hpp +++ b/Drv/Ip/UdpSocket.hpp @@ -20,11 +20,9 @@ #ifdef TGT_OS_TYPE_VXWORKS #include #include -#elif defined TGT_OS_TYPE_LINUX || TGT_OS_TYPE_DARWIN +#else #include #include -#else - #error OS not supported for IP Socket Communications #endif namespace Drv { @@ -100,6 +98,24 @@ class UdpSocket : public IpSocket { */ U16 getRecvPort(); + /** + * \brief UDP-specific implementation of send that handles zero-length datagrams correctly. + * \param socketDescriptor: descriptor to send to + * \param data: data pointer to send + * \param size: size of data to send + * \return: status of the send operation + */ + SocketIpStatus send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) override; + + /** + * \brief UDP-specific implementation of recv that handles zero-length datagrams as success. + * \param socketDescriptor: descriptor to recv from + * \param data: data pointer to fill + * \param req_read: size of data buffer on input, size of data received on output + * \return: status of the receive operation + */ + SocketIpStatus recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) override; + protected: /** * \brief bind the UDP to a port such that it can receive packets at the previously configured port diff --git a/Drv/Ip/test/ut/SocketTestHelper.cpp b/Drv/Ip/test/ut/SocketTestHelper.cpp index 81c5d6bec39..aeda78b7ee2 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.cpp +++ b/Drv/Ip/test/ut/SocketTestHelper.cpp @@ -17,11 +17,17 @@ namespace Test { const U32 MAX_DRV_TEST_MESSAGE_SIZE = 1024; -void force_recv_timeout(PlatformIntType fd, Drv::IpSocket& socket) { +void force_recv_timeout(PlatformIntType fd, Drv::IpSocket& socket, const TestTimeouts* custom_timeouts) { // Set timeout socket option struct timeval timeout; - timeout.tv_sec = 0; - timeout.tv_usec = 100000; // 100ms max before test failure + if (custom_timeouts != nullptr) { + timeout.tv_sec = static_cast(custom_timeouts->m_sec); + timeout.tv_usec = static_cast(custom_timeouts->m_usec); + } else { + // Default timeout if no custom one is provided + timeout.tv_sec = 0; + timeout.tv_usec = static_cast(100000); // 100ms default + } setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, reinterpret_cast(&timeout), sizeof(timeout)); } diff --git a/Drv/Ip/test/ut/SocketTestHelper.hpp b/Drv/Ip/test/ut/SocketTestHelper.hpp index 90b3160bce9..e7993d309bb 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.hpp +++ b/Drv/Ip/test/ut/SocketTestHelper.hpp @@ -13,13 +13,21 @@ namespace Drv { namespace Test { static constexpr U16 MAX_ITER = 10; + +// Define the TestTimeouts struct +struct TestTimeouts { + U32 m_sec; + U32 m_usec; +}; + /** * Force a receive timeout on a socket such that it will not hang our testing despite the normal recv behavior of * "block forever" until it gets data. * @param fd: socket file descriptor * @param socket: socket to make timeout + * @param custom_timeouts: optional custom timeout values. If nullptr, a default is used. */ -void force_recv_timeout(PlatformIntType fd, Drv::IpSocket &socket); +void force_recv_timeout(PlatformIntType fd, Drv::IpSocket &socket, const TestTimeouts* custom_timeouts = nullptr); /** * Validate random data from data against truth diff --git a/Drv/Ip/test/ut/TestUdp.cpp b/Drv/Ip/test/ut/TestUdp.cpp index a5c4de89d48..3a56ebe6205 100644 --- a/Drv/Ip/test/ut/TestUdp.cpp +++ b/Drv/Ip/test/ut/TestUdp.cpp @@ -6,16 +6,6 @@ #include #include -#ifdef __VXWORKS__ -#include -#include -#include -#else -#include -#include -#include -#endif - #include #include #include @@ -121,16 +111,21 @@ TEST(UdpZeroLength, TestZeroLengthUdpDatagram) { ASSERT_EQ(receiver.configureRecv("127.0.0.1", port), Drv::SOCK_SUCCESS); ASSERT_EQ(receiver.open(recv_fd), Drv::SOCK_SUCCESS); - // Set a receive timeout to avoid hanging (now 100ms) - Drv::Test::force_recv_timeout(recv_fd.fd, receiver); - ASSERT_EQ(sender.configureSend("127.0.0.1", port, 0, 100), Drv::SOCK_SUCCESS); + // Set a custom, longer receive timeout for this specific test to 500ms + const Drv::Test::TestTimeouts custom_recv_timeout = {0, 500000}; + Drv::Test::force_recv_timeout(recv_fd.fd, receiver, &custom_recv_timeout); + ASSERT_EQ(sender.configureSend("127.0.0.1", port, 1, 0), Drv::SOCK_SUCCESS); // Send timeout set to 1 sec ASSERT_EQ(sender.open(send_fd), Drv::SOCK_SUCCESS); - // Send a zero-length datagram + // Send a zero-length datagram using the F' socket wrapper U8 empty_data[1] = {0}; // Buffer is required, but size is 0 - ASSERT_EQ(sender.send(send_fd, empty_data, 0), Drv::SOCK_SUCCESS); + ASSERT_EQ(sender.send(send_fd, empty_data, 0), Drv::SOCK_SUCCESS) + << "Failed to send zero-length datagram using F' socket wrapper"; + + // Add a small delay to ensure the packet has time to be processed by the OS + usleep(10000); // 10ms delay - // Receive the zero-length datagram + // Receive the zero-length datagram using the F' socket wrapper U8 recv_buf[1] = {0xFF}; U32 recv_buf_len = 1; I32 recv_status = receiver.recv(recv_fd, recv_buf, recv_buf_len); @@ -138,6 +133,12 @@ TEST(UdpZeroLength, TestZeroLengthUdpDatagram) { // Expect 0 (success) for a zero-length datagram. ASSERT_EQ(recv_status, 0) << "Expected recv_status 0 for zero-length datagram, but got " << recv_status << " with errno=" << errno; + + // Check that the received length is reported as 0 + ASSERT_EQ(recv_buf_len, 0) << "Expected received length 0, but got " << recv_buf_len; + + // Check that the received buffer is unchanged meaning no data was received + ASSERT_EQ(recv_buf[0], 0xFF) << "Expected unchanged buffer (0xFF), but got " << recv_buf[0]; sender.close(send_fd); receiver.close(recv_fd); From 319b38e3421aede3fc74f9f2c0a9c5bdead27d45 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Tue, 17 Jun 2025 21:34:22 -0700 Subject: [PATCH 10/15] Incorporating PR fixes. Adding function argument checks for send/recv. --- Drv/Ip/IpSocket.cpp | 8 ++++++++ Drv/Ip/UdpSocket.cpp | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index e674a08401d..890b494f2ee 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -129,6 +129,10 @@ SocketIpStatus IpSocket::open(SocketDescriptor& socketDescriptor) { } SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { + FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + FW_ASSERT((size == 0) || (data != nullptr)); + FW_ASSERT(size <= SOCKET_MAX_SEND_SIZE, static_cast(size)); + U32 total = 0; I32 sent = 0; // Attempt to send out data and retry as necessary @@ -161,6 +165,10 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8 } SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { + FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + FW_ASSERT((req_read == 0) || (data != nullptr)); + FW_ASSERT(req_read <= SOCKET_MAX_RECV_SIZE, static_cast(req_read)); + I32 bytes_received_or_status; // Stores the return value from recvProtocol // Loop primarily for EINTR. Other conditions should lead to an earlier exit. diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index 393e7addeea..a2d1bdf171d 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -219,6 +219,10 @@ I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const } SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { + FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + FW_ASSERT((size == 0) || (data != nullptr)); + FW_ASSERT(size <= SOCKET_MAX_SEND_SIZE, static_cast(size)); + // Special case for zero-length datagrams in UDP if (size == 0) { errno = 0; @@ -247,6 +251,10 @@ SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U } SocketIpStatus UdpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { + FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + FW_ASSERT((req_read == 0) || (data != nullptr)); + FW_ASSERT(req_read <= SOCKET_MAX_RECV_SIZE, static_cast(req_read)); + I32 bytes_received_or_status; // Stores the return from recvProtocol, which is byte count or -1 // Loop primarily for EINTR. Other conditions should lead to an earlier exit. From 9ac24cd795711893a5bb3258b9eba394672cda16 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Tue, 17 Jun 2025 21:52:17 -0700 Subject: [PATCH 11/15] Fixing build errors --- Drv/Ip/IpSocket.cpp | 5 +---- Drv/Ip/UdpSocket.cpp | 7 +++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index 890b494f2ee..406d132d773 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -129,9 +129,7 @@ SocketIpStatus IpSocket::open(SocketDescriptor& socketDescriptor) { } SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { - FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); FW_ASSERT((size == 0) || (data != nullptr)); - FW_ASSERT(size <= SOCKET_MAX_SEND_SIZE, static_cast(size)); U32 total = 0; I32 sent = 0; @@ -165,9 +163,8 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8 } SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { - FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + // Note: socketDescriptor.fd can be -1 in some test cases FW_ASSERT((req_read == 0) || (data != nullptr)); - FW_ASSERT(req_read <= SOCKET_MAX_RECV_SIZE, static_cast(req_read)); I32 bytes_received_or_status; // Stores the return value from recvProtocol diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index a2d1bdf171d..24fb0a62456 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -219,9 +219,9 @@ I32 UdpSocket::recvProtocol(const SocketDescriptor& socketDescriptor, U8* const } SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { - FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + // Note: socketDescriptor.fd can be -1 in some test cases FW_ASSERT((size == 0) || (data != nullptr)); - FW_ASSERT(size <= SOCKET_MAX_SEND_SIZE, static_cast(size)); + // Special case for zero-length datagrams in UDP if (size == 0) { @@ -251,9 +251,8 @@ SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U } SocketIpStatus UdpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { - FW_ASSERT(socketDescriptor != -1, static_cast(socketDescriptor)); + // Note: socketDescriptor.fd can be -1 in some test cases FW_ASSERT((req_read == 0) || (data != nullptr)); - FW_ASSERT(req_read <= SOCKET_MAX_RECV_SIZE, static_cast(req_read)); I32 bytes_received_or_status; // Stores the return from recvProtocol, which is byte count or -1 From 5a2e11debde6ab4c6ee7184387cce2f002183285 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Wed, 18 Jun 2025 17:22:11 -0700 Subject: [PATCH 12/15] Incorporating PR comments --- Drv/Ip/IpSocket.cpp | 14 +++++--- Drv/Ip/IpSocket.hpp | 13 ++++++- Drv/Ip/UdpSocket.cpp | 54 +++-------------------------- Drv/Ip/UdpSocket.hpp | 19 +++++----- Drv/Ip/test/ut/SocketTestHelper.cpp | 33 ++++-------------- Drv/Ip/test/ut/SocketTestHelper.hpp | 10 +----- Drv/Ip/test/ut/TestUdp.cpp | 7 ++-- 7 files changed, 46 insertions(+), 104 deletions(-) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index 406d132d773..8255ff2b7c4 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -129,6 +129,7 @@ SocketIpStatus IpSocket::open(SocketDescriptor& socketDescriptor) { } SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { + FW_ASSERT(socketDescriptor.fd != -1, static_cast(socketDescriptor.fd)); FW_ASSERT((size == 0) || (data != nullptr)); U32 total = 0; @@ -164,7 +165,7 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8 SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { // Note: socketDescriptor.fd can be -1 in some test cases - FW_ASSERT((req_read == 0) || (data != nullptr)); + FW_ASSERT(data != nullptr); I32 bytes_received_or_status; // Stores the return value from recvProtocol @@ -180,10 +181,9 @@ SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data req_read = static_cast(bytes_received_or_status); return SOCK_SUCCESS; } else if (bytes_received_or_status == 0) { - // For TCP (which IpSocket primarily serves as a base for, or when not overridden), - // a return of 0 from ::recv means the peer has performed an orderly shutdown. + // Handle zero return based on protocol-specific behavior req_read = 0; - return SOCK_DISCONNECTED; + return this->handleZeroReturn(); } else { // bytes_received_or_status == -1, an error occurred if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) { // Non-blocking socket would block, or SO_RCVTIMEO timeout occurred. @@ -212,4 +212,10 @@ SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data return SOCK_INTERRUPTED_TRY_AGAIN; } +SocketIpStatus IpSocket::handleZeroReturn() { + // For TCP (which IpSocket primarily serves as a base for, or when not overridden), + // a return of 0 from ::recv means the peer has performed an orderly shutdown. + return SOCK_DISCONNECTED; +} + } // namespace Drv diff --git a/Drv/Ip/IpSocket.hpp b/Drv/Ip/IpSocket.hpp index 94bc4ac20b8..e164b2c50ce 100644 --- a/Drv/Ip/IpSocket.hpp +++ b/Drv/Ip/IpSocket.hpp @@ -200,13 +200,24 @@ class IpSocket { /** * \brief Protocol specific implementation of recv. Called directly with error handling from recv. - * \param socket: socket descriptor to recv from + * \param socketDescriptor: socket descriptor to recv from * \param data: data pointer to fill * \param size: size of data buffer * \return: size of data received, or -1 on error. */ virtual I32 recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) = 0; + /** + * \brief Handle zero return from recvProtocol + * + * This method is called when recvProtocol returns 0. The default implementation + * treats this as a disconnection (appropriate for TCP). Subclasses can override + * this to provide different behavior. + * + * @return SocketIpStatus Status to return from recv + */ + virtual SocketIpStatus handleZeroReturn(); + U32 m_timeoutSeconds; U32 m_timeoutMicroseconds; U16 m_port; //!< IP address port used diff --git a/Drv/Ip/UdpSocket.cpp b/Drv/Ip/UdpSocket.cpp index 24fb0a62456..5f9a5c537d4 100644 --- a/Drv/Ip/UdpSocket.cpp +++ b/Drv/Ip/UdpSocket.cpp @@ -222,7 +222,6 @@ SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U // Note: socketDescriptor.fd can be -1 in some test cases FW_ASSERT((size == 0) || (data != nullptr)); - // Special case for zero-length datagrams in UDP if (size == 0) { errno = 0; @@ -250,55 +249,10 @@ SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U return IpSocket::send(socketDescriptor, data, size); } -SocketIpStatus UdpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { - // Note: socketDescriptor.fd can be -1 in some test cases - FW_ASSERT((req_read == 0) || (data != nullptr)); - - I32 bytes_received_or_status; // Stores the return from recvProtocol, which is byte count or -1 - - // Loop primarily for EINTR. Other conditions should lead to an earlier exit. - for (U32 i = 0; i < SOCKET_MAX_ITERATIONS; i++) { - errno = 0; - // Note: recvProtocol for UdpSocket takes 'size' as const U32, - // so it won't modify 'req_read' directly. We pass req_read. - // The actual number of bytes read will be the return value 'bytes_received_or_status'. - bytes_received_or_status = this->recvProtocol(socketDescriptor, data, req_read); - - if (bytes_received_or_status > 0) { - // Successfully read data - req_read = static_cast(bytes_received_or_status); - return SOCK_SUCCESS; - } else if (bytes_received_or_status == 0) { - // For UDP, a return of 0 from recvfrom means a 0-byte datagram was received. - // This is a success case for UDP, not a disconnection. - req_read = 0; - return SOCK_SUCCESS; - } else { // bytes_received_or_status == -1, an error occurred - if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) { - // Timeout or would block (non-blocking socket) - req_read = 0; // No data read - return SOCK_NO_DATA_AVAILABLE; - } else if (errno == EINTR) { - // Interrupted system call. Retry if not the last iteration. - if (i == (SOCKET_MAX_ITERATIONS - 1)) { // Last attempt - req_read = 0; - return SOCK_INTERRUPTED_TRY_AGAIN; // Max retries for EINTR reached - } - // Loop will continue for EINTR. The 'continue' is implicit here. - } else if ((errno == ECONNRESET) || (errno == EBADF)) { - // Connection reset or bad file descriptor. - req_read = 0; - return SOCK_DISCONNECTED; - } else { - // Other unhandled socket read error. - req_read = 0; - return SOCK_READ_ERROR; - } - } - } - // If the loop completes, it means SOCKET_MAX_ITERATIONS of EINTR occurred. - req_read = 0; - return SOCK_INTERRUPTED_TRY_AGAIN; +SocketIpStatus UdpSocket::handleZeroReturn() { + // For UDP, a return of 0 from recvfrom means a 0-byte datagram was received. + // This is a success case for UDP, not a disconnection. + return SOCK_SUCCESS; } } // namespace Drv diff --git a/Drv/Ip/UdpSocket.hpp b/Drv/Ip/UdpSocket.hpp index 19c700bcd3c..01031964bf5 100644 --- a/Drv/Ip/UdpSocket.hpp +++ b/Drv/Ip/UdpSocket.hpp @@ -107,15 +107,6 @@ class UdpSocket : public IpSocket { */ SocketIpStatus send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) override; - /** - * \brief UDP-specific implementation of recv that handles zero-length datagrams as success. - * \param socketDescriptor: descriptor to recv from - * \param data: data pointer to fill - * \param req_read: size of data buffer on input, size of data received on output - * \return: status of the receive operation - */ - SocketIpStatus recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) override; - protected: /** * \brief bind the UDP to a port such that it can receive packets at the previously configured port @@ -145,6 +136,16 @@ class UdpSocket : public IpSocket { * \return: size of data received, or -1 on error. */ I32 recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) override; + /** + * \brief Handle zero return from recvProtocol for UDP + * + * For UDP, a return of 0 from recvfrom means a 0-byte datagram was received, + * which is a success case, not a disconnection. + * + * @return SocketIpStatus Status to return from recv + */ + SocketIpStatus handleZeroReturn() override; + private: struct sockaddr_in m_addr_send; //!< UDP server address for sending struct sockaddr_in m_addr_recv; //!< UDP server address for receiving diff --git a/Drv/Ip/test/ut/SocketTestHelper.cpp b/Drv/Ip/test/ut/SocketTestHelper.cpp index 6be6271a07e..e361429286f 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.cpp +++ b/Drv/Ip/test/ut/SocketTestHelper.cpp @@ -17,17 +17,11 @@ namespace Test { const U32 MAX_DRV_TEST_MESSAGE_SIZE = 1024; -void force_recv_timeout(int fd, Drv::IpSocket& socket, const TestTimeouts* custom_timeouts) { +void force_recv_timeout(int fd, Drv::IpSocket& socket) { // Set timeout socket option struct timeval timeout; - if (custom_timeouts != nullptr) { - timeout.tv_sec = static_cast(custom_timeouts->m_sec); - timeout.tv_usec = static_cast(custom_timeouts->m_usec); - } else { - // Default timeout if no custom one is provided - timeout.tv_sec = 0; - timeout.tv_usec = static_cast(100000); // 100ms default - } + timeout.tv_sec = 0; + timeout.tv_usec = 50; // 50ms max before test failure setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, reinterpret_cast(&timeout), sizeof(timeout)); } @@ -71,23 +65,10 @@ void receive_all(Drv::IpSocket& receiver, Drv::SocketDescriptor& receiver_fd, U8 U32 received_size = 0; Drv::SocketIpStatus status; do { - U32 bytes_to_request = size - received_size; - U32 bytes_actually_received = bytes_to_request; // Will be updated by receiver.recv - status = receiver.recv(receiver_fd, buffer + received_size, bytes_actually_received); - - if (status == Drv::SOCK_SUCCESS) { - received_size += bytes_actually_received; - } else if (status == Drv::SOCK_NO_DATA_AVAILABLE) { - // Socket timeout occurred before all expected data was received. - FAIL() << "Drv::Test::receive_all timed out (SOCK_NO_DATA_AVAILABLE) expecting " << size - << " bytes, but received only " << received_size << " bytes so far."; - return; // Exit to prevent infinite loop and mark test as failed - } else { - // Other unexpected socket error - FAIL() << "Drv::Test::receive_all encountered an unexpected socket error: " << status - << " while expecting " << size << " bytes. Received " << received_size << " bytes so far."; - return; // Exit to prevent infinite loop and mark test as failed - } + U32 size_in_out = size - received_size; + status = receiver.recv(receiver_fd, buffer + received_size, size_in_out); + ASSERT_TRUE((status == Drv::SOCK_NO_DATA_AVAILABLE || status == Drv::SOCK_SUCCESS)); + received_size += size_in_out; } while (size > received_size); EXPECT_EQ(received_size, size); } diff --git a/Drv/Ip/test/ut/SocketTestHelper.hpp b/Drv/Ip/test/ut/SocketTestHelper.hpp index 43dfce7cd14..5828598b4d2 100644 --- a/Drv/Ip/test/ut/SocketTestHelper.hpp +++ b/Drv/Ip/test/ut/SocketTestHelper.hpp @@ -13,21 +13,13 @@ namespace Drv { namespace Test { static constexpr U16 MAX_ITER = 10; - -// Define the TestTimeouts struct -struct TestTimeouts { - U32 m_sec; - U32 m_usec; -}; - /** * Force a receive timeout on a socket such that it will not hang our testing despite the normal recv behavior of * "block forever" until it gets data. * @param fd: socket file descriptor * @param socket: socket to make timeout - * @param custom_timeouts: optional custom timeout values. If nullptr, a default is used. */ -void force_recv_timeout(int fd, Drv::IpSocket &socket, const TestTimeouts* custom_timeouts = nullptr); +void force_recv_timeout(int fd, Drv::IpSocket &socket); /** * Validate random data from data against truth diff --git a/Drv/Ip/test/ut/TestUdp.cpp b/Drv/Ip/test/ut/TestUdp.cpp index 3a56ebe6205..585361ac354 100644 --- a/Drv/Ip/test/ut/TestUdp.cpp +++ b/Drv/Ip/test/ut/TestUdp.cpp @@ -110,11 +110,8 @@ TEST(UdpZeroLength, TestZeroLengthUdpDatagram) { // Configure receiver and sender ASSERT_EQ(receiver.configureRecv("127.0.0.1", port), Drv::SOCK_SUCCESS); ASSERT_EQ(receiver.open(recv_fd), Drv::SOCK_SUCCESS); - - // Set a custom, longer receive timeout for this specific test to 500ms - const Drv::Test::TestTimeouts custom_recv_timeout = {0, 500000}; - Drv::Test::force_recv_timeout(recv_fd.fd, receiver, &custom_recv_timeout); - ASSERT_EQ(sender.configureSend("127.0.0.1", port, 1, 0), Drv::SOCK_SUCCESS); // Send timeout set to 1 sec + + ASSERT_EQ(sender.configureSend("127.0.0.1", port, 1, 0), Drv::SOCK_SUCCESS); ASSERT_EQ(sender.open(send_fd), Drv::SOCK_SUCCESS); // Send a zero-length datagram using the F' socket wrapper From 9a2969985e17dad27c783e70baa4124b8c8b0884 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Wed, 18 Jun 2025 17:24:14 -0700 Subject: [PATCH 13/15] Forgot to add change for IpSocket --- Drv/Ip/IpSocket.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index 8255ff2b7c4..937e1a3a1f1 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -189,13 +189,6 @@ SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data // Non-blocking socket would block, or SO_RCVTIMEO timeout occurred. req_read = 0; return SOCK_NO_DATA_AVAILABLE; - } else if (errno == EINTR) { - // Interrupted system call. Retry if not the last iteration. - if (i == (SOCKET_MAX_ITERATIONS - 1)) { - req_read = 0; - return SOCK_INTERRUPTED_TRY_AGAIN; // Max retries for EINTR reached - } - // Loop will continue for EINTR. The 'continue' is implicit here. } else if ((errno == ECONNRESET) || (errno == EBADF)) { // Connection reset or bad file descriptor. req_read = 0; From 7bc3ab221c0afc24c64a98ef40745b9153c99b0f Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Wed, 18 Jun 2025 17:42:30 -0700 Subject: [PATCH 14/15] Fixing asserts and removing virtual from recv that we shouldn't have to override. --- Drv/Ip/IpSocket.cpp | 7 +++++-- Drv/Ip/IpSocket.hpp | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index 937e1a3a1f1..907b2a96fb8 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -130,7 +130,8 @@ SocketIpStatus IpSocket::open(SocketDescriptor& socketDescriptor) { SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) { FW_ASSERT(socketDescriptor.fd != -1, static_cast(socketDescriptor.fd)); - FW_ASSERT((size == 0) || (data != nullptr)); + FW_ASSERT(data != nullptr); + FW_ASSERT(size > 0); U32 total = 0; I32 sent = 0; @@ -164,8 +165,10 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8 } SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { - // Note: socketDescriptor.fd can be -1 in some test cases + //TODO: Uncomment FW_ASSERT for socketDescriptor.fd once we fix TcpClientTester to not pass in unconfigured socketDescriptor + // FW_ASSERT(socketDescriptor.fd != -1, static_cast(socketDescriptor.fd)); FW_ASSERT(data != nullptr); + I32 bytes_received_or_status; // Stores the return value from recvProtocol diff --git a/Drv/Ip/IpSocket.hpp b/Drv/Ip/IpSocket.hpp index e164b2c50ce..b3810f22c94 100644 --- a/Drv/Ip/IpSocket.hpp +++ b/Drv/Ip/IpSocket.hpp @@ -131,7 +131,7 @@ class IpSocket { * \param size: maximum size of data buffer to fill * \return status of the send, SOCK_DISCONNECTED to reopen, SOCK_SUCCESS on success, something else on error */ - virtual SocketIpStatus recv(const SocketDescriptor& fd, U8* const data, U32& size); + SocketIpStatus recv(const SocketDescriptor& fd, U8* const data, U32& size); /** * \brief closes the socket From eb73a9d54c9c37c950e1fbd9e2e6bd0206c60d97 Mon Sep 17 00:00:00 2001 From: Vince Woo Date: Wed, 18 Jun 2025 17:46:23 -0700 Subject: [PATCH 15/15] Spelling --- Drv/Ip/IpSocket.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp index 907b2a96fb8..a9107d27298 100644 --- a/Drv/Ip/IpSocket.cpp +++ b/Drv/Ip/IpSocket.cpp @@ -165,7 +165,7 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8 } SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) { - //TODO: Uncomment FW_ASSERT for socketDescriptor.fd once we fix TcpClientTester to not pass in unconfigured socketDescriptor + //TODO: Uncomment FW_ASSERT for socketDescriptor.fd once we fix TcpClientTester to not pass in uninitialized socketDescriptor // FW_ASSERT(socketDescriptor.fd != -1, static_cast(socketDescriptor.fd)); FW_ASSERT(data != nullptr);