use macro to portably test for socket system call errors

On some combinations of Montavista Linux running on Cavium Octeon
chips, some socket-related system calls erroneously return negative
numbers other than -1 to indicate errors, but inet_drv.c specifically
compares against -1 to test for errors. The result is that beam dumps
core due to the code treating these negative numbers as success
indicators, as counts/offsets of bytes written, etc. thereby
corrupting its own internal data structures.

To fix this, introduce a portability macro to test the result of
socket system calls. The test remains unchanged on Windows but for
other platforms the macro considers all return values that are less
than zero to be errors.

Though POSIX specifies that errors from these system calls are
indicated by a return value of -1, treating all negative return values
as errors is also safe, as described in detail below. In networking
programming, treating all negative return values from system calls as
errors is very common practice -- see the examples in W. Richard
Stevens's popular and highly lauded network programming books, for
example.

For system calls that return 0 to indicate success, treating all
negative numbers as errors is safe because only 0 is specified to
indicate success. These include:

getsockname
getpeername
getsockopt
gethostname
bind
listen
connect
close
shutdown

Likewise, for system calls that return non-negative numbers to
indicate success, treating all negative numbers as errors is also
safe. These functions typically return signed integers of type
ssize_t, and they treat any parameters of type size_t that cannot fit
within the ssize_t return value, such as numbers of bytes to read or
write, as errors (specifically EINVAL). For example, in the "ERRORS"
section of the man page for writev from several varieties of Linux, it
states that EINVAL is returned when the total length of the I/O is
more than can be expressed by the ssize_t return value.

These calls include:

recv
recvfrom
recvmsg
writev
send
sendto
sendmsg

Finaly, the socket() system call is also similar to these in that it
returns a signed type (int) with all non-negative return values
indicating success, so treating all negative return values as errors
is safe.

1 files changed, 47 insertions, 28 deletions

diff --git a/erts/emulator/drivers/common/inet_drv.c b/erts/emulator/drivers/common/inet_drv.c
index e8456cc616..83e4443630 100644
--- a/erts/emulator/drivers/common/inet_drv.c
+++ b/erts/emulator/drivers/common/inet_drv.c
@@ -55,6 +55,21 @@
 
 #include "erl_driver.h"
 
+/* The IS_SOCKET_ERROR macro below is used for portability reasons. While
+   POSIX specifies that errors from socket-related system calls should be
+   indicated with a -1 return value, some users have experienced non-Windows
+   OS kernels that return negative values other than -1. While one can argue
+   that such kernels are technically broken, comparing against values less
+   than 0 covers their out-of-spec return values without imposing incorrect
+   semantics on systems that manage to correctly return -1 for errors, thus
+   increasing Erlang's portability.
+*/
+#ifdef __WIN32__
+#define IS_SOCKET_ERROR(val) ((val) == SOCKET_ERROR)
+#else
+#define IS_SOCKET_ERROR(val) ((val) < 0)
+#endif
+
 #ifdef __WIN32__
 #define  STRNCASECMP strncasecmp
 
@@ -299,6 +314,7 @@ static int my_strncasecmp(const char *s1, const char *s2, size_t n)
 #define INVALID_SOCKET -1
 #define INVALID_EVENT  -1
 #define SOCKET_ERROR   -1
+
 #define SOCKET int
 #define HANDLE long int
 #define FD_READ    ERL_DRV_READ
