[PATCH v4] socket: Never use negative buf->len in link_socket_read*

We really don't want to use negative buf lengths in
our code. So remove some places where we did so before.

Change-Id: Iabe1992b2608661b862a81363574e957d18395f0
Signed-off-by: Frank Lichtenheld <[email protected]>
Acked-by: Arne Schwabe <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1554
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1554
This mail reflects revision 4 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <[email protected]>

        
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index d24f534..30a117c 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -929,14 +929,12 @@
      * Set up for recvfrom call to read datagram
      * sent to our TCP/UDP port.
      */
-    int status;
-
     /*ASSERT (!c->c2.to_tun.len);*/
 
     c->c2.buf = c->c2.buffers->read_link_buf;
     ASSERT(buf_init(&c->c2.buf, c->c2.frame.buf.headroom));
 
-    status = link_socket_read(sock, &c->c2.buf, &c->c2.from);
+    ssize_t status = link_socket_read(sock, &c->c2.buf, &c->c2.from);
 
     if (socket_connection_reset(sock, status))
     {
@@ -955,14 +953,14 @@
             if (event_timeout_defined(&c->c2.explicit_exit_notification_interval))
             {
                 msg(D_STREAM_ERRORS,
-                    "Connection reset during exit notification period, ignoring [%d]", status);
+                    "Connection reset during exit notification period, ignoring [%zd]", status);
                 management_sleep(1);
             }
             else
             {
                 register_signal(c->sig, SIGUSR1,
                                 "connection-reset"); /* SOFT-SIGUSR1 -- TCP connection reset */
-                msg(D_STREAM_ERRORS, "Connection reset, restarting [%d]", status);
+                msg(D_STREAM_ERRORS, "Connection reset, restarting [%zd]", status);
             }
         }
         return;
@@ -1026,7 +1024,7 @@
      * Good, non-zero length packet received.
      * Commence multi-stage processing of packet,
      * such as authenticate, decrypt, decompress.
-     * If any stage fails, it sets buf.len to 0 or -1,
+     * If any stage fails, it sets buf.len to 0,
      * telling downstream stages to ignore the packet.
      */
     if (c->c2.buf.len > 0)
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index df2cc9e..7a0d9ca 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1130,7 +1130,7 @@
 
 static void stream_buf_close(struct stream_buf *sb);
 
-static bool stream_buf_added(struct stream_buf *sb, int length_added);
+static bool stream_buf_added(struct stream_buf *sb, ssize_t length_added);
 
 /* For stream protocols, allocate a buffer to build up packet.
  * Called after frame has been finalized. */
