From d1f9fb42472323edb17c3ee3cbbfce3557083ceb Mon Sep 17 00:00:00 2001
From: Jeremy Harris <jgh146exb@wizmail.org>
Date: Thu, 22 Sep 2016 19:59:48 +0100
Subject: Routing: avoid doing the one_time replacement operation when a
 redirect leaves the address unchanged

When done, in combination with a defer the retry would see the address as delivered, hence losing mail.
---
 doc/doc-txt/ChangeLog      |  5 +++++
 src/src/deliver.c          | 27 ++++++++++++---------------
 src/src/routers/redirect.c | 17 ++++++-----------
 3 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index f2000b5d4..b920d92cc 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -94,6 +94,11 @@ JH/25 Decoding ACL controls is now done using a binary search; the sourcecode
       takes up less space and should be simpler to maintain.  Merge the ACL
       condition decode tables also, with similar effect.
 
+JH/26 Fix problem with one_time used on a redirect router which returned the
+      parent address unchanged.  A retry would see the parent address marked as
+      delivered, so not attempt the (identical) child.  As a result mail would
+      be lost.
+
 
 Exim version 4.87
 -----------------
diff --git a/src/src/deliver.c b/src/src/deliver.c
index dc616a1db..72405da39 100644
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -7867,10 +7867,12 @@ else if (addr_defer != (address_item *)(+1))
         }
 
       /* Didn't find the address already in the list, and did find the
-      ultimate parent's address in the list. After adding the recipient,
+      ultimate parent's address in the list, and they really are different
+      (i.e. not from an identity-redirect). After adding the recipient,
       update the errors address in the recipients list. */
 
-      if (i >= recipients_count && t < recipients_count)
+      if (  i >= recipients_count && t < recipients_count
+         && Ustrcmp(otaddr->address, otaddr->parent->address) != 0)
         {
         DEBUG(D_deliver) debug_printf("one_time: adding %s in place of %s\n",
           otaddr->address, otaddr->parent->address);
@@ -7885,19 +7887,14 @@ else if (addr_defer != (address_item *)(+1))
     this deferred address or, if there is none, the sender address, is on the
     list of recipients for a warning message. */
 
-    if (sender_address[0] != 0)
-      if (addr->prop.errors_address)
-        {
-        if (Ustrstr(recipients, addr->prop.errors_address) == NULL)
-          recipients = string_sprintf("%s%s%s", recipients,
-            (recipients[0] == 0)? "" : ",", addr->prop.errors_address);
-        }
-      else
-        {
-        if (Ustrstr(recipients, sender_address) == NULL)
-          recipients = string_sprintf("%s%s%s", recipients,
-            (recipients[0] == 0)? "" : ",", sender_address);
-        }
+    if (sender_address[0])
+      {
+      uschar * s = addr->prop.errors_address;
+      if (!s) s = sender_address;
+      if (Ustrstr(recipients, s) == NULL)
+	recipients = string_sprintf("%s%s%s", recipients,
+	  recipients[0] ? "," : "", s);
+      }
     }
 
   /* Send a warning message if the conditions are right. If the condition check
diff --git a/src/src/routers/redirect.c b/src/src/routers/redirect.c
index 2434d3cb7..8aad1d4ab 100644
--- a/src/src/routers/redirect.c
+++ b/src/src/routers/redirect.c
@@ -325,7 +325,7 @@ add_generated(router_instance *rblock, address_item **addr_new,
 redirect_router_options_block *ob =
   (redirect_router_options_block *)(rblock->options_block);
 
-while (generated != NULL)
+while (generated)
   {
   address_item *parent;
   address_item *next = generated;
@@ -347,7 +347,7 @@ while (generated != NULL)
 
   if (ob->one_time && !queue_2stage)
     {
-    for (parent = addr; parent->parent != NULL; parent = parent->parent);
+    for (parent = addr; parent->parent; parent = parent->parent) ;
     next->onetime_parent = parent->address;
     }
 
@@ -358,21 +358,16 @@ while (generated != NULL)
   unless the ancestor was routed by a case-sensitive router. */
 
   if (ob->check_ancestor)
-    {
-    for (parent = addr; parent != NULL; parent = parent->parent)
-      {
-      if (((parent->router != NULL && parent->router->caseful_local_part)?
-           Ustrcmp(next->address, parent->address)
-           :
-           strcmpic(next->address, parent->address)
+    for (parent = addr; parent; parent = parent->parent)
+      if ((parent->router && parent->router->caseful_local_part
+	   ? Ustrcmp(next->address, parent->address)
+           : strcmpic(next->address, parent->address)
           ) == 0)
         {
         DEBUG(D_route) debug_printf("generated parent replaced by child\n");
         next->address = string_copy(addr->address);
         break;
         }
-      }
-    }
 
   /* A user filter may, under some circumstances, set up an errors address.
   If so, we must take care to re-instate it when we copy in the propagated
-- 
cgit v1.2.3