@@ -3684,7 +3700,7 @@ static int inet_ctl_fdopen(inet_descriptor* desc, int domain, int type,
     unsigned int sz = sizeof(name);
 
     /* check that it is a socket and that the socket is bound */
-    if (sock_name(s, (struct sockaddr*) &name, &sz) == SOCKET_ERROR)
+    if (IS_SOCKET_ERROR(sock_name(s, (struct sockaddr*) &name, &sz)))
 	return ctl_error(sock_errno(), rbuf, rsize);
     desc->s = s;
     if ((desc->event = sock_create_event(desc)) == INVALID_EVENT)
@@ -3696,7 +3712,7 @@ static int inet_ctl_fdopen(inet_descriptor* desc, int domain, int type,
     desc->state = INET_STATE_BOUND; /* assume bound */
     if (type == SOCK_STREAM) { /* check if connected */
 	sz = sizeof(name);
-	if (sock_peer(s, (struct sockaddr*) &name, &sz) != SOCKET_ERROR)
+	if (!IS_SOCKET_ERROR(sock_peer(s, (struct sockaddr*) &name, &sz)))
 	    desc->state = INET_STATE_CONNECTED;
     }
 
@@ -5627,8 +5643,8 @@ static int inet_fill_opts(inet_descriptor* desc,
 		    buf += arg_sz;
 		    len -= arg_sz;
 		}
-		if (sock_getopt(desc->s,proto,type,arg_ptr,&arg_sz) == 
-		    SOCKET_ERROR) {
+		if (IS_SOCKET_ERROR(sock_getopt(desc->s,proto,type,
+						arg_ptr,&arg_sz))) {
 		    TRUNCATE_TO(0,ptr); 
 		    continue;
 		}
@@ -5645,7 +5661,7 @@ static int inet_fill_opts(inet_descriptor* desc,
 	    RETURN_ERROR();
 	}
 	/* We have 5 bytes allocated to ptr */
-	if (sock_getopt(desc->s,proto,type,arg_ptr,&arg_sz) == SOCKET_ERROR) {
+	if (IS_SOCKET_ERROR(sock_getopt(desc->s,proto,type,arg_ptr,&arg_sz))) {
 	    TRUNCATE_TO(0,ptr);
 	    continue;
 	}
@@ -6711,7 +6727,7 @@ static int inet_ctl(inet_descriptor* desc, int cmd, char* buf, int len,
 	if (len != 0)
 	    return ctl_error(EINVAL, rbuf, rsize);
 
-	if (sock_hostname(tbuf, MAXHOSTNAMELEN) == SOCKET_ERROR)
+	if (IS_SOCKET_ERROR(sock_hostname(tbuf, MAXHOSTNAMELEN)))
 	    return ctl_error(sock_errno(), rbuf, rsize);
 	return ctl_reply(INET_REP_OK, tbuf, strlen(tbuf), rbuf, rsize);
     }
@@ -6728,7 +6744,7 @@ static int inet_ctl(inet_descriptor* desc, int cmd, char* buf, int len,
 	    return ctl_error(ENOTCONN, rbuf, rsize);
 	if ((ptr = desc->peer_ptr) == NULL) {
 	    ptr = &peer;
-	    if (sock_peer(desc->s, (struct sockaddr*)ptr,&sz) == SOCKET_ERROR)
+	    if (IS_SOCKET_ERROR(sock_peer(desc->s, (struct sockaddr*)ptr,&sz)))
 		return ctl_error(sock_errno(), rbuf, rsize);
 	}
 	if (inet_get_address(desc->sfamily, tbuf, ptr, &sz) < 0)
@@ -6765,7 +6781,7 @@ static int inet_ctl(inet_descriptor* desc, int cmd, char* buf, int len,
 
 	if ((ptr = desc->name_ptr) == NULL) {
 	    ptr = &name;
-	    if (sock_name(desc->s, (struct sockaddr*)ptr, &sz) == SOCKET_ERROR)
+	    if (IS_SOCKET_ERROR(sock_name(desc->s, (struct sockaddr*)ptr, &sz)))
 		return ctl_error(sock_errno(), rbuf, rsize);
 	}
 	if (inet_get_address(desc->sfamily, tbuf, ptr, &sz) < 0)
@@ -6804,7 +6820,7 @@ static int inet_ctl(inet_descriptor* desc, int cmd, char* buf, int len,
 	if (inet_set_address(desc->sfamily, &local, buf, &len) == NULL)
 	    return ctl_error(EINVAL, rbuf, rsize);
 
-	if (sock_bind(desc->s,(struct sockaddr*) &local, len) == SOCKET_ERROR)
+	if (IS_SOCKET_ERROR(sock_bind(desc->s,(struct sockaddr*) &local, len)))
 	    return ctl_error(sock_errno(), rbuf, rsize);
 
 	desc->state = INET_STATE_BOUND;
@@ -7237,7 +7253,7 @@ static int tcp_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf, int len,
 	if (len != 2)
 	    return ctl_error(EINVAL, rbuf, rsize);
 	backlog = get_int16(buf);
-	if (sock_listen(desc->inet.s, backlog) == SOCKET_ERROR)
+	if (IS_SOCKET_ERROR(sock_listen(desc->inet.s, backlog)))
 	    return ctl_error(sock_errno(), rbuf, rsize);
 	desc->inet.state = TCP_STATE_LISTEN;
 	return ctl_reply(INET_REP_OK, NULL, 0, rbuf, rsize);
@@ -7271,7 +7287,7 @@ static int tcp_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf, int len,
 	
 	code = sock_connect(desc->inet.s, 
 			    (struct sockaddr*) &desc->inet.remote, len);
-	if ((code == SOCKET_ERROR) && 
+	if (IS_SOCKET_ERROR(code) &&
 		((sock_errno() == ERRNO_BLOCK) ||  /* Winsock2 */
 		 (sock_errno() == EINPROGRESS))) {	/* Unix & OSE!! */
           sock_select(INETP(desc), FD_CONNECT, 1);
@@ -7947,7 +7963,7 @@ static int tcp_recv(tcp_descriptor* desc, int request_len)
 
     n = sock_recv(desc->inet.s, desc->i_ptr, nread, 0);
 
-    if (n == SOCKET_ERROR) {
+    if (IS_SOCKET_ERROR(n)) {
 	int err = sock_errno();
 	if (err == ECONNRESET) {
 	    DEBUGF((" => detected close (connreset)\r\n"));
@@ -8449,8 +8465,8 @@ static int tcp_sendv(tcp_descriptor* desc, ErlIOVec* ev)
 		(long)desc->inet.port, desc->inet.s, h_len, len));
 	if (desc->tcp_add_flags & TCP_ADDF_DELAY_SEND) {
 	    n = 0;
-	} else if (sock_sendv(desc->inet.s, ev->iov, vsize, &n, 0) 
-		   == SOCKET_ERROR) {
+	} else if (IS_SOCKET_ERROR(sock_sendv(desc->inet.s, ev->iov,
+					      vsize, &n, 0))) {
 	    if ((sock_errno() != ERRNO_BLOCK) && (sock_errno() != EINTR)) {
 		int err = sock_errno();
 		DEBUGF(("tcp_sendv(%ld): s=%d, "
@@ -8543,7 +8559,7 @@ static int tcp_send(tcp_descriptor* desc, char* ptr, int len)
 	if (desc->tcp_add_flags & TCP_ADDF_DELAY_SEND) {
 	    sock_send(desc->inet.s, buf, 0, 0);
 	    n = 0;
-	} else 	if (sock_sendv(desc->inet.s,iov,2,&n,0) == SOCKET_ERROR) {
+	} else 	if (IS_SOCKET_ERROR(sock_sendv(desc->inet.s,iov,2,&n,0))) {
 	    if ((sock_errno() != ERRNO_BLOCK) && (sock_errno() != EINTR)) {
 		int err = sock_errno();
 		DEBUGF(("tcp_send(%ld): s=%d,sock_sendv(size=2) errno = %d\r\n",
@@ -8616,7 +8632,7 @@ static int tcp_inet_output(tcp_descriptor* desc, HANDLE event)
 	    int code = sock_peer(desc->inet.s,
 				 (struct sockaddr*) &desc->inet.remote, &sz);
 
-	    if (code == SOCKET_ERROR) {
+	    if (IS_SOCKET_ERROR(code)) {
 		desc->inet.state = TCP_STATE_BOUND;  /* restore state */
 		ret =  async_error(INETP(desc), sock_errno());
 		goto done;
@@ -8657,7 +8673,7 @@ static int tcp_inet_output(tcp_descriptor* desc, HANDLE event)
 	    vsize = vsize > MAX_VSIZE ? MAX_VSIZE : vsize;
 	    DEBUGF(("tcp_inet_output(%ld): s=%d, About to send %d items\r\n", 
 		    (long)desc->inet.port, desc->inet.s, vsize));
-	    if (sock_sendv(desc->inet.s, iov, vsize, &n, 0)==SOCKET_ERROR) {
+	    if (IS_SOCKET_ERROR(sock_sendv(desc->inet.s, iov, vsize, &n, 0))) {
 		if ((sock_errno() != ERRNO_BLOCK) && (sock_errno() != EINTR)) {
 		    DEBUGF(("tcp_inet_output(%ld): sock_sendv(%d) errno = %d\r\n",
 			    (long)desc->inet.port, vsize, sock_errno()));
@@ -8926,7 +8942,7 @@ static int packet_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf, int len,
 	    sock_select(desc, FD_CONNECT, 1);
 	    code = sock_connect(desc->s, &remote.sa, len);
 
-	    if ((code == SOCKET_ERROR) && (sock_errno() == EINPROGRESS)) {
+	    if (IS_SOCKET_ERROR(code) && (sock_errno() == EINPROGRESS)) {
 		/* XXX: Unix only -- WinSock would have a different cond! */
 		desc->state = SCTP_STATE_CONNECTING;
 		if (timeout != INET_INFINITY)
@@ -8966,7 +8982,7 @@ static int packet_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf, int len,
 	    
 	    code = sock_connect(desc->s,
 				(struct sockaddr*) &desc->remote, len);
-	    if (code == SOCKET_ERROR) {
+	    if (IS_SOCKET_ERROR(code)) {
 		sock_connect(desc->s, (struct sockaddr*) NULL, 0);
 		desc->state &= ~INET_F_ACTIVE;
 		return ctl_error(sock_errno(), rbuf, rsize);
@@ -9000,7 +9016,7 @@ static int packet_inet_ctl(ErlDrvData e, unsigned int cmd, char* buf, int len,
 		return ctl_error(EINVAL, rbuf, rsize);
 	    flag = get_int8(buf);
 
-	    if (sock_listen(desc->s, flag) == SOCKET_ERROR)
+	    if (IS_SOCKET_ERROR(sock_listen(desc->s, flag)))
 		return ctl_error(sock_errno(), rbuf, rsize);
 
 	    desc->state = SCTP_STATE_LISTEN;   /* XXX: not used? */
@@ -9205,7 +9221,7 @@ static void packet_inet_command(ErlDrvData e, char* buf, int len)
  check_result_code:
     /* "code" analysis is the same for both SCTP and UDP cases above: */
 #endif
-    if (code == SOCKET_ERROR) {
+    if (IS_SOCKET_ERROR(code)) {
 	int err = sock_errno();
 	inet_reply_error(desc, err);
     }
@@ -9304,7 +9320,7 @@ static int packet_inet_input(udp_descriptor* udesc, HANDLE event)
     check_result:
 #endif
 	/* Analyse the result: */
-	if (n == SOCKET_ERROR
+	if (IS_SOCKET_ERROR(n)
 #ifdef HAVE_SCTP
 	    || (short_recv = (IS_SCTP(desc) && !(mhdr.msg_flags & MSG_EOR)))
 	    /* NB: here we check for EOR not being set -- this is an error as
@@ -9419,7 +9435,7 @@ static int packet_inet_output(udp_descriptor* udesc, HANDLE event)
 	    int code = sock_peer(desc->s,
 				 (struct sockaddr*) &desc->remote, &sz);
 
-	    if (code == SOCKET_ERROR) {
+	    if (IS_SOCKET_ERROR(code)) {
 		desc->state = PACKET_STATE_BOUND;  /* restore state */
 		ret =  async_error(desc, sock_errno());
 		goto done;
@@ -9860,23 +9876,26 @@ int erts_sock_connect(erts_sock_t socket, byte *ip_addr, int len, Uint16 port)
     if (!inet_set_address(AF_INET, &addr, buf, &blen))
 	return 0;
 
-    if (SOCKET_ERROR == sock_connect(s,
+    if (IS_SOCKET_ERROR(sock_connect(s,
 				     (struct sockaddr *) &addr,
-				     sizeof(struct sockaddr_in)))
+				     sizeof(struct sockaddr_in))))
 	return 0;
     return 1;
 }
 
 Sint erts_sock_send(erts_sock_t socket, const void *buf, Sint len)
 {
-    return (Sint) sock_send((SOCKET) socket, buf, (size_t) len, 0);
+    Sint result = (Sint) sock_send((SOCKET) socket, buf, (size_t) len, 0);
+    if (IS_SOCKET_ERROR(result))
+	return SOCKET_ERROR;
+    return result;
 }
 
 
 int erts_sock_gethostname(char *buf, int bufsz)
 {
-    if (sock_hostname(buf, bufsz) == SOCKET_ERROR)
-	return -1;
+    if (IS_SOCKET_ERROR(sock_hostname(buf, bufsz)))
+	return SOCKET_ERROR;
     return 0;
 }