@@ -1799,6 +1799,10 @@
     }
 }
 
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+
 void
 link_socket_close(struct link_socket *sock)
 {
@@ -2166,12 +2170,13 @@
  * @return true if \c sb->buf contains fully reassembled packet
  */
 static bool
-stream_buf_added(struct stream_buf *sb, int length_added)
+stream_buf_added(struct stream_buf *sb, ssize_t length_added)
 {
-    dmsg(D_STREAM_DEBUG, "STREAM: ADD length_added=%d", length_added);
+    dmsg(D_STREAM_DEBUG, "STREAM: ADD length_added=%zd", length_added);
     if (length_added > 0)
     {
-        sb->buf.len += length_added;
+        ASSERT(sb->buf.len + length_added <= INT_MAX);
+        sb->buf.len += (int)length_added;
     }
 
     /* if length unknown, see if we can get the length prefix from
@@ -2273,10 +2278,10 @@
  * Socket Read Routines
  */
 
-int
+ssize_t
 link_socket_read_tcp(struct link_socket *sock, struct buffer *buf)
 {
-    int len = 0;
+    ssize_t len = 0;
 
     if (!sock->stream_buf.residual_fully_formed)
     {
@@ -2306,7 +2311,8 @@
         }
         if (len <= 0)
         {
-            return buf->len = len;
+            buf->len = 0;
+            return len;
         }
     }
 
@@ -2338,14 +2344,14 @@
     max_int(CMSG_SPACE(sizeof(struct in6_pktinfo)), CMSG_SPACE(sizeof(struct in_addr)))
 #endif
 
-static socklen_t
+static ssize_t
 link_socket_read_udp_posix_recvmsg(struct link_socket *sock, struct buffer *buf,
-                                   struct link_socket_actual *from)
+                                   struct link_socket_actual *from, socklen_t *fromlen)
 {
     struct iovec iov;
     uint8_t pktinfo_buf[PKTINFO_BUF_SIZE];
     struct msghdr mesg = { 0 };
-    socklen_t fromlen = sizeof(from->dest.addr);
+    *fromlen = sizeof(from->dest.addr);
 
     ASSERT(sock->sd >= 0); /* can't happen */
 
@@ -2354,62 +2360,67 @@
     mesg.msg_iov = &iov;
     mesg.msg_iovlen = 1;
     mesg.msg_name = &from->dest.addr;
-    mesg.msg_namelen = fromlen;
+    mesg.msg_namelen = *fromlen;
     mesg.msg_control = pktinfo_buf;
-    mesg.msg_controllen = sizeof pktinfo_buf;
-    buf->len = recvmsg(sock->sd, &mesg, 0);
-    if (buf->len >= 0)
+    mesg.msg_controllen = (socklen_t)sizeof(pktinfo_buf);
+    ssize_t len = recvmsg(sock->sd, &mesg, 0);
+    if (len < 0)
     {
-        struct cmsghdr *cmsg;
-        fromlen = mesg.msg_namelen;
-        cmsg = CMSG_FIRSTHDR(&mesg);
-        if (cmsg != NULL && CMSG_NXTHDR(&mesg, cmsg) == NULL
+        buf->len = 0;
+        return len;
+    }
+    ASSERT(len <= INT_MAX);
+    buf->len = (int)len;
+    struct cmsghdr *cmsg;
+    *fromlen = mesg.msg_namelen;
+    cmsg = CMSG_FIRSTHDR(&mesg);
+    if (cmsg != NULL && CMSG_NXTHDR(&mesg, cmsg) == NULL
 #if defined(HAVE_IN_PKTINFO) && defined(HAVE_IPI_SPEC_DST)
-            && cmsg->cmsg_level == SOL_IP && cmsg->cmsg_type == IP_PKTINFO
-            && cmsg->cmsg_len >= CMSG_LEN(sizeof(struct in_pktinfo)))
+        && cmsg->cmsg_level == SOL_IP && cmsg->cmsg_type == IP_PKTINFO
+        && cmsg->cmsg_len >= CMSG_LEN(sizeof(struct in_pktinfo)))
 #elif defined(IP_RECVDSTADDR)
-            && cmsg->cmsg_level == IPPROTO_IP && cmsg->cmsg_type == IP_RECVDSTADDR
-            && cmsg->cmsg_len >= CMSG_LEN(sizeof(struct in_addr)))
+        && cmsg->cmsg_level == IPPROTO_IP && cmsg->cmsg_type == IP_RECVDSTADDR
+        && cmsg->cmsg_len >= CMSG_LEN(sizeof(struct in_addr)))
 #else /* if defined(HAVE_IN_PKTINFO) && defined(HAVE_IPI_SPEC_DST) */
 #error ENABLE_IP_PKTINFO is set without IP_PKTINFO xor IP_RECVDSTADDR (fix syshead.h)
 #endif
