From: Philip Hazel <ph10@hermes.cam.ac.uk>
Date: Tue, 4 Apr 2006 09:09:44 +0000 (+0000)
Subject: Fix subtle but important bug in ip_connect(); it shouldn't close the
X-Git-Tag: exim-4_61~6
X-Git-Url: https://git.exim.org/exim.git/commitdiff_plain/d515a9174a5ea517bc3d27bc4d40223b24d7a47f?ds=sidebyside

Fix subtle but important bug in ip_connect(); it shouldn't close the
socket on a connection error. Also ensure that socket is closed in
iplookup.c after ip_connect() failure.
---

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index ddc8b47ab..fcf835025 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -1,4 +1,4 @@
-$Cambridge: exim/doc/doc-txt/ChangeLog,v 1.336 2006/04/04 08:35:39 ph10 Exp $
+$Cambridge: exim/doc/doc-txt/ChangeLog,v 1.337 2006/04/04 09:09:44 ph10 Exp $
 
 Change log file for Exim from version 4.21
 -------------------------------------------
@@ -340,6 +340,18 @@ PH/69 The HTML version of the specification is now built in a directory called
 
 PH/70 Catch two compiler warnings in sieve.c.
 
+PH/71 Fixed an obscure and subtle bug (thanks Alexander & Matthias). The
+      function verify_get_ident() calls ip_connect() to connect a socket, but
+      if the "connect()" function timed out, ip_connect() used to close the
+      socket. However, verify_get_ident() also closes the socket later, and in
+      between Exim writes to the log, which may get opened at this point. When
+      the socket was closed in ip_connect(), the log could get the same file
+      descriptor number as the socket. This naturally causes chaos. The fix is
+      not to close the socket in ip_connect(); the socket should be closed by
+      the function that creates it. There was only one place in the code where
+      this was missing, in the iplookup router, which I don't think anybody now
+      uses, but I've fixed it anyway.
+
 
 Exim version 4.60
 -----------------
diff --git a/src/ACKNOWLEDGMENTS b/src/ACKNOWLEDGMENTS
index fe9ac9d35..4fb6c922d 100644
--- a/src/ACKNOWLEDGMENTS
+++ b/src/ACKNOWLEDGMENTS
@@ -1,4 +1,4 @@
-$Cambridge: exim/src/ACKNOWLEDGMENTS,v 1.44 2006/03/16 12:07:55 ph10 Exp $
+$Cambridge: exim/src/ACKNOWLEDGMENTS,v 1.45 2006/04/04 09:09:45 ph10 Exp $
 
 EXIM ACKNOWLEDGEMENTS
 
@@ -20,7 +20,7 @@ relatively small patches.
 Philip Hazel
 
 Lists created: 20 November 2002
-Last updated:  16 March 2006
+Last updated:  04 April 2006
 
 
 THE OLD LIST
@@ -173,6 +173,7 @@ Tom Kistner               SPA server code
                             extension (exiscan)
 Jürgen Kreileder          Fix for cyrus_sasl advertisement problem
 Friso Kuipers             Patch for GDBM problem
+Matthias Lederhofer       Diagnosing and patching obscure and subtle socket bug
 Chris Liddiard            Fix for bug in exiqsumm
 Chris Lightfoot           Patch for -restore-times in exim_lock
 Edgar Lovecraft           Patch for ${str2b64:
@@ -199,6 +200,7 @@ Alex Miller               Suggested readline() patch
                           Support for the DrWeb content scanner
 Andreas Mueller           Patch for logging uncompleted SMTP transactions
 Pete Naylor               Patch for LDAP TCP connect timeout setting
+Alexander Newmann         Diagnosing and patching obscure and subtle socket bug
 Matthew Newton            Patch for exicyclog log location problem
 Marcin Owsiany            Diagnosis of a tricky timeout failure bug
 Eric Parusel              Patch for tls_remember_esmtp
diff --git a/src/src/ip.c b/src/src/ip.c
index 0e4eab650..c15e50147 100644
--- a/src/src/ip.c
+++ b/src/src/ip.c
@@ -1,4 +1,4 @@
-/* $Cambridge: exim/src/src/ip.c,v 1.5 2006/02/16 10:05:33 ph10 Exp $ */
+/* $Cambridge: exim/src/src/ip.c,v 1.6 2006/04/04 09:09:45 ph10 Exp $ */
 
 /*************************************************
 *     Exim - an Internet mail transport agent    *
@@ -165,7 +165,9 @@ return bind(sock, (struct sockaddr *)&sin, s_len);
 *************************************************/
 
 /* This function connects a socket to a remote address and port. The socket may
-or may not have previously been bound to a local interface.
+or may not have previously been bound to a local interface. The socket is not
+closed, even in cases of error. It is expected that the calling function, which
+created the socket, will be the one that closes it.
 
 Arguments:
   sock        the socket
@@ -243,7 +245,6 @@ if (rc >= 0) return 0;
 /* A failure whose error code is "Interrupted system call" is in fact
 an externally applied timeout if the signal handler has been run. */
 
-(void)close(sock);
 errno = (save_errno == EINTR && sigalrm_seen)? ETIMEDOUT : save_errno;
 return -1;
 }
diff --git a/src/src/routers/iplookup.c b/src/src/routers/iplookup.c
index 8c17c62fa..9dafd6fa5 100644
--- a/src/src/routers/iplookup.c
+++ b/src/src/routers/iplookup.c
@@ -1,4 +1,4 @@
-/* $Cambridge: exim/src/src/routers/iplookup.c,v 1.6 2006/02/07 11:19:02 ph10 Exp $ */
+/* $Cambridge: exim/src/src/routers/iplookup.c,v 1.7 2006/04/04 09:09:45 ph10 Exp $ */
 
 /*************************************************
 *     Exim - an Internet mail transport agent    *
@@ -240,6 +240,7 @@ while ((hostname = string_nextinlist(&listptr, &sep, host_buffer,
 
     if (ip_connect(query_socket, host_af, h->address,ob->port, ob->timeout) < 0)
       {
+      close(query_socket);
       DEBUG(D_route)
         debug_printf("connection to %s failed: %s\n", h->address,
           strerror(errno));