Move duplicate checking till after routing. Should fix several subtle

bugs.

2 files changed, 102 insertions, 2 deletions

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index cb661b943..758317801 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -1,4 +1,4 @@
-$Cambridge: exim/doc/doc-txt/ChangeLog,v 1.122 2005/04/07 10:54:54 ph10 Exp $
+$Cambridge: exim/doc/doc-txt/ChangeLog,v 1.123 2005/04/07 15:37:13 ph10 Exp $
 
 Change log file for Exim from version 4.21
 -------------------------------------------
@@ -212,6 +212,24 @@ PH/35 ACL_WHERE_MIME is now declared unconditionally, to avoid too much code
 
 PH/36 The change PH/12 above was broken. Fixed it.
 
+PH/37 Exim used to check for duplicate addresses in the middle of routing, on
+      the grounds that routing the same address twice would always produce the
+      same answer. This might have been true once, but it is certainly no
+      longer true now. Routing a child address may depend on the previous
+      routing that produced that child. Some complicated redirection strategies
+      went wrong when messages had multiple recipients, and made Exim's
+      behaviour dependent on the order in which the addresses were given.
+
+      I have moved the duplicate checking until after the routing is complete.
+      Exim scans the addresses that are assigned to local and remote
+      transports, and removes any duplicates. This means that more work will be
+      done, as duplicates will always all be routed, but duplicates are
+      presumably rare, so I don't expect this is of any significance.
+
+      For deliveries to pipes, files, and autoreplies, the duplicate checking
+      still happens during the routing process, since they are not going to be
+      routed further.
+
 
 A note about Exim versions 4.44 and 4.50
 ----------------------------------------
diff --git a/src/src/deliver.c b/src/src/deliver.c
index ac23a33aa..d610b38ca 100644
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -1,4 +1,4 @@
-/* $Cambridge: exim/src/src/deliver.c,v 1.11 2005/04/06 14:40:24 ph10 Exp $ */
+/* $Cambridge: exim/src/src/deliver.c,v 1.12 2005/04/07 15:37:14 ph10 Exp $ */
 
 /*************************************************
 *     Exim - an Internet mail transport agent    *
@@ -4274,6 +4274,58 @@ else
 
 
 
+/*************************************************
+*     Check list of addresses for duplication    *
+*************************************************/
+
+/* This function was introduced when the test for duplicate addresses that are
+not pipes, files, or autoreplies was moved from the middle of routing to when
+routing was complete. That was to fix obscure cases when the routing history
+affects the subsequent routing of identical addresses. If that change has to be
+reversed, this function is no longer needed. For a while, the old code that was
+affected by this change is commented with !!!OLD-DE-DUP!!! so it can be found
+easily.
+
+This function is called after routing, to check that the final routed addresses
+are not duplicates. If we detect a duplicate, we remember what it is a
+duplicate of. Note that pipe, file, and autoreply de-duplication is handled
+during routing, so we must leave such "addresses" alone here, as otherwise they
+will incorrectly be discarded.
+
+Argument:     address of list anchor
+Returns:      nothing
+*/
+
+static void
+do_duplicate_check(address_item **anchor)
+{
+address_item *addr;
+while ((addr = *anchor) != NULL)
+  {
+  tree_node *tnode;
+  if (testflag(addr, af_pfr))
+    {
+    anchor = &(addr->next);
+    }
+  else if ((tnode = tree_search(tree_duplicates, addr->unique)) != NULL)
+    {
+    DEBUG(D_deliver|D_route)
+      debug_printf("%s is a duplicate address: discarded\n", addr->unique);
+    *anchor = addr->next;
+    addr->dupof = tnode->data.ptr;
+    addr->next = addr_duplicate;
+    addr_duplicate = addr;
+    }
+  else
+    {
+    tree_add_duplicate(addr->unique, addr);
+    anchor = &(addr->next);
+    }
+  }
+}
+
+
+
 
 /*************************************************
 *              Deliver one message               *
@@ -5290,6 +5342,18 @@ while (addr_new != NULL)           /* Loop until all addresses dealt with */
       continue;
       }
 
+
+    /* !!!OLD-DE-DUP!!!  We used to test for duplicates at this point, in order
+    to save effort on routing duplicate addresses. However, facilities have
+    been added to Exim so that now two identical addresses that are children of
+    other addresses may be routed differently as a result of their previous
+    routing history. For example, different redirect routers may have given
+    them different redirect_router values, but there are other cases too.
+    Therefore, tests for duplicates now take place when routing is complete.
+    This is the old code, kept for a while for the record, and in case this
+    radical change has to be backed out for some reason.
+
+    #ifdef NEVER
     /* If it's a duplicate, remember what it's a duplicate of */
 
     if ((tnode = tree_search(tree_duplicates, addr->unique)) != NULL)
@@ -5305,6 +5369,9 @@ while (addr_new != NULL)           /* Loop until all addresses dealt with */
     /* Record this address, so subsequent duplicates get picked up. */
 
     tree_add_duplicate(addr->unique, addr);
+    #endif
+
+
 
     /* Get the routing retry status, saving the two retry keys (with and
     without the local part) for subsequent use. Ignore retry records that
@@ -5617,6 +5684,21 @@ Ensure they are not set in transports. */
 local_user_gid = (gid_t)(-1);
 local_user_uid = (uid_t)(-1);
 
+
+/* !!!OLD-DE-DUP!!! The next two statement were introduced when checking for
+duplicates was moved from within routing to afterwards. If that change has to
+be backed out, they should be removed. */
+
+/* Check for any duplicate addresses. This check is delayed until after
+routing, because the flexibility of the routing configuration means that
+identical addresses with different parentage may end up being redirected to
+different addresses. Checking for duplicates too early (as we previously used
+to) makes this kind of thing not work. */
+
+do_duplicate_check(&addr_local);
+do_duplicate_check(&addr_remote);
+
+
 /* When acting as an MUA wrapper, we proceed only if all addresses route to a
 remote transport. The check that they all end up in one transaction happens in
 the do_remote_deliveries() function. */