Cutthrough: fix race resulting in duplicate-delivery. Bug 2273

Cherry-picked from: cfbb0d24e8

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 5e1ff056ad13b07c5d88e4b97a496e7b7bee9880..d48ea832442a4f194ca9a9d310d307f9e7d240da 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -15,6 +15,11 @@ JH/03 Bug 2269: When presented with a received message having a stupidly large
       number of DKIM-Signature headers, disable DKIM verification to avoid
       a resource-consumption attack.  The limit is set at twenty.
 
+JH/05 Bug 2273: Cutthrough delivery left a window where the received messsage
+      files in the spool were present and unlocked.  A queue-runner could spot
+      them, resulting in a duplicate delivery.  Fix that by doing the unlock
+      after the unlink.  Investigation by Time Stewart.
+
 
 Exim version 4.91
 -----------------

diff --git a/src/src/receive.c b/src/src/receive.c
index 3b215f23e9cb590c54e8e82eda0bdee9815875b0..e7dc910bd21ebecf5f85652deb3e2b5173548cc7 100644 (file)
--- a/src/src/receive.c
+++ b/src/src/receive.c
@@ -4228,22 +4228,30 @@ if (deliver_freeze && freeze_tell != NULL && freeze_tell[0] != 0)
 
 /* Either a message has been successfully received and written to the two spool
 files, or an error in writing the spool has occurred for an SMTP message, or
-an SMTP message has been rejected for policy reasons. (For a non-SMTP message
-we will have already given up because there's no point in carrying on!) In
-either event, we must now close (and thereby unlock) the data file. In the
-successful case, this leaves the message on the spool, ready for delivery. In
-the error case, the spool file will be deleted. Then tidy up store, interact
-with an SMTP call if necessary, and return.
+an SMTP message has been rejected for policy reasons, or a message was passed on
+by cutthrough delivery. (For a non-SMTP message we will have already given up
+because there's no point in carrying on!) For non-cutthrough we must now close
+(and thereby unlock) the data file. In the successful case, this leaves the
+message on the spool, ready for delivery. In the error case, the spool file will
+be deleted. Then tidy up store, interact with an SMTP call if necessary, and
+return.
+
+For cutthrough we hold the data file locked until we have deleted it, otherwise
+a queue-runner could grab it in the window.
 
 A fflush() was done earlier in the expectation that any write errors on the
 data file will be flushed(!) out thereby. Nevertheless, it is theoretically
 possible for fclose() to fail - but what to do? What has happened to the lock
-if this happens? */
+if this happens?  We can at least log it; if it is observed on some platform
+then we can think about properly declaring the message not-received. */
 
 
 TIDYUP:
-process_info[process_info_len] = 0;                /* Remove message id */
-if (data_file != NULL) (void)fclose(data_file);    /* Frees the lock */
+process_info[process_info_len] = 0;                    /* Remove message id */
+if (data_file && cutthrough_done == NOT_TRIED)
+  if (fclose(data_file))                               /* Frees the lock */
+    log_write(0, LOG_MAIN|LOG_PANIC,
+      "spoolfile error on close: %s", strerror(errno));
 
 /* Now reset signal handlers to their defaults */
 
@@ -4330,6 +4338,8 @@ if (smtp_input)
       }
     if (cutthrough_done != NOT_TRIED)
       {
+      if (data_file)
+       (void) fclose(data_file);  /* Frees the lock; do not care if error */
       message_id[0] = 0;         /* Prevent a delivery from starting */
       cutthrough.delivery = cutthrough.callout_hold_only = FALSE;
       cutthrough.defer_pass = FALSE;