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.

2 files changed, 18 insertions, 26 deletions

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