From 789f8a4f4046120b7ae2aafa45f7f45c3ae4c8f5 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Wed, 27 Apr 2016 00:34:11 +0100 Subject: [PATCH] Delivery: quieten smtp transport conn reuse vs. delivery race. Bug 1810 The fix is in deliver.c only. The remainder is just tidying. --- doc/doc-txt/ChangeLog | 3 +++ src/src/deliver.c | 13 ++++++++-- src/src/exim.c | 2 +- src/src/functions.h | 2 +- src/src/queue.c | 2 +- src/src/spool_in.c | 26 ++++++++++---------- src/src/transport.c | 50 ++++++++++++--------------------------- src/src/transports/smtp.c | 1 - 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; -- 2.30.2