+++ /dev/null
-From bd21a787cdeef803334a6c7bf50d23b2a18cbd6f Mon Sep 17 00:00:00 2001
-From: Wolfgang Breyha <wbreyha@gmx.net>
-Date: Sun, 28 Sep 2014 13:40:45 +0100
-Subject: [PATCH] Fix transport-results pipe for multiple recipients combined
- with certs.
-
-The previous parsing failed when a result item split over a buffer boundary;
-fix by prefixing sizes to items, and checking enough has been read as the
-initial parsing stage.
----
- doc/doc-txt/ChangeLog | 7 +++
- src/deliver.c | 162 +++++++++++++++++++++++++++++++++++++-------------
- src/macros.h | 4 ++
- 3 files changed, 131 insertions(+), 42 deletions(-)
-
-# diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
-# index 46fed6f..76ecc20 100644
-# --- a/doc/doc-txt/ChangeLog
-# +++ b/doc/doc-txt/ChangeLog
-# @@ -37,6 +37,13 @@ TL/04 Bugzilla 1216: Add -M (related messages) option to exigrep.
-# TL/05 GitHub Issue 18: Adjust logic testing for true/false in redis lookups.
-# Merged patch from Sebastian Wiedenroth.
-#
-# +JH/05 Fix results-pipe from transport process. Several recipients, combined
-# + with certificate use, exposed issues where response data items split
-# + over buffer boundaries were not parsed properly. This eventually
-# + resulted in duplicates being sent. This issue only became common enough
-# + to notice due to the introduction of conection certificate information,
-# + the item size being so much larger. Found and fixed by Wolfgang Breyha.
-# +
-# Exim version 4.84
-# -----------------
-# TL/01 Bugzilla 1506: Re-add a 'return NULL' to silence complaints from static
---- a/src/deliver.c
-+++ b/src/deliver.c
-@@ -2823,6 +2823,8 @@ uschar *ptr = endptr;
- uschar *msg = p->msg;
- BOOL done = p->done;
- BOOL unfinished = TRUE;
-+/* minimum size to read is header size including id, subid and length */
-+int required = PIPE_HEADER_SIZE;
-
- /* Loop through all items, reading from the pipe when necessary. The pipe
- is set up to be non-blocking, but there are two different Unix mechanisms in
-@@ -2845,12 +2847,15 @@ while (!done)
- {
- retry_item *r, **rp;
- int remaining = endptr - ptr;
-+ uschar header[PIPE_HEADER_SIZE + 1];
-+ uschar id, subid;
-+ uschar *endc;
-
- /* Read (first time) or top up the chars in the buffer if necessary.
- There will be only one read if we get all the available data (i.e. don't
- fill the buffer completely). */
-
-- if (remaining < 2500 && unfinished)
-+ if (remaining < required && unfinished)
- {
- int len;
- int available = big_buffer_size - remaining;
-@@ -2883,17 +2888,64 @@ while (!done)
- won't read any more, as "unfinished" will get set FALSE. */
-
- endptr += len;
-+ remaining += len;
- unfinished = len == available;
- }
-
- /* If we are at the end of the available data, exit the loop. */
--
- if (ptr >= endptr) break;
-
-+ /* copy and read header */
-+ memcpy(header, ptr, PIPE_HEADER_SIZE);
-+ header[PIPE_HEADER_SIZE] = '\0';
-+ id = header[0];
-+ subid = header[1];
-+ required = Ustrtol(header + 2, &endc, 10) + PIPE_HEADER_SIZE; /* header + data */
-+ if (*endc)
-+ {
-+ msg = string_sprintf("failed to read pipe from transport process "
-+ "%d for transport %s: error reading size from header", pid, addr->transport->driver_name);
-+ done = TRUE;
-+ break;
-+ }
-+
-+ DEBUG(D_deliver)
-+ debug_printf("header read id:%c,subid:%c,size:%s,required:%d,remaining:%d,unfinished:%d\n",
-+ id, subid, header+2, required, remaining, unfinished);
-+
-+ /* is there room for the dataset we want to read ? */
-+ if (required > big_buffer_size - PIPE_HEADER_SIZE)
-+ {
-+ msg = string_sprintf("failed to read pipe from transport process "
-+ "%d for transport %s: big_buffer too small! required size=%d buffer size=%d", pid, addr->transport->driver_name,
-+ required, big_buffer_size - PIPE_HEADER_SIZE);
-+ done = TRUE;
-+ break;
-+ }
-+
-+ /* we wrote all datasets with atomic write() calls
-+ remaining < required only happens if big_buffer was too small
-+ to get all available data from pipe. unfinished has to be true
-+ as well. */
-+ if (remaining < required)
-+ if (unfinished)
-+ continue;
-+ else
-+ {
-+ msg = string_sprintf("failed to read pipe from transport process "
-+ "%d for transport %s: required size=%d > remaining size=%d and unfinished=false",
-+ pid, addr->transport->driver_name, required, remaining);
-+ done = TRUE;
-+ break;
-+ }
-+
-+ /* step behind the header */
-+ ptr += PIPE_HEADER_SIZE;
-+
- /* Handle each possible type of item, assuming the complete item is
- available in store. */
-
-- switch (*ptr++)
-+ switch (id)
- {
- /* Host items exist only if any hosts were marked unusable. Match
- up by checking the IP address. */
-@@ -2990,7 +3042,7 @@ while (!done)
- #ifdef SUPPORT_TLS
- case 'X':
- if (addr == NULL) goto ADDR_MISMATCH; /* Below, in 'A' handler */
-- switch (*ptr++)
-+ switch (subid)
- {
- case '1':
- addr->cipher = NULL;
-@@ -3028,7 +3080,7 @@ while (!done)
- #endif /*SUPPORT_TLS*/
-
- case 'C': /* client authenticator information */
-- switch (*ptr++)
-+ switch (subid)
- {
- case '1':
- addr->authenticator = (*ptr)? string_copy(ptr) : NULL;
-@@ -3051,7 +3103,7 @@ while (!done)
-
- #ifdef EXPERIMENTAL_DSN
- case 'D':
-- if (addr == NULL) break;
-+ if (addr == NULL) goto ADDR_MISMATCH;
- memcpy(&(addr->dsn_aware), ptr, sizeof(addr->dsn_aware));
- ptr += sizeof(addr->dsn_aware);
- DEBUG(D_deliver) debug_printf("DSN read: addr->dsn_aware = %d\n", addr->dsn_aware);
-@@ -3119,7 +3171,7 @@ while (!done)
- continue_hostname = NULL;
- }
- done = TRUE;
-- DEBUG(D_deliver) debug_printf("Z%c item read\n", *ptr);
-+ DEBUG(D_deliver) debug_printf("Z0%c item read\n", *ptr);
- break;
-
- /* Anything else is a disaster. */
-@@ -3572,9 +3624,40 @@ while (parcount > max)
-
-
- static void
--rmt_dlv_checked_write(int fd, void * buf, int size)
-+rmt_dlv_checked_write(int fd, char id, char subid, void * buf, int size)
- {
--int ret = write(fd, buf, size);
-+uschar writebuffer[PIPE_HEADER_SIZE + BIG_BUFFER_SIZE];
-+int header_length;
-+
-+/* we assume that size can't get larger then BIG_BUFFER_SIZE which currently is set to 16k */
-+/* complain to log if someone tries with buffer sizes we can't handle*/
-+
-+if (size > 99999)
-+{
-+ log_write(0, LOG_MAIN|LOG_PANIC_DIE,
-+ "Failed writing transport result to pipe: can't handle buffers > 99999 bytes. truncating!\n");
-+ size = 99999;
-+}
-+
-+/* to keep the write() atomic we build header in writebuffer and copy buf behind */
-+/* two write() calls would increase the complexity of reading from pipe */
-+
-+/* convert size to human readable string prepended by id and subid */
-+header_length = snprintf(writebuffer, PIPE_HEADER_SIZE+1, "%c%c%05d", id, subid, size);
-+if (header_length != PIPE_HEADER_SIZE)
-+{
-+ log_write(0, LOG_MAIN|LOG_PANIC_DIE, "header snprintf failed\n");
-+ writebuffer[0] = '\0';
-+}
-+
-+DEBUG(D_deliver) debug_printf("header write id:%c,subid:%c,size:%d,final:%s\n",
-+ id, subid, size, writebuffer);
-+
-+if (buf && size > 0)
-+ memcpy(writebuffer + PIPE_HEADER_SIZE, buf, size);
-+
-+size += PIPE_HEADER_SIZE;
-+int ret = write(fd, writebuffer, size);
- if(ret != size)
- log_write(0, LOG_MAIN|LOG_PANIC_DIE, "Failed writing transport result to pipe: %s\n",
- ret == -1 ? strerror(errno) : "short write");
-@@ -4100,8 +4183,8 @@ for (delivery_count = 0; addr_remote !=
- for (h = addr->host_list; h != NULL; h = h->next)
- {
- if (h->address == NULL || h->status < hstatus_unusable) continue;
-- sprintf(CS big_buffer, "H%c%c%s", h->status, h->why, h->address);
-- rmt_dlv_checked_write(fd, big_buffer, Ustrlen(big_buffer+3) + 4);
-+ sprintf(CS big_buffer, "%c%c%s", h->status, h->why, h->address);
-+ rmt_dlv_checked_write(fd, 'H', '0', big_buffer, Ustrlen(big_buffer+2) + 3);
- }
-
- /* The number of bytes written. This is the same for each address. Even
-@@ -4109,9 +4192,8 @@ for (delivery_count = 0; addr_remote !=
- size of each one is the same, and it's that value we have got because
- transport_count gets reset before calling transport_write_message(). */
-
-- big_buffer[0] = 'S';
-- memcpy(big_buffer+1, &transport_count, sizeof(transport_count));
-- rmt_dlv_checked_write(fd, big_buffer, sizeof(transport_count) + 1);
-+ memcpy(big_buffer, &transport_count, sizeof(transport_count));
-+ rmt_dlv_checked_write(fd, 'S', '0', big_buffer, sizeof(transport_count));
-
- /* Information about what happened to each address. Four item types are
- used: an optional 'X' item first, for TLS information, then an optional "C"
-@@ -4131,7 +4213,7 @@ for (delivery_count = 0; addr_remote !=
- if (addr->cipher)
- {
- ptr = big_buffer;
-- sprintf(CS ptr, "X1%.128s", addr->cipher);
-+ sprintf(CS ptr, "%.128s", addr->cipher);
- while(*ptr++);
- if (!addr->peerdn)
- *ptr++ = 0;
-@@ -4141,35 +4223,33 @@ for (delivery_count = 0; addr_remote !=
- while(*ptr++);
- }
-
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'X', '1', big_buffer, ptr - big_buffer);
- }
- if (addr->peercert)
- {
- ptr = big_buffer;
-- *ptr++ = 'X'; *ptr++ = '2';
- if (!tls_export_cert(ptr, big_buffer_size-2, addr->peercert))
- while(*ptr++);
- else
- *ptr++ = 0;
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'X', '2', big_buffer, ptr - big_buffer);
- }
- if (addr->ourcert)
- {
- ptr = big_buffer;
-- *ptr++ = 'X'; *ptr++ = '3';
- if (!tls_export_cert(ptr, big_buffer_size-2, addr->ourcert))
- while(*ptr++);
- else
- *ptr++ = 0;
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'X', '3', big_buffer, ptr - big_buffer);
- }
- #ifndef DISABLE_OCSP
- if (addr->ocsp > OCSP_NOT_REQ)
- {
- ptr = big_buffer;
-- sprintf(CS ptr, "X4%c", addr->ocsp + '0');
-+ sprintf(CS ptr, "%c", addr->ocsp + '0');
- while(*ptr++);
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'X', '4', big_buffer, ptr - big_buffer);
- }
- # endif
- #endif /*SUPPORT_TLS*/
-@@ -4177,34 +4257,33 @@ for (delivery_count = 0; addr_remote !=
- if (client_authenticator)
- {
- ptr = big_buffer;
-- sprintf(CS big_buffer, "C1%.64s", client_authenticator);
-+ sprintf(CS big_buffer, "%.64s", client_authenticator);
- while(*ptr++);
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'C', '1', big_buffer, ptr - big_buffer);
- }
- if (client_authenticated_id)
- {
- ptr = big_buffer;
-- sprintf(CS big_buffer, "C2%.64s", client_authenticated_id);
-+ sprintf(CS big_buffer, "%.64s", client_authenticated_id);
- while(*ptr++);
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'C', '2', big_buffer, ptr - big_buffer);
- }
- if (client_authenticated_sender)
- {
- ptr = big_buffer;
-- sprintf(CS big_buffer, "C3%.64s", client_authenticated_sender);
-+ sprintf(CS big_buffer, "%.64s", client_authenticated_sender);
- while(*ptr++);
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'C', '3', big_buffer, ptr - big_buffer);
- }
-
- #ifndef DISABLE_PRDR
- if (addr->flags & af_prdr_used)
-- rmt_dlv_checked_write(fd, "P", 1);
-+ rmt_dlv_checked_write(fd, 'P', '0', NULL, 0);
- #endif
-
- #ifdef EXPERIMENTAL_DSN
-- big_buffer[0] = 'D';
-- memcpy(big_buffer+1, &addr->dsn_aware, sizeof(addr->dsn_aware));
-- rmt_dlv_checked_write(fd, big_buffer, sizeof(addr->dsn_aware) + 1);
-+ memcpy(big_buffer, &addr->dsn_aware, sizeof(addr->dsn_aware));
-+ rmt_dlv_checked_write(fd, 'D', '0', big_buffer, sizeof(addr->dsn_aware));
- DEBUG(D_deliver) debug_printf("DSN write: addr->dsn_aware = %d\n", addr->dsn_aware);
- #endif
-
-@@ -4213,7 +4292,7 @@ for (delivery_count = 0; addr_remote !=
- for (r = addr->retries; r != NULL; r = r->next)
- {
- uschar *ptr;
-- sprintf(CS big_buffer, "R%c%.500s", r->flags, r->key);
-+ sprintf(CS big_buffer, "%c%.500s", r->flags, r->key);
- ptr = big_buffer + Ustrlen(big_buffer+2) + 3;
- memcpy(ptr, &(r->basic_errno), sizeof(r->basic_errno));
- ptr += sizeof(r->basic_errno);
-@@ -4224,13 +4303,13 @@ for (delivery_count = 0; addr_remote !=
- sprintf(CS ptr, "%.512s", r->message);
- while(*ptr++);
- }
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'R', '0', big_buffer, ptr - big_buffer);
- }
-
- /* The rest of the information goes in an 'A' item. */
-
-- ptr = big_buffer + 3;
-- sprintf(CS big_buffer, "A%c%c", addr->transport_return,
-+ ptr = big_buffer + 2;
-+ sprintf(CS big_buffer, "%c%c", addr->transport_return,
- addr->special_action);
- memcpy(ptr, &(addr->basic_errno), sizeof(addr->basic_errno));
- ptr += sizeof(addr->basic_errno);
-@@ -4265,7 +4344,7 @@ for (delivery_count = 0; addr_remote !=
- : addr->host_used->dnssec==DS_NO ? '1' : '0';
-
- }
-- rmt_dlv_checked_write(fd, big_buffer, ptr - big_buffer);
-+ rmt_dlv_checked_write(fd, 'A', '0', big_buffer, ptr - big_buffer);
- }
-
- /* Add termination flag, close the pipe, and that's it. The character
-@@ -4273,9 +4352,8 @@ for (delivery_count = 0; addr_remote !=
- A change from non-NULL to NULL indicates a problem with a continuing
- connection. */
-
-- big_buffer[0] = 'Z';
-- big_buffer[1] = (continue_transport == NULL)? '0' : '1';
-- rmt_dlv_checked_write(fd, big_buffer, 2);
-+ big_buffer[0] = (continue_transport == NULL)? '0' : '1';
-+ rmt_dlv_checked_write(fd, 'Z', '0', big_buffer, 1);
- (void)close(fd);
- exit(EXIT_SUCCESS);
- }
---- a/src/macros.h
-+++ b/src/macros.h
-@@ -156,6 +156,10 @@ as long as the maximum path length. */
- #define BIG_BUFFER_SIZE 16384
- #endif
-
-+/* header size of pipe content
-+ currently: char id, char subid, char[5] length */
-+#define PIPE_HEADER_SIZE 7
-+
- /* This limits the length of data returned by local_scan(). Because it is
- written on the spool, it gets read into big_buffer. */
-
HCoop / hcoop / debian / exim4.git / blobdiff