Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1554?usp=email
to review the following change.
Change subject: socket: Never use negative buf->len in link_socket_read*
......................................................................
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]>
---
M src/openvpn/forward.c
M src/openvpn/socket.c
M src/openvpn/socket.h
M src/openvpn/tun.h
4 files changed, 84 insertions(+), 69 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/54/1554/1
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 5004e35..081e79a 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 033444e..b5f000a 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1132,7 +1132,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. */
@@ -1794,6 +1794,10 @@
}
}
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic pop
+#endif
+
void
link_socket_close(struct link_socket *sock)
{
@@ -2130,12 +2134,13 @@
}
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
@@ -2238,10 +2243,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)
{
@@ -2272,7 +2277,8 @@
}
if (len <= 0)
{
- return buf->len = len;
+ buf->len = 0;
+ return len;
}
}
@@ -2304,14 +2310,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 */
@@ -2320,62 +2326,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)
+ 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);
@@ -2386,16 +2397,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);
}
@@ -2411,8 +2429,10 @@
ssize_t
link_socket_write_tcp(struct link_socket *sock, struct buffer *buf, struct
link_socket_actual *to)
{
- packet_size_type len = BLEN(buf);
- dmsg(D_STREAM_DEBUG, "STREAM: WRITE %d offset=%d", (int)len, buf->offset);
+ 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);
ASSERT(buf_write_prepend(buf, &len, sizeof(len)));
@@ -2423,10 +2443,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 3f46dc6..f11634e 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)
@@ -441,7 +442,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))
{
@@ -568,7 +569,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
@@ -588,20 +589,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 34f4929..cf5eb3b 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -537,7 +537,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);
}
@@ -575,7 +575,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;
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1554?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iabe1992b2608661b862a81363574e957d18395f0
Gerrit-Change-Number: 1554
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