-        {
+    {
 #if defined(HAVE_IN_PKTINFO) && defined(HAVE_IPI_SPEC_DST)
-            struct in_pktinfo *pkti = (struct in_pktinfo *)CMSG_DATA(cmsg);
-            from->pi.in4.ipi_ifindex =
-                (sock->sockflags & SF_PKTINFO_COPY_IIF) ? pkti->ipi_ifindex : 0;
-            from->pi.in4.ipi_spec_dst = pkti->ipi_spec_dst;
+        struct in_pktinfo *pkti = (struct in_pktinfo *)CMSG_DATA(cmsg);
+        from->pi.in4.ipi_ifindex =
+            (sock->sockflags & SF_PKTINFO_COPY_IIF) ? pkti->ipi_ifindex : 0;
+        from->pi.in4.ipi_spec_dst = pkti->ipi_spec_dst;
 #elif defined(IP_RECVDSTADDR)
-            from->pi.in4 = *(struct in_addr *)CMSG_DATA(cmsg);
+        from->pi.in4 = *(struct in_addr *)CMSG_DATA(cmsg);
 #else /* if defined(HAVE_IN_PKTINFO) && defined(HAVE_IPI_SPEC_DST) */
 #error ENABLE_IP_PKTINFO is set without IP_PKTINFO xor IP_RECVDSTADDR (fix syshead.h)
 #endif
-        }
-        else if (cmsg != NULL && CMSG_NXTHDR(&mesg, cmsg) == NULL
-                 && cmsg->cmsg_level == IPPROTO_IPV6 && cmsg->cmsg_type == IPV6_PKTINFO
-                 && cmsg->cmsg_len >= CMSG_LEN(sizeof(struct in6_pktinfo)))
-        {
-            struct in6_pktinfo *pkti6 = (struct in6_pktinfo *)CMSG_DATA(cmsg);
-            from->pi.in6.ipi6_ifindex =
-                (sock->sockflags & SF_PKTINFO_COPY_IIF) ? pkti6->ipi6_ifindex : 0;
-            from->pi.in6.ipi6_addr = pkti6->ipi6_addr;
-        }
-        else if (cmsg != NULL)
-        {
-            msg(M_WARN,
-                "CMSG received that cannot be parsed (cmsg_level=%d, cmsg_type=%d, cmsg=len=%d)",
-                (int)cmsg->cmsg_level, (int)cmsg->cmsg_type, (int)cmsg->cmsg_len);
-        }
+    }
+    else if (cmsg != NULL && CMSG_NXTHDR(&mesg, cmsg) == NULL
+             && cmsg->cmsg_level == IPPROTO_IPV6 && cmsg->cmsg_type == IPV6_PKTINFO
+             && cmsg->cmsg_len >= CMSG_LEN(sizeof(struct in6_pktinfo)))
+    {
+        struct in6_pktinfo *pkti6 = (struct in6_pktinfo *)CMSG_DATA(cmsg);
+        from->pi.in6.ipi6_ifindex =
+            (sock->sockflags & SF_PKTINFO_COPY_IIF) ? pkti6->ipi6_ifindex : 0;
+        from->pi.in6.ipi6_addr = pkti6->ipi6_addr;
+    }
+    else if (cmsg != NULL)
+    {
+        msg(M_WARN,
+            "CMSG received that cannot be parsed (cmsg_level=%d, cmsg_type=%d, cmsg=len=%d)",
+            (int)cmsg->cmsg_level, (int)cmsg->cmsg_type, (int)cmsg->cmsg_len);
     }
 
-    return fromlen;
+    return buf->len;
 }
 #endif /* if ENABLE_IP_PKTINFO */
 
