Re: "uridnsbl: bogus rr" run ...

[Justin Mason <jm...@jmason.org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


Sidney Markowitz writes:
> Matt Sergeant wrote:
> > I didn't think you could do that because in newer versions of Net::DNS
> > the id is a lexical variable. The only way to reinitialise it is to
> > reload the module.
> 
> If I remember it correctly, Justin's code keeps its own counter and sets the
> packet ID after creating the packet, making it independent of Net:DNS's counter.

Yep, our code just uses its own counter, and avoids using Net::DNS'
counter code as much as possible.

> I guess that would break down if there are any uses of Net::DNS by the same
> process that do not go through his code. If that is what is happening and it
> results in ID collision, the fix would be to use code like yours to reload
> the module and rely on its own counter. I'll try that now that I can
> reproduce the problem myself (painful as it is).

That should not be a problem, as long as Net::DNS uses a different part
of the ID space and increments separately:

  Net::DNS:   123456789...
  our code:                                             123456789...

I have a hard time figuring out how Net::DNS could be using the same
counter value, since it just uses rand() to seed it and counts
consecutively from there by default -- plus there's very few places
we still use Net::DNS::Resolver::search() or bgsend().

I think collisions with Net::DNS are a red herring.

- --j.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Exmh CVS

iD8DBQFCbpe8MJF5cimLx9ARArW6AJ9cla2VSn6TKzomdMrBptXhdUPl/wCeLmJi
qmt5sNEb2NKbvIuV+H7NlR8=
=nJpD
-----END PGP SIGNATURE-----

Re: "uridnsbl: bogus rr" run ...

[John Gardiner Myers <jg...@proofpoint.com>]

If a new socket is created for each message, you might find the port 
wrapping, leading to collisions.

Re: "uridnsbl: bogus rr" run ...

[Sidney Markowitz <si...@sidney.com>]

Sidney Markowitz wrote:
> I think it would be better to create the new socket with each message. If
> old replies are arriving as they seem to, wouldn't it be more efficient to
> not have a listener on the socket when they arrive?

I got confused when I reread this, so I thought I should clarify it.

If the socket is not changed with each message, then a process sends out
queries on a port with one message, then continues to send out queries on
the same port in subsequent messages. The ID is incremented and the socket
is changed when the ID wraps, so there are no collisions.

However, packets still arrive in reply to queries sent for old messages.
Until the ID wraps, the replies are received, the ID is not found in the
pending list, and the packet is discarded.

There is no collision problem, but processing may take a lot longer than if
a new socket is created for each message causing the old replies to find no
listener on their port.

 -- sidney

Re: "uridnsbl: bogus rr" run ...

[Sidney Markowitz <si...@sidney.com>]

I'm going to respond to yours and John Gardiner Myers replies in the bug
4260 discussion to keep everything tracked there now that I've re-opened the
bug.

 -- sidney

Re: "uridnsbl: bogus rr" run ...

[Theo Van Dinter <fe...@kluge.net>]

On Wed, Apr 27, 2005 at 10:12:39AM +1200, Sidney Markowitz wrote:
> A fix would be to close and reopen the socket at each message, or as you
> suggested when the counter wraps. But it should not be when the counter
> wraps to zero, it should be when it wraps to its initial value.

Yeah.  But then we have to store the initial value, etc, etc.  By wrapping
at 0, we get basically the same functionality, but it occurs more
frequently than it has to.  It's "randomly" in between new socket per
message and 1 socket per child.

> I think it would be better to create the new socket with each message. If
> old replies are arriving as they seem to, wouldn't it be more efficient to
> not have a listener on the socket when they arrive?

True, but 1) that's more overhead, 2) in theory the child (or a different
child) could get the same socket back after processing a single message
(depends how the OS assigns out new sockets).

I like the idea of only getting a new socket when necessary (or
thereabouts), which leaves the issue still possible but very unlikely.

-- 
Randomly Generated Tagline:
My favorite mythical creature?  The honest politician.

Re: "uridnsbl: bogus rr" run ...

[Sidney Markowitz <si...@sidney.com>]

Theo Van Dinter wrote:
> I'm trying a small patch which basically calls the reinit function when
> the counter wraps to 0, as well as using rand when initializing.  This way
> it'll get a new random starting point and a new socket occasionally.

I think I understand the problem now. It's similar to what you said. I
noticed when debugging t/dnsbl.t that the one message in it generates 52 DNS
queries. When there are tens of thousands of messages in a mass check and
-j=4, there are going to be several wraparounds of the 16 bit ID. Apparently
nameserver responses can arrive quite late.

My guess about the slowdown you saw when using the sha1 patch is that while
it avoided errors from collisions, all the old reply packets were still read
and hashed before being discarded.

A fix would be to close and reopen the socket at each message, or as you
suggested when the counter wraps. But it should not be when the counter
wraps to zero, it should be when it wraps to its initial value.

I think it would be better to create the new socket with each message. If
old replies are arriving as they seem to, wouldn't it be more efficient to
not have a listener on the socket when they arrive?

 -- sidney

Re: "uridnsbl: bogus rr" run ...

[Theo Van Dinter <fe...@kluge.net>]

On Tue, Apr 26, 2005 at 12:34:20PM -0700, Justin Mason wrote:
> I have a hard time figuring out how Net::DNS could be using the same
> counter value, since it just uses rand() to seed it and counts
> consecutively from there by default -- plus there's very few places
> we still use Net::DNS::Resolver::search() or bgsend().

Unless I missed something, no it doesn't.  It sets the initial counter
based on pid (no rand), then increments each time.

$DNS_ID_COUNTER = (($$ >> 10) ^ (($$ << 6) & 0xffff));

My only theory right now is that there are problems once the counter wraps
around the full 16-bits.  The current code may work fine in spamd, whose
children don't tend to have a huge lifespan.  However, in mass-check,
the children (unless --restart is used) live for the entirety of the
run so looping is much more likely.

I'm trying a small patch which basically calls the reinit function when
the counter wraps to 0, as well as using rand when initializing.  This way
it'll get a new random starting point and a new socket occasionally.

-- 
Randomly Generated Tagline:
Wait a minute, Marge.  I saw "Mrs. Doubtfire."  This is a man in drag!
 
 		-- Homer Simpson
 		   Simpsoncalifragilisticexpiala(annoyed grunt)ocious