Re: [exim] Re: Potential logic error in retry handling for I…

Top Page
Delete this message
Reply to this message
Author: Florian Weimer
Date:  
To: Andreas Metzler
CC: exim-users
Subject: Re: [exim] Re: Potential logic error in retry handling for IPv4+IPv6 hosts
* Andreas Metzler:

> Philip Hazel <ph10@???> wrote:
> [...]
>> I have not looked at the source, but the patch should be trivial; just
>> delete (or skip) the code that processes the additional section.
> [...]
>
> I see. I think this should do the trick:
> ---------------
> --- exim-4.60/src/host.c.orig    2006-01-01 15:15:21.614492448 +0100
> +++ exim-4.60/src/host.c    2006-01-01 14:08:54.374645040 +0100
> @@ -2882,7 +2882,8 @@
>  items (which is typically only 3 or 4 long anyway.) Add extra host items for
>  multi-homed hosts. */

>
> -for (rr = dns_next_rr(&dnsa, &dnss, RESET_ADDITIONAL);
> +/*for (rr = dns_next_rr(&dnsa, &dnss, RESET_ADDITIONAL);*/
> +for (rr = dns_next_rr(&dnsa, &dnss, RESET_NEXT);
>       rr != NULL;
>       rr = dns_next_rr(&dnsa, &dnss, RESET_NEXT))
>    {
> ---------------


I think that due to an exist condition on a previous loop, this change
is equivalent to a "while (0) { ..." loop, i.e. the loop body is never
executed. As far as the subsequent code is concerned, this has the
same effect as an empty additional section in the DNS reply.

Consequently, the patch below completely removes the offending loop,
and updates the comment. I have verified that Exim performs the
expected DNS lookups.

#! /bin/sh /usr/share/dpatch/dpatch-run
## 37_dns_disable_additional_section.dpatch by <fw@???>
##
## DP: Disable AD section processing, so that A/AAAA RRs cannot
## DP: mask each other.

@DPATCH@
diff -urNad trunk~/src/host.c trunk/src/host.c
--- trunk~/src/host.c    2005-11-28 11:57:32.000000000 +0100
+++ trunk/src/host.c    2006-01-05 19:51:40.000000000 +0100
@@ -2874,162 +2874,19 @@
     }   /* Move on to the next host */
   }