-int
+ssize_t
 link_socket_read_udp_posix(struct link_socket *sock, struct buffer *buf,
                            struct link_socket_actual *from)
 {
+    ssize_t recvlen;
     socklen_t fromlen = sizeof(from->dest.addr);
     socklen_t expectedlen = af_addr_size(sock->info.af);
     addr_zero_host(&from->dest);
@@ -2420,16 +2431,23 @@
     /* Both PROTO_UDPv4 and PROTO_UDPv6 */
     if (sock->info.proto == PROTO_UDP && sock->sockflags & SF_USE_IP_PKTINFO)
     {
-        fromlen = link_socket_read_udp_posix_recvmsg(sock, buf, from);
+        recvlen = link_socket_read_udp_posix_recvmsg(sock, buf, from, &fromlen);
     }
     else
 #endif
     {
-        buf->len = recvfrom(sock->sd, BPTR(buf), buf_forward_capacity(buf), 0, &from->dest.addr.sa,
-                            &fromlen);
+        recvlen = recvfrom(sock->sd, BPTR(buf), buf_forward_capacity(buf), 0,
+                           &from->dest.addr.sa, &fromlen);
     }
+    if (recvlen < 0)
+    {
+        buf->len = 0;
+        return recvlen;
+    }
+    ASSERT(recvlen <= INT_MAX);
+    buf->len = (int)recvlen;
     /* FIXME: won't do anything when sock->info.af == AF_UNSPEC */
-    if (buf->len >= 0 && expectedlen && fromlen != expectedlen)
+    if (expectedlen && fromlen != expectedlen)
     {
         bad_address_length(fromlen, expectedlen);
     }
@@ -2445,7 +2463,9 @@
 ssize_t
 link_socket_write_tcp(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *to)
 {
-    packet_size_type len = (packet_size_type)BLENZ(buf);
+    const int blen = BLEN(buf);
+    ASSERT(blen >= 0 && blen <= PACKET_SIZE_MAX);
+    packet_size_type len = (packet_size_type)blen;
     dmsg(D_STREAM_DEBUG, "STREAM: WRITE %u offset=%d", len, buf->offset);
     ASSERT(len <= sock->stream_buf.maxlen);
     len = htonps(len);
@@ -2457,10 +2477,6 @@
 #endif
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 #if ENABLE_IP_PKTINFO
 
 ssize_t
diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h
index cd4e8ed..8b49f75 100644
--- a/src/openvpn/socket.h
+++ b/src/openvpn/socket.h
@@ -54,6 +54,7 @@
  */
 
 typedef uint16_t packet_size_type;
+#define PACKET_SIZE_MAX UINT16_MAX
 
 /* convert a packet_size_type from host to network order */
 #define htonps(x) htons(x)
@@ -459,7 +460,7 @@
 #endif /* if PORT_SHARE */
 
 static inline bool
-socket_connection_reset(const struct link_socket *sock, int status)
+socket_connection_reset(const struct link_socket *sock, ssize_t status)
 {
     if (link_socket_connection_oriented(sock))
     {
@@ -586,7 +587,7 @@
  * Socket Read Routines
  */
 
-int link_socket_read_tcp(struct link_socket *sock, struct buffer *buf);
+ssize_t link_socket_read_tcp(struct link_socket *sock, struct buffer *buf);
 
 #ifdef _WIN32
 
@@ -606,20 +607,20 @@
 
 #else  /* ifdef _WIN32 */
 
-int link_socket_read_udp_posix(struct link_socket *sock, struct buffer *buf,
-                               struct link_socket_actual *from);
+ssize_t link_socket_read_udp_posix(struct link_socket *sock, struct buffer *buf,
+                                   struct link_socket_actual *from);
 
 #endif /* ifdef _WIN32 */
 
 /* read a TCP or UDP packet from link */
-static inline int
+static inline ssize_t
 link_socket_read(struct link_socket *sock, struct buffer *buf, struct link_socket_actual *from)
 {
     if (proto_is_udp(sock->info.proto) || socket_is_dco_win(sock))
     /* unified UDPv4 and UDPv6, for DCO-WIN the kernel
      * will strip the length header */
     {
-        int res;
+        ssize_t res;
 
 #ifdef _WIN32
         res = link_socket_read_udp_win32(sock, buf, from);
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index 6a7af85..931e54d 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -535,7 +535,7 @@
 }
 
 static inline bool
-tuntap_is_dco_win_timeout(struct tuntap *tt, int status)
+tuntap_is_dco_win_timeout(struct tuntap *tt, ssize_t status)
 {
     return tuntap_is_dco_win(tt) && (status < 0) && (openvpn_errno() == ERROR_NETNAME_DELETED);
 }
@@ -573,7 +573,7 @@
 }
 
 static inline bool
-tuntap_is_dco_win_timeout(struct tuntap *tt, int status)
+tuntap_is_dco_win_timeout(struct tuntap *tt, ssize_t status)
 {
     return false;
 }