Use SOCKET_OK macros in even more places

Add a TOR_INVALID_SOCKET macro to wrap -1/INVALID_SOCKET.

Partial work for bug4533.

changes/tor_socket_tests

@@ -0,0 +1,8 @@
+  o Minor bugfixes:
+    - Find more places in the code that should have been testing for
+      invalid sockets using the SOCKET_OK macro. Required for a fix
+      for bug 4533. Bugfix on 0.2.2.28-beta.
+
+  o Code simplification and refactoring:
+    - Use a TOR_INVALID_SOCKET macro when initializing a socket to an
+      invalid value, rather than just -1.

src/common/compat.c

@@ -1053,17 +1053,17 @@ tor_socketpair(int family, int type, int protocol, tor_socket_t fd[2])
   r = socketpair(family, type, protocol, fd);
   if (r == 0) {
 #if !defined(SOCK_CLOEXEC) && defined(FD_CLOEXEC)
-    if (fd[0] >= 0)
+    if (SOCKET_OK(fd[0]))
       fcntl(fd[0], F_SETFD, FD_CLOEXEC);
-    if (fd[1] >= 0)
+    if (SOCKET_OK(fd[1]))
       fcntl(fd[1], F_SETFD, FD_CLOEXEC);
 #endif
     socket_accounting_lock();
-    if (fd[0] >= 0) {
+    if (SOCKET_OK(fd[0])) {
       ++n_sockets_open;
       mark_socket_open(fd[0]);
     }
-    if (fd[1] >= 0) {
+    if (SOCKET_OK(fd[1])) {
       ++n_sockets_open;
       mark_socket_open(fd[1]);
     }
@@ -1100,7 +1100,7 @@ tor_socketpair(int family, int type, int protocol, tor_socket_t fd[2])
     }
 
     listener = tor_open_socket(AF_INET, type, 0);
-    if (listener < 0)
+    if (!SOCKET_OK(listener))
       return -tor_socket_errno(-1);
     memset(&listen_addr, 0, sizeof(listen_addr));
     listen_addr.sin_family = AF_INET;
@@ -1113,7 +1113,7 @@ tor_socketpair(int family, int type, int protocol, tor_socket_t fd[2])
       goto tidy_up_and_fail;
 
     connector = tor_open_socket(AF_INET, type, 0);
-    if (connector < 0)
+    if (!SOCKET_OK(connector))
       goto tidy_up_and_fail;
     /* We want to find out the port number to connect to.  */
     size = sizeof(connect_addr);
@@ -1128,7 +1128,7 @@ tor_socketpair(int family, int type, int protocol, tor_socket_t fd[2])
     size = sizeof(listen_addr);
     acceptor = tor_accept_socket(listener,
                                  (struct sockaddr *) &listen_addr, &size);
-    if (acceptor < 0)
+    if (!SOCKET_OK(acceptor))
       goto tidy_up_and_fail;
     if (size != sizeof(listen_addr))
       goto abort_tidy_up_and_fail;

src/common/compat.h

@@ -399,11 +399,19 @@ typedef int socklen_t;
 #endif
 
 #ifdef MS_WINDOWS
+/* XXX Actually, this should arguably be SOCKET; we use intptr_t here so that
+ * any inadvertant checks for the socket being <= 0 or > 0 will probably
+ * still work. */
 #define tor_socket_t intptr_t
 #define SOCKET_OK(s) ((unsigned)(s) != INVALID_SOCKET)
+#define TOR_INVALID_SOCKET INVALID_SOCKET
 #else
+/** Type used for a network socket. */
 #define tor_socket_t int
+/** Macro: true iff 's' is a possible value for a valid initialized socket. */
 #define SOCKET_OK(s) ((s) >= 0)
+/** Error/uninitialized value for a tor_socket_t. */
+#define TOR_INVALID_SOCKET (-1)
 #endif
 
 int tor_close_socket(tor_socket_t s);

src/or/buffers.c

@@ -690,7 +690,7 @@ read_to_buf(tor_socket_t s, size_t at_most, buf_t *buf, int *reached_eof,
 
   check();
   tor_assert(reached_eof);
-  tor_assert(s >= 0);
+  tor_assert(SOCKET_OK(s));
 
   while (at_most > total_read) {
     size_t readlen = at_most - total_read;
@@ -858,7 +858,7 @@ flush_buf(tor_socket_t s, buf_t *buf, size_t sz, size_t *buf_flushlen)
   int r;
   size_t flushed = 0;
   tor_assert(buf_flushlen);
-  tor_assert(s >= 0);
+  tor_assert(SOCKET_OK(s));
   tor_assert(*buf_flushlen <= buf->datalen);
   tor_assert(sz <= *buf_flushlen);
 

src/or/connection.c

@@ -372,7 +372,7 @@ connection_init(time_t now, connection_t *conn, int type, int socket_family)
       break;
   }
 
-  conn->s = -1; /* give it a default of 'not used' */
+  conn->s = TOR_INVALID_SOCKET; /* give it a default of 'not used' */
   conn->conn_array_index = -1; /* also default to 'not used' */
   conn->global_identifier = n_connections_allocated++;
 
@@ -395,8 +395,8 @@ connection_init(time_t now, connection_t *conn, int type, int socket_family)
 void
 connection_link_connections(connection_t *conn_a, connection_t *conn_b)
 {
-  tor_assert(conn_a->s < 0);
-  tor_assert(conn_b->s < 0);
+  tor_assert(! SOCKET_OK(conn_a->s));
+  tor_assert(! SOCKET_OK(conn_b->s));
 
   conn_a->linked = 1;
   conn_b->linked = 1;
@@ -540,7 +540,7 @@ _connection_free(connection_t *conn)
   if (SOCKET_OK(conn->s)) {
     log_debug(LD_NET,"closing fd %d.",(int)conn->s);
     tor_close_socket(conn->s);
-    conn->s = -1;
+    conn->s = TOR_INVALID_SOCKET;
   }
 
   if (conn->type == CONN_TYPE_OR &&
@@ -625,7 +625,7 @@ connection_about_to_close_connection(connection_t *conn)
 /** Return true iff connection_close_immediate() has been called on this
  * connection. */
 #define CONN_IS_CLOSED(c) \
-  ((c)->linked ? ((c)->linked_conn_is_closed) : ((c)->s < 0))
+  ((c)->linked ? ((c)->linked_conn_is_closed) : (! SOCKET_OK(c->s)))
 
 /** Close the underlying socket for <b>conn</b>, so we don't try to
  * flush it. Must be used in conjunction with (right before)
@@ -651,7 +651,7 @@ connection_close_immediate(connection_t *conn)
 
   if (SOCKET_OK(conn->s))
     tor_close_socket(conn->s);
-  conn->s = -1;
+  conn->s = TOR_INVALID_SOCKET;
   if (conn->linked)
     conn->linked_conn_is_closed = 1;
   if (conn->outbuf)
@@ -958,7 +958,7 @@ connection_create_listener(const struct sockaddr *listensockaddr,
       goto err;
     }
     s = tor_open_socket(AF_UNIX, SOCK_STREAM, 0);
-    if (s < 0) {
+    if (! SOCKET_OK(s)) {
       log_warn(LD_NET,"Socket creation failed: %s.", strerror(errno));
       goto err;
     }
@@ -1331,7 +1331,7 @@ connection_connect(connection_t *conn, const char *address,
   }
 
   s = tor_open_socket(protocol_family,SOCK_STREAM,IPPROTO_TCP);
-  if (s < 0) {
+  if (! SOCKET_OK(s)) {
     *socket_error = tor_socket_errno(-1);
     log_warn(LD_NET,"Error creating network socket: %s",
              tor_socket_strerror(*socket_error));

src/or/cpuworker.c

@@ -329,8 +329,8 @@ spawn_cpuworker(void)
     return -1;
   }
 
-  tor_assert(fdarray[0] >= 0);
-  tor_assert(fdarray[1] >= 0);
+  tor_assert(SOCKET_OK(fdarray[0]));
+  tor_assert(SOCKET_OK(fdarray[1]));
 
   fd = fdarray[0];
   spawn_func(cpuworker_main, (void*)fdarray);

src/or/dns.c

@@ -454,7 +454,7 @@ purge_expired_resolves(time_t now)
         pend = resolve->pending_connections;
         resolve->pending_connections = pend->next;
         /* Connections should only be pending if they have no socket. */
-        tor_assert(pend->conn->_base.s == -1);
+        tor_assert(pend->conn->_base.s == TOR_INVALID_SOCKET);
         pendconn = pend->conn;
         connection_edge_end(pendconn, END_STREAM_REASON_TIMEOUT);
         circuit_detach_stream(circuit_get_by_edge_conn(pendconn), pendconn);
@@ -681,7 +681,7 @@ dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
   uint8_t is_reverse = 0;
   int r;
   assert_connection_ok(TO_CONN(exitconn), 0);
-  tor_assert(exitconn->_base.s == -1);
+  tor_assert(exitconn->_base.s == TOR_INVALID_SOCKET);
   assert_cache_ok();
   tor_assert(oncirc);
 
@@ -849,7 +849,7 @@ assert_all_pending_dns_resolves_ok(void)
          pend;
          pend = pend->next) {
       assert_connection_ok(TO_CONN(pend->conn), 0);
-      tor_assert(pend->conn->_base.s == -1);
+      tor_assert(pend->conn->_base.s == TOR_INVALID_SOCKET);
       tor_assert(!connection_in_array(TO_CONN(pend->conn)));
     }
   }
@@ -955,7 +955,7 @@ dns_cancel_pending_resolve(const char *address)
     pend->conn->_base.state = EXIT_CONN_STATE_RESOLVEFAILED;
     pendconn = pend->conn;
     assert_connection_ok(TO_CONN(pendconn), 0);
-    tor_assert(pendconn->_base.s == -1);
+    tor_assert(pendconn->_base.s == TOR_INVALID_SOCKET);
     if (!pendconn->_base.marked_for_close) {
       connection_edge_end(pendconn, END_STREAM_REASON_RESOLVEFAILED);
     }