Refactor Drv::Ip::UdpSocket to clean up state members and remove dependency on dynamic memory

Drv/Ip/UdpSocket.cpp

@@ -40,24 +40,12 @@
 
 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_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;
-}
+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
@@ -66,42 +54,57 @@
 
 
 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);
+    if (hostname == nullptr) {
+        return SOCK_INVALID_IP_ADDRESS;
+    }
+    if (timeout_microseconds >= 1000000) {
+        return SOCK_FAILED_TO_SET_SOCKET_OPTIONS;
+    }
+    return IpSocket::configure(hostname, port, timeout_seconds, timeout_microseconds);
 }
 
 SocketIpStatus UdpSocket::configureRecv(const char* hostname, const U16 port) {
-    FW_ASSERT(this->isValidPort(port));
-    FW_ASSERT(hostname != nullptr);
-    this->m_recv_port = port;
-    (void) Fw::StringUtils::string_copy(this->m_recv_hostname, hostname, static_cast<FwSizeType>(SOCKET_MAX_HOSTNAME_SIZE));
+    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;
+    }
+
+    // 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() {
-    U16 port = this->m_recv_port;
-    return port;
+    return ntohs(this->m_addr_recv.sin_port);
 }
 
 
 SocketIpStatus UdpSocket::bind(const PlatformIntType fd) {
-    struct sockaddr_in address;
-    FW_ASSERT(fd != -1);
-
-    // Set up the address port and name
-    address.sin_family = AF_INET;
-    address.sin_port = htons(this->m_recv_port);
+    if (fd < 0) {
+        return SOCK_FAILED_TO_BIND;
+    }
+
+    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<U8>(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<struct sockaddr*>(&address), sizeof(address)) < 0) {
         return SOCK_FAILED_TO_BIND;
@@ -112,22 +115,27 @@
         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_state->m_addr_recv) == sizeof(address), static_cast<FwAssertArgType>(sizeof(this->m_state->m_addr_recv)), static_cast<FwAssertArgType>(sizeof(address)));
-    memcpy(&this->m_state->m_addr_recv, &address, sizeof(this->m_state->m_addr_recv));
 
     return SOCK_SUCCESS;
 }
 
 SocketIpStatus UdpSocket::openProtocol(SocketDescriptor& socketDescriptor) {
+    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;
+    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) {
@@ -147,18 +155,19 @@
 
         // 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<I32>(status));
             ::close(socketFd);
             return status;
         };
 
         // Now apply timeouts
-        if ((status = IpSocket::setupTimeouts(socketFd)) != SOCK_SUCCESS) {
+        if ((status = this->setupTimeouts(socketFd)) != SOCK_SUCCESS) {
             ::close(socketFd);
             return status;
         }
-        FW_ASSERT(sizeof(this->m_state->m_addr_send) == sizeof(address), static_cast<FwAssertArgType>(sizeof(this->m_state->m_addr_send)),
+        FW_ASSERT(sizeof(this->m_addr_send) == sizeof(address), static_cast<FwAssertArgType>(sizeof(this->m_addr_send)),
                   static_cast<FwAssertArgType>(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)
@@ -172,12 +181,15 @@
     }
 
     // Log message for UDP
+    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", m_recv_hostname, 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", 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_addr, recv_port, m_hostname, port);
     }
 
     FW_ASSERT(status == SOCK_SUCCESS, static_cast<FwAssertArgType>(status));
@@ -186,21 +198,31 @@
 }
 
 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
+    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<I32>(::sendto(socketDescriptor.fd, data, size, SOCKET_IP_SEND_FLAGS,
-                    reinterpret_cast<struct sockaddr *>(&this->m_state->m_addr_send), sizeof(this->m_state->m_addr_send)));
+                    reinterpret_cast<struct sockaddr *>(&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
+    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<I32>(::recvfrom(socketDescriptor.fd, data, size, SOCKET_IP_RECV_FLAGS,
                                               reinterpret_cast<struct sockaddr*>(&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);
     }

Drv/Ip/UdpSocket.hpp

@@ -16,9 +16,18 @@
 #include <Drv/Ip/IpSocket.hpp>
 #include <config/IpCfg.hpp>
 
-namespace Drv {
+// Include system headers for sockaddr_in
+#ifdef TGT_OS_TYPE_VXWORKS
+    #include <socket.h>
+    #include <inetLib.h>
+#elif defined TGT_OS_TYPE_LINUX || TGT_OS_TYPE_DARWIN
+    #include <sys/socket.h>
+    #include <arpa/inet.h>
+#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,9 +131,8 @@ class UdpSocket : public IpSocket {
      */
     I32 recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) override;
   private:
-    SocketState* m_state; //!< State storage
-    U16 m_recv_port;  //!< Port to receive on
-    CHAR m_recv_hostname[SOCKET_MAX_HOSTNAME_SIZE]; //!< Hostname to receive on
+    struct sockaddr_in m_addr_send;  //!< UDP server address for sending
+    struct sockaddr_in m_addr_recv;  //!< UDP server address for receiving
     bool m_recv_configured; //!< True if configureRecv was called
 };
 }  // namespace Drv