that's not too hot.  it kills further scanning of *other* messages, or just that
one message?

if the former, it should be in an eval { } block anyway.

aiming at 3.0.0

> it kills further scanning of *other* messages, or just that 
> one message? 
 
In case of amavisd-new, it kills just that one message, which stays in the MTA 
queue (after a 4xx temporary failure) and delivery is re-attempted every so 
often for a couple of days, leaving a pile of intentionally preserved temporary 
files behind. 
 
> it should be in an eval { } block anyway. 
 
Perhaps, although I think a cleaner solution would be two-part: 
 
- don't assemble illegal DNS queries in the first place 
  (in this particular example one 'node label' in DNS parlance 
  exceeded 63 characters, and the complete query should 
  not be too long - rfc1034 mentions 255) 
 
- when analyzing the Net::DNS::Packet in process_dnsbl_result, 
  allow for its fields to be empty (the $packet->question in this case), 
  i.e. ignore the result, if does not appear valid. 
 
I think an eval {} could be used an additional safety net, 
but the known/expected cases should be handled explicitly. 
 
  Mark 
 

Subject: Re:  Dns.pm: Can't call method "qname" on an undefined value

On Mon, Sep 20, 2004 at 11:31:13AM -0700, bugzilla-daemon@bugzilla.spamassassin.org wrote:
> I think an eval {} could be used an additional safety net, 
> but the known/expected cases should be handled explicitly. 

FYI: We really can't do much about this issue without having a message
attached (not cut/paste) to the ticket for debugging purposes.

I agree we should simply do the right thing instead of kluging around the
issue though (I think we use eval{} too much already).

> We really can't do much about this issue without having a message 
> attached (not cut/paste) to the ticket for debugging purposes. 
 
Sure, attached, also: http://www.ijs.si/people/mark/trouble-sa-2.txt 
 
The message is a capture (postcat) directly from the MTA queue, untouched. 
Warning: may contain a virus. 

Subject: Re:  Dns.pm: Can't call method "qname" on an undefined value

What version of Net::DNS are you running?

I'm not able to recreate by checking that message with spamassassin on
the command line.

Michael

Subject: Re:  Dns.pm: Can't call method "qname" on an undefined value

| I'm not able to recreate by checking that message with spamassassin on
| the command line.

Neither can I, for unknown reason.
(but I didn't investigate, as the broken DNS query
is clearly there if I debug the Dns.pm code).

perl -le 'use Net::DNS; print Net::DNS->version'
0.48

  Mark

I also can't reproduce the issue.  Recommend pushing to 3.0.1 queue for further debugging.

Created attachment 2354 [details]
proposed patch

add a bunch of defined() tests to catch some possible undefined values

As I was saying in IRC: IMO, we need to use eval { } with Net::DNS.

1. Net::DNS contains several explicit calls to die()

/usr/lib/perl5/Net/DNS/Nameserver.pm:   $sock->send($reply_data) or die "send: $!";
/usr/lib/perl5/Net/DNS/Nameserver.pm: ) || die "couldn't create nameserver
object\n";
/usr/lib/perl5/Net/DNS/Packet.pm:       #       die "dn_expand: loop:
offset=$offset (seen = ",
/usr/lib/perl5/Net/DNS/RR.pm:                 die $@;
/usr/lib/perl5/Net/DNS/RR.pm:           die "$rdata is inconsistent; length does
not match content"
/usr/lib/perl5/Net/DNS/RR.pm:           die 'Expected RFC3597 representation of
RDATA'
/usr/lib/perl5/Net/DNS/RR.pm:           die "$rdata is inconsistent; length does
not match content"

etc.

2. we don't currently have an eval{ } trap in the harvest_dnsbl_results() call
path from user code, so any die()s are propagated into the user code

3. Net::DNS' design is such that new objects are created, returned to us, and we
then are expected to call methods on those to get results.  This means that
(similar to this bug) there are many situations where we might try to call a
method that doesn't exist by that name on the object we've been given; this
results in a die() from perl itself.  The doco doesn't mention this case, but
then, according to the N:D documentation, this current bug should not exist
either, but yet it does. ;)

As far as I can see by a cursory examination, the Net::DNS object model
shouldn't screw up too much aside from this case, but I don't know for sure --
and that's subject to change in future N:D releases. for defensive-programming's
sake, we have to expect that we can receive die() exceptions from Net::DNS in
future.

+1

patch looks good for fixing for 3.0.0.   to reiterate: for 3.0.1, we need to add
use of eval { } at the harvest_dnsbl_results() level, to catch all possible
other Net::DNS screwups as well.

+1

the patch is a bit larger than I expected, but I think it's ok

+1

Subject: Re:  Dns.pm: Can't call method "qname" on an undefined value

Just to clarify why the supplied sample mail does not cause
'spamassassin -t' to die. Prepend a 'Return-Path: <test@example.com>'
to it, and the spamassassin will die just as well.

The difference is in the process_dnsbl_result where an empty
$self->{sender_host}, (i.e. envelope sender address) causes
further checks on DNS query result (on From address) to be skipped
(is this expected behaviour?).

In statement:
  my $question = ($packet->question)[0];

the ($packet->question) is an empty list, which is not against
Net::DNS::Packet specifications. There may be 0, 1, or any other
number of items in the list, at least theoretically.

  Mark

Okay, checked in the fix with one minor change and I removed some of the
more complicated parts of the patch which I don't think are as necessary
and frankly make me nervous.

The minor change was adding a defined() check for $packet->header which is
used later.  I also removed the sections of the patch that check defined
values for non-objects, I don't think those are quite as critical so I wanted
to keep this as simple as possible.

Created attachment 2355 [details]
proposed patch

patch for branch, please approve

2355: +1

+1

committed, moving to 3.1 for further changes

ok -- r51854 is a checkin of an eval { } block around the "hot spot" loop in
harvest_dnsbl_results() for 3.1.0.  that should catch any further wierd stuff
thrown by Net::DNS in this case.

if it looks good enough, feel free to close this bug...

> if it looks good enough, feel free to close this bug... 
 
As far as I am concerned, this can be closed. Thanks!