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

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

8 files changed, 45 insertions, 54 deletions

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index cecd2a038..43009011f 100644
--- 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 5c6a983fe..a1fb602e9 100644
--- 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 490248917..4ea42fdc2 100644
--- 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 6c3c37463..454037fe1 100644
--- 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 bf62aea88..cc8d36b23 100644
--- 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 085437212..cafca603d 100644
--- 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 8ac6f30a0..86350ba9d 100644
--- 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 361531a9f..848a4ce21 100644
--- 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;