Delivery: quieten smtp transport conn reuse vs. delivery race. Bug 1810

The fix is in deliver.c only.  The remainder is just tidying.

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index cecd2a0388173049d9da37e23be9c3c5d632f832..43009011fec675234d783748e9e47322f0e0f6ae 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -20,6 +20,9 @@ JH/03 Upgrade security requirements imposed for hosts_try_dane: previously
       This means that a poorly-configured remote DNS will make it incommunicado;
       but it protects against a DNS-interception attack on it.
 
+JH/04 Bug 1810: make continued-use of an open smtp transport connection
+      non-noisy when a race steals the message being considered.
+
 
 Exim version 4.87
 -----------------

diff --git a/src/src/deliver.c b/src/src/deliver.c
index 5c6a983fefb51b2ceeecb12b182c76ad1269eec8..a1fb602e9d02b38d6ad83d42bb2176cae64d9fd2 100644 (file)
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -5226,7 +5226,7 @@ Any failures cause messages to be written to the log, except for missing files
 while queue running - another process probably completed delivery. As part of
 opening the data file, message_subdir gets set. */
 
-if (!spool_open_datafile(id))
+if ((deliver_datafile = spool_open_datafile(id)) < 0)
   return continue_closedown();  /* yields DELIVER_NOT_ATTEMPTED */
 
 /* The value of message_size at this point has been set to the data length,
@@ -8091,8 +8091,17 @@ deliver_get_sender_address (uschar * id)
 int rc;
 uschar * new_sender_address,
        * save_sender_address;
+BOOL save_qr = queue_running;
 
-if (!spool_open_datafile(id))
+/* make spool_open_datafile non-noisy on fail */
+
+queue_running = TRUE;
+
+/* Side effect: message_subdir is set for the (possibly split) spool directory */
+
+deliver_datafile = spool_open_datafile(id);
+queue_running = save_qr;
+if (deliver_datafile < 0)
   return NULL;
 
 /* Save and restore the global sender_address.  I'm not sure if we should

diff --git a/src/src/exim.c b/src/src/exim.c
index 490248917775b35e2272a67180409b1aacc2ddf3..4ea42fdc249938c20b745b2f85bfcc45f1f5b769 100644 (file)
--- a/src/src/exim.c
+++ b/src/src/exim.c
@@ -4952,7 +4952,7 @@ if (expansion_test)
       }
     message_id = argv[msg_action_arg];
     (void)string_format(spoolname, sizeof(spoolname), "%s-H", message_id);
-    if (!spool_open_datafile(message_id))
+    if ((deliver_datafile = spool_open_datafile(message_id)) < 0)
       printf ("Failed to load message datafile %s\n", message_id);
     if (spool_read_header(spoolname, TRUE, FALSE) != spool_read_OK)
       printf ("Failed to load message %s\n", message_id);

diff --git a/src/src/functions.h b/src/src/functions.h
index 6c3c37463aba66d8f745ee8e02d3112484498cbf..454037fe130324da646cf425376f875582ec7bce 100644 (file)
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -405,7 +405,7 @@ extern int     spam(const uschar **);
 extern FILE   *spool_mbox(unsigned long *, const uschar *);
 #endif
 extern BOOL    spool_move_message(uschar *, uschar *, uschar *, uschar *);
-extern BOOL    spool_open_datafile(uschar *);
+extern int     spool_open_datafile(uschar *);
 extern int     spool_open_temp(uschar *);
 extern int     spool_read_header(uschar *, BOOL, BOOL);
 extern int     spool_write_header(uschar *, int, uschar **);

diff --git a/src/src/queue.c b/src/src/queue.c
index bf62aea886c8f360976fae1d51fc0064be11555b..cc8d36b235274d1c7957e33877bd00b8c2e0867c 100644 (file)
--- a/src/src/queue.c
+++ b/src/src/queue.c
@@ -1030,7 +1030,7 @@ other process is working on this message. If the file does not exist, continue
 only if the action is remove and the user is an admin user, to allow for
 tidying up broken states. */
 
