Bug 4234: comm_connect_addr uses errno incorrectly

comm_connect_addr() uses errno to determine whether library calls like connect()
are successful. Its callers also use errno for extra information on the cause
of any problem. However, after calling library functions like connect(),
comm_connect_addr() calls other library functions which can overwrite errno.

As the errno manpage explains, "a function that succeeds is allowed to change
errno". So even when nothing is wrong, comm_connect_addr() may return an error
flag if libc sets errno. And when something *is* wrong, incorrect error information
may be returned to the caller because errno was overwritten with a different code.

Correct this by using our own error code variable which is set only when a library
call fails. To avoid breaking callers, set errno before returning.

diff --git a/src/comm.cc b/src/comm.cc
index 89786f57f05b4fdb3921f5b493193f536e1e2ef5..d73867bf6731c5d1645144503f23b0c790595505 100644 (file)
--- a/src/comm.cc
+++ b/src/comm.cc
@@ -581,6 +581,12 @@ commUnsetConnTimeout(const Comm::ConnectionPointer &conn)
     return commSetConnTimeout(conn, -1, nil);
 }
 
+
+/**
+ * Connect socket FD to given remote address.
+ * If return value is an error flag (COMM_ERROR, ERR_CONNECT, ERR_PROTOCOL, etc.),
+ * then error code will also be returned in errno.
+ */
 int
 comm_connect_addr(int sock, const Ip::Address &address)
 {
@@ -621,7 +627,7 @@ comm_connect_addr(int sock, const Ip::Address &address)
     address.getAddrInfo(AI, F->sock_family);
 
     /* Establish connection. */
-    errno = 0;
+    int xerrno = 0;
 
     if (!F->flags.called_connect) {
         F->flags.called_connect = true;
@@ -633,10 +639,8 @@ comm_connect_addr(int sock, const Ip::Address &address)
         // Async calls development will fix this.
         if (x == 0) {
             x = -1;
-            errno = EINPROGRESS;
-        }
-
-        if (x < 0) {
+            xerrno = EINPROGRESS;
+        } else if (x < 0) {
             debugs(5,5, "comm_connect_addr: sock=" << sock << ", addrinfo( " <<
                    " flags=" << AI->ai_flags <<
                    ", family=" << AI->ai_family <<
@@ -645,30 +649,28 @@ comm_connect_addr(int sock, const Ip::Address &address)
                    ", &addr=" << AI->ai_addr <<
                    ", addrlen=" << AI->ai_addrlen <<
                    " )" );
-            debugs(5, 9, "connect FD " << sock << ": (" << x << ") " << xstrerror());
+            debugs(5, 9, "connect FD " << sock << ": (" << x << ") " << xstrerr(xerrno));
             debugs(14,9, "connecting to: " << address );
         }
+
     } else {
+        errno = 0;
 #if _SQUID_NEWSOS6_
         /* Makoto MATSUSHITA <matusita@ics.es.osaka-u.ac.jp> */
+        if (connect(sock, AI->ai_addr, AI->ai_addrlen) < 0)
+            xerrno = errno;
 
-        connect(sock, AI->ai_addr, AI->ai_addrlen);
-
-        if (errno == EINVAL) {
+        if (xerrno == EINVAL) {
             errlen = sizeof(err);
             x = getsockopt(sock, SOL_SOCKET, SO_ERROR, &err, &errlen);
-
             if (x >= 0)
-                errno = x;
+                xerrno = x;
         }
-
 #else
         errlen = sizeof(err);
-
         x = getsockopt(sock, SOL_SOCKET, SO_ERROR, &err, &errlen);
-
         if (x == 0)
-            errno = err;
+            xerrno = err;
 
 #if _SQUID_SOLARIS_
         /*
@@ -677,23 +679,24 @@ comm_connect_addr(int sock, const Ip::Address &address)
         * connect and just returns EPIPE.  Create a fake
         * error message for connect.   -- fenner@parc.xerox.com
         */
-        if (x < 0 && errno == EPIPE)
-            errno = ENOTCONN;
-
+        if (x < 0 && xerrno == EPIPE)
+            xerrno = ENOTCONN;
+        else
+            xerrno = errno;
 #endif
 #endif
-
     }
 
     Ip::Address::FreeAddr(AI);
 
     PROF_stop(comm_connect_addr);
 
-    if (errno == 0 || errno == EISCONN)
+    errno = xerrno;
+    if (xerrno == 0 || xerrno == EISCONN)
         status = Comm::OK;
-    else if (ignoreErrno(errno))
+    else if (ignoreErrno(xerrno))
         status = Comm::INPROGRESS;
-    else if (errno == EAFNOSUPPORT || errno == EINVAL)
+    else if (xerrno == EAFNOSUPPORT || xerrno == EINVAL)
         return Comm::ERR_PROTOCOL;
     else
         return Comm::COMM_ERROR;
@@ -708,6 +711,7 @@ comm_connect_addr(int sock, const Ip::Address &address)
         debugs(5, DBG_DATA, "comm_connect_addr: FD " << sock << " connection pending");
     }
 
+    errno = xerrno;
     return status;
 }