-/* Now we have to ensure addresses exist for all the hosts. We have ensured
-above that the names in the host items are all unique. The addresses may have
-been returned in the additional data section of the DNS query. Because it is
-more expensive to scan the returned DNS records (because you have to expand the
-names) we do a single scan over them, and multiple scans of the chain of host
-items (which is typically only 3 or 4 long anyway.) Add extra host items for
-multi-homed hosts. */
-
-for (rr = dns_next_rr(&dnsa, &dnss, RESET_ADDITIONAL);
-     rr != NULL;
-     rr = dns_next_rr(&dnsa, &dnss, RESET_NEXT))
-  {
-  dns_address *da;
-  int status = hstatus_unknown;
-  int why = hwhy_unknown;
-  int randoffset;
-
-  if (rr->type != T_A
-  #if HAVE_IPV6
-    && rr->type != T_AAAA
-    #ifdef SUPPORT_A6
-    && rr->type != T_A6
-    #endif
-  #endif
-    ) continue;
-
-  /* Find the first host that matches this record's name. If there isn't
-  one, move on to the next RR. */
-
-  for (h = host; h != last->next; h = h->next)
-    { if (strcmpic(h->name, rr->name) == 0) break; }
-  if (h == last->next) continue;
-
-  /* For IPv4 addresses, add 500 to the random part of the sort key, to ensure
-  they sort after IPv6 addresses. */
-
-  randoffset = (rr->type == T_A)? 500 : 0;
-
-  /* Get the list of textual addresses for this RR. There may be more than one
-  if it is an A6 RR. Then loop to handle multiple addresses from an A6 record.
-  If there are none, nothing will get done - the record is ignored. */
-
-  for (da = dns_address_from_rr(&dnsa, rr); da != NULL; da = da->next)
-    {
-    /* Set status for an ignorable host. */
-
-    #ifndef STAND_ALONE
-    if (ignore_target_hosts != NULL &&
-          verify_check_this_host(&ignore_target_hosts, NULL, h->name,
-            da->address, NULL) == OK)
-      {
-      DEBUG(D_host_lookup)
-        debug_printf("ignored host %s [%s]\n", h->name, da->address);
-      status = hstatus_unusable;
-      why = hwhy_ignored;
-      }
-    #endif
-
-    /* If the address is already set for this host, it may be that
-    we just have a duplicate DNS record. Alternatively, this may be
-    a multi-homed host. Search all items with the same host name
-    (they will all be together) and if this address is found, skip
-    to the next RR. */
-
-    if (h->address != NULL)
-      {
-      int new_sort_key;
-      host_item *thishostlast;
-      host_item *hh = h;
-
-      do
-        {
-        if (hh->address != NULL && Ustrcmp(CS da->address, hh->address) == 0)
-          goto DNS_NEXT_RR;         /* Need goto to escape from inner loop */
-        thishostlast = hh;
-        hh = hh->next;
-        }
-      while (hh != last->next && strcmpic(hh->name, rr->name) == 0);
-
-      /* We have a multi-homed host, since we have a new address for
-      an existing name. Create a copy of the current item, and give it
-      the new address. RRs can be in arbitrary order, but one is supposed
-      to randomize the addresses of multi-homed hosts, so compute a new
-      sorting key and do that. [Latest SMTP RFC says not to randomize multi-
-      homed hosts, but to rely on the resolver. I'm not happy about that -
-      caching in the resolver will not rotate as often as the name server
-      does.] */
-
-      new_sort_key = h->mx * 1000 + random_number(500) + randoffset;
-      hh = store_get(sizeof(host_item));
-
-      /* New address goes first: insert the new block after the first one
-      (so as not to disturb the original pointer) but put the new address
-      in the original block. */
-
-      if (new_sort_key < h->sort_key)
-        {
-        *hh = *h;                       /* Note: copies the port */
-        h->next = hh;
-        h->address = da->address;
-        h->sort_key = new_sort_key;
-        h->status = status;
-        h->why = why;
-        }
-
-      /* Otherwise scan down the addresses for this host to find the
-      one to insert after. */
-
-      else
-        {
-        while (h != thishostlast)
-          {
-          if (new_sort_key < h->next->sort_key) break;
-          h = h->next;
-          }
-        *hh = *h;                       /* Note: copies the port */
-        h->next = hh;
-        hh->address = da->address;
-        hh->sort_key = new_sort_key;
-        hh->status = status;
-        hh->why = why;
-        }
-
-      if (h == last) last = hh;         /* Inserted after last */
-      }
-
-    /* The existing item doesn't have its address set yet, so just set it.
-    Ensure that an IPv4 address gets its sort key incremented in case an IPv6
-    address is found later. */
-
-    else
-      {
-      h->address = da->address;         /* Port should be set already */
-      h->status = status;
-      h->why = why;
-      h->sort_key += randoffset;
-      }
-    }    /* Loop for addresses extracted from one RR */
-
-  /* Carry on to the next RR. It would be nice to be able to be able to stop
-  when every host on the list has an address, but we can't be sure there won't
-  be an additional address for a multi-homed host further down the list, so
-  we have to continue to the end. */
-
-  DNS_NEXT_RR: continue;
-  }
-
 /* Set the default yield to failure */


yield = HOST_FIND_FAILED;

-/* If we haven't found all the addresses in the additional section, we
-need to search for A or AAAA records explicitly. The names shouldn't point to
+/* We search for A or AAAA records explicitly. The names shouldn't point to
CNAMES, but we use the general lookup function that handles them, just
in case. If any lookup gives a soft error, change the default yield.

+We used to process records from the additional section in the DNS
+packet. However, the DNS resolver is free to drop any resource
+records from the additional section, and compensating for that is not
+worth the complexity.
+
For these DNS lookups, we must disable qualify_single and search_parents;
otherwise invalid host names obtained from MX or SRV records can cause trouble
if they happen to match something local. */