-if (!spool_open_datafile(id))
+if ((deliver_datafile = spool_open_datafile(id)) < 0)
   {
   if (errno == ENOENT)
     {

diff --git a/src/src/spool_in.c b/src/src/spool_in.c
index 08543721247ce95a6a3e116c956a758e58e189cd..cafca603d54f09c5f169c3ea8fcdeb1f773cf14f 100644 (file)
--- a/src/src/spool_in.c
+++ b/src/src/spool_in.c
@@ -26,18 +26,19 @@ overwriting some other file descriptor with the value of this one), open it
 with append.
 
 Argument: the id of the message
-Returns:  TRUE if file successfully opened and locked
+Returns:  fd if file successfully opened and locked, else -1
 
-Side effect: deliver_datafile is set to the fd of the open file.
+Side effect: message_subdir is set for the (possibly split) spool directory
 */
 
-BOOL
+int
 spool_open_datafile(uschar *id)
 {
 int i;
 struct stat statbuf;
 flock_t lock_data;
 uschar spoolname[256];
+int fd;
 
 /* If split_spool_directory is set, first look for the file in the appropriate
 sub-directory of the input directory. If it is not found there, try the input
@@ -51,8 +52,8 @@ for (i = 0; i < 2; i++)
   int save_errno;
   message_subdir[0] = (split_spool_directory == (i == 0))? id[5] : 0;
   sprintf(CS spoolname, "%s/input/%s/%s-D", spool_directory, message_subdir, id);
-  deliver_datafile = Uopen(spoolname, O_RDWR | O_APPEND, 0);
-  if (deliver_datafile >= 0) break;
+  if ((fd = Uopen(spoolname, O_RDWR | O_APPEND, 0)) >= 0)
+    break;
   save_errno = errno;
   if (errno == ENOENT)
     {
@@ -63,7 +64,7 @@ for (i = 0; i < 2; i++)
   else log_write(0, LOG_MAIN, "Spool error for %s: %s", spoolname,
     strerror(errno));
   errno = save_errno;
-  return FALSE;
+  return -1;
   }
 
 /* File is open and message_subdir is set. Set the close-on-exec flag, and lock
@@ -74,7 +75,7 @@ an open file descriptor (at least, I think that's the Cygwin story). On real
 Unix systems it doesn't make any difference as long as Exim is consistent in
 what it locks. */
 
-(void)fcntl(deliver_datafile, F_SETFD, fcntl(deliver_datafile, F_GETFD) |
+(void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) |
   FD_CLOEXEC);
 
 lock_data.l_type = F_WRLCK;
@@ -82,27 +83,26 @@ lock_data.l_whence = SEEK_SET;
 lock_data.l_start = 0;
 lock_data.l_len = SPOOL_DATA_START_OFFSET;
 
-if (fcntl(deliver_datafile, F_SETLK, &lock_data) < 0)
+if (fcntl(fd, F_SETLK, &lock_data) < 0)
   {
   log_write(L_skip_delivery,
             LOG_MAIN,
             "Spool file is locked (another process is handling this message)");
-  (void)close(deliver_datafile);
-  deliver_datafile = -1;
+  (void)close(fd);
   errno = 0;
-  return FALSE;
+  return -1;
   }
 
 /* Get the size of the data; don't include the leading filename line
 in the count, but add one for the newline before the data. */
 
-if (fstat(deliver_datafile, &statbuf) == 0)
+if (fstat(fd, &statbuf) == 0)
   {
   message_body_size = statbuf.st_size - SPOOL_DATA_START_OFFSET;
   message_size = message_body_size + 1;
   }
 
-return TRUE;
+return fd;
 }
 #endif  /* COMPILE_UTILITY */
 

diff --git a/src/src/transport.c b/src/src/transport.c
index 8ac6f30a095303578d4a359878cdd5b7f4b0cf1e..86350ba9d8ccee39061081d9ea34d449c95830b5 100644 (file)
--- a/src/src/transport.c
+++ b/src/src/transport.c
@@ -1654,15 +1654,10 @@ open_db dbblock;
 open_db *dbm_file;
 uschar buffer[256];
 
-msgq_t      *msgq = NULL;
-int         msgq_count = 0;
-int         msgq_actual = 0;
 int         i;
-BOOL        bFound = FALSE;
 uschar      spool_dir [PATH_MAX];
 uschar      spool_file [PATH_MAX];
 struct stat statbuf;
-BOOL        bContinuation = FALSE;
 
 *more = FALSE;
 
@@ -1692,8 +1687,7 @@ if (dbm_file == NULL) return FALSE;
 
 /* See if there is a record for this host; if not, there's nothing to do. */
 
-host_record = dbfn_read(dbm_file, hostname);
-if (host_record == NULL)
+if (!(host_record = dbfn_read(dbm_file, hostname)))
   {
   dbfn_close(dbm_file);
   DEBUG(D_transport) debug_printf("no messages waiting for %s\n", hostname);
@@ -1726,6 +1720,12 @@ host_length = host_record->count * MESSAGE_ID_LENGTH;
 
 while (1)
   {
+  msgq_t      *msgq;
+  int         msgq_count = 0;
+  int         msgq_actual = 0;
+  BOOL        bFound = FALSE;
+  BOOL        bContinuation = FALSE;
+
   /* create an array to read entire message queue into memory for processing  */
 
   msgq = (msgq_t*) malloc(sizeof(msgq_t) * host_record->count);
@@ -1752,8 +1752,6 @@ while (1)
 
   /* now find the next acceptable message_id */
 
-  bFound = FALSE;
-
   for (i = msgq_count - 1; i >= 0; --i) if (msgq[i].bKeep)
     {
     if (split_spool_directory)
@@ -1779,8 +1777,7 @@ while (1)
       msgq_actual++;
 
   /* reassemble the host record, based on removed message ids, from in
-   * memory queue.
-   */
+  memory queue  */
 
   if (msgq_actual <= 0)
     {
@@ -1807,8 +1804,6 @@ while (1)
 /* Jeremy: check for a continuation record, this code I do not know how to
 test but the code should work */
 
-  bContinuation = FALSE;
-
   while (host_length <= 0)
     {
     int i;
@@ -1839,8 +1834,11 @@ test but the code should work */
     bContinuation = TRUE;
     }
 
-  if (bFound)
+  if (bFound)          /* Usual exit from main loop */
+    {
+    free (msgq);
     break;
+    }
 
   /* If host_length <= 0 we have emptied a record and not found a good message,
   and there are no continuation records. Otherwise there is a continuation
@@ -1859,20 +1857,13 @@ test but the code should work */
 
   if (!bContinuation)
     {
-    Ustrcpy (new_message_id, message_id);
+    Ustrcpy(new_message_id, message_id);
     dbfn_close(dbm_file);
     return FALSE;
     }
-  }            /* we need to process a continuation record */
 
-/* clean up in memory queue */
-if (msgq)
-  {
-  free (msgq);
-  msgq = NULL;
-  msgq_count = 0;
-  msgq_actual = 0;
-  }
+  free(msgq);
+  }            /* we need to process a continuation record */
 
 /* Control gets here when an existing message has been encountered; its
 id is in new_message_id, and host_length is the revised length of the
@@ -1881,19 +1872,8 @@ record if required, close the database, and return TRUE. */
 
 if (host_length > 0)
   {
-  uschar  msg [MESSAGE_ID_LENGTH + 1];
-  int i;
-
   host_record->count = host_length/MESSAGE_ID_LENGTH;
 
-  /* rebuild the host_record->text */
-
-  for (i = 0; i < host_record->count; ++i)
-    {
-    Ustrncpy(msg, host_record->text + (i*MESSAGE_ID_LENGTH), MESSAGE_ID_LENGTH);
-    msg[MESSAGE_ID_LENGTH] = 0;
-    }
-
   dbfn_write(dbm_file, hostname, host_record, (int)sizeof(dbdata_wait) + host_length);
   *more = TRUE;
   }

diff --git a/src/src/transports/smtp.c b/src/src/transports/smtp.c
index 361531a9fee83c938d732b3e64c8b0ea4d35a18d..848a4ce2103efdd9aded6ce55e36330e6e8f93a5 100644 (file)
--- a/src/src/transports/smtp.c
+++ b/src/src/transports/smtp.c
@@ -1306,7 +1306,6 @@ we will veto this new message.  */
 static BOOL
 smtp_are_same_identities(uschar * message_id, smtp_compare_t * s_compare)
 {
-
 uschar * message_local_identity,
        * current_local_identity,
        * new_sender_address;