Bug 4154 - is_dns_available fails to loop as designed
Summary: is_dns_available fails to loop as designed
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamassassin (show other bugs)
Version: 3.0.1
Hardware: Other All
: P2 major
Target Milestone: 3.1.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
: 3925 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-02-24 09:26 UTC by Jay Levitt
Modified: 2005-02-24 10:15 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Levitt 2005-02-24 09:26:30 UTC 
Dns.pm's is_dns_available checks a randomly picked well-known site to see if DNS
is working.  My caching-only nameserver times out when looking up NS records for
a site that's not in the cache.  Not entirely surprising, with a 3-second
timeout in SA.  And my site is infinitely small (just me), so it's going to be
fairly common that one of the well-known sites is not in cache.

is_dns_available realizes this, and tries to loop, but the loop is coded wrong,
because either a success or a failure breaks out of the loop!  A timeout in
lookup_ns will result in $result defined, but containing no records, and that
triggers the "failed horribly" clause, setting $IS_DNS_AVAILABLE to zero.  This,
of course, results in many missed spams that are listedin DNSBLs.

I *think* the bug fix is just to remove that whole else clause from
is_dns_available, but as a Perl novice I'd certainly like someone to
double-check that.

And, you know, now that I look at it, it seems like is_dns_available uses
lookup_ns to test general DNS availability, but lookup_ns has its own caching
that would seem to defeat the point of the test if a site is ever hit twice! 

I believe this also explains bugs 3626 and 3925, and quite possibly any number
of "why did this get through" complaints.
Comment 1 Theo Van Dinter 2005-02-24 10:15:37 UTC 
Subject: Re:   New: is_dns_available fails to loop as designed upon timeouts

On Thu, Feb 24, 2005 at 09:26:31AM -0800, bugzilla-daemon@bugzilla.spamassassin.org wrote:
> is_dns_available realizes this, and tries to loop, but the loop is coded wrong,
> because either a success or a failure breaks out of the loop!  A timeout in
> lookup_ns will result in $result defined, but containing no records, and that
> triggers the "failed horribly" clause, setting $IS_DNS_AVAILABLE to zero.  This,
> of course, results in many missed spams that are listedin DNSBLs.

Oh, I see what you're saying.  Basically, the loop in question:

  for(my $retry = 3; $retry > 0 and $#domains>-1; $retry--) {
    my $domain = splice(@domains, rand(@domains), 1);
    dbg ("trying ($retry) $domain...", "dnsavailable", -2);
[...]

tries to pick a random domain and try it.  but the section in question:

    if(defined $result && scalar @$result > 0) {
[...]
        $IS_DNS_AVAILABLE = 1;
        last;
[...]
    }     
    else {
      dbg ("NS lookup of $domain failed horribly => Perhaps your resolv.conf
isn't pointing at a valid server?", "dnsavailable", -1);
      $IS_DNS_AVAILABLE = 0; # should already be 0, but let's be sure.
      last; 
    } 

So "last" is called either way and we don't actually try 3 times, only once.

> And, you know, now that I look at it, it seems like is_dns_available uses
> lookup_ns to test general DNS availability, but lookup_ns has its own caching
> that would seem to defeat the point of the test if a site is ever hit twice! 

Judging by the code, the cache only exists for the lifetime of 1 message
check.  When rbl_finish() is called post-check, the cache is destroyed.
Comment 2 Theo Van Dinter 2005-02-24 10:17:31 UTC 
moving to 3.1 queue
Comment 3 Theo Van Dinter 2005-02-24 10:38:34 UTC 
ok, patch committed.  r155223

basically, lookup_ns will always return defined unless Net::DNS dies.  in that
case, the result is an empty array.
Comment 4 Jay Levitt 2005-02-24 17:45:40 UTC 
This patch has made me very happy.  Thanks!

One suggestion: You might want to include $self->{res}->errorstring in the dbg
message for the NS failure.
Comment 5 Fred T 2005-02-24 19:15:04 UTC 
*** Bug 3925 has been marked as a duplicate of this bug. ***