Skip Menu |

This queue is for tickets about the Net-DNS CPAN distribution.

Report information
The Basics
Id: 88778
Status: resolved
Priority: 0/
Queue: Net-DNS
People
Owner: Nobody in particular
Requestors: calle [...] init.se
Cc:
AdminCc:
Bug Information
Severity: Normal
Broken in: 0.72
Fixed in: (no value)
Attachments
0001-Suggested-fix-for-ticket-88778.patch
0001-Tests-to-go-with-the-fix-for-RT88778.patch
signature.asc
unique_push_bug.pl

History Show all quoted text
  Wed Sep 18 05:42:58 2013 calle [...] lysator.liu.se - Ticket created
Subject: Net::DNS::Packet::unique_push does not work as advertised
When using unique_push on packets received from the net, it will add records that are already there. Script that illustrates the problem included.
Subject: unique_push_bug.pl
#!/usr/bin/env perl use strict; use warnings; use Net::DNS; my $res = Net::DNS::Resolver->new; my $p = $res->send( 'www.net-dns.org' ); my $rr = Net::DNS::RR->new( 'www.net-dns.org. 100 IN A 213.154.224.135' ); $p->unique_push( answer => $rr ); $p->print;
  Wed Sep 18 06:36:59 2013 calle [...] lysator.liu.se - Correspondence added
Suggested patch (generated with "git format-patch"). Not tested with perl versions older than 5.10.1.
Subject: 0001-Suggested-fix-for-ticket-88778.patch
From 27c4b6071bbb79176568c72b39fd4e46dc836bf8 Mon Sep 17 00:00:00 2001 From: Calle Dybedahl <calle@init.se> Date: Wed, 18 Sep 2013 12:33:11 +0200 Subject: [PATCH] Suggested fix for ticket #88778 --- lib/Net/DNS/Packet.pm | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/Net/DNS/Packet.pm b/lib/Net/DNS/Packet.pm index 26c8fab..78f6a4c 100644 --- a/lib/Net/DNS/Packet.pm +++ b/lib/Net/DNS/Packet.pm @@ -161,6 +161,11 @@ sub decode { $self->print; } + foreach my $rr ( @{$self->{answer}}, @{$self->{authority}}, @{$self->{additional}} ) { + next if $rr->type eq 'OPT'; + $self->{seen}->{lc( $rr->name ) . $rr->class . $rr->type . $rr->rdatastr} += 1; + } + return wantarray ? ( $self, $offset ) : $self; } -- 1.8.4
  Wed Sep 18 06:37:00 2013 The RT System itself - Status changed from 'new' to 'open'
  Sat Sep 28 07:16:28 2013 rwfranks [...] acm.org - Correspondence added
From: rwfranks [...] acm.org
On Wed Sep 18 06:36:59 2013, CDYBED wrote: Show quoted text
> Suggested patch (generated with "git format-patch"). Not tested with > perl versions older than 5.10.1.
unique_push()is provided for assembling dynamic update requests, and has absolutely no impact on the decoding of inbound packets. Perhaps you should read our advertising message more carefully.
  Mon Sep 30 02:08:01 2013 calle [...] init.se - Correspondence added
Subject: Re: [rt.cpan.org #88778] Net::DNS::Packet::unique_push does not work as advertised
Date: Mon, 30 Sep 2013 08:07:47 +0200
To: bug-Net-DNS [...] rt.cpan.org
From: Calle Dybedahl <calle [...] init.se>
On 28 sep 2013, at 13:16, "Dick Franks via RT" <bug-Net-DNS@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=88778 > > > On Wed Sep 18 06:36:59 2013, CDYBED wrote:
>> Suggested patch (generated with "git format-patch"). Not tested with >> perl versions older than 5.10.1.
> > unique_push()is provided for assembling dynamic update requests, and has absolutely no impact on the decoding of inbound packets. > > Perhaps you should read our advertising message more carefully.
Am I to understand that you are refusing my patch on the grounds that I'm using the library for something other than what you intended? This is not just a spurious patch, I'm trying to fix an actual problem I'm having in real-life code. -- Calle Dybedahl calle@init.se -*- +46 703 - 970 612
  Mon Sep 30 08:36:19 2013 rwfranks [...] acm.org - Correspondence added
From: rwfranks [...] acm.org
On Mon Sep 30 02:08:01 2013, calle@init.se wrote: Show quoted text
> Am I to understand that you are refusing my patch on the grounds that > I'm using the library for something other than what you intended? This > is not just a spurious patch, I'm trying to fix an actual problem I'm > having in real-life code.
No, you can do what you wish with Net::DNS. But your patch modifies packet->decode(), which is the wrong place to do it. The semantic of each section is different, so uniqueness can never stretch across more than one section. The obvious failing case being: $update->push( pre => yxrrset('foo.example 600 A 192.0.2.1') ); $update->unique_push( upd => rr_add('foo.example 3600 A 192.0.2.1') ); However, your idea that unique_push() should take account of what is already there is not unreasonable. Proper solution is already on SVN trunk.
  Mon Sep 30 09:24:32 2013 calle [...] init.se - Correspondence added
Subject: Re: [rt.cpan.org #88778] Net::DNS::Packet::unique_push does not work as advertised
Date: Mon, 30 Sep 2013 15:24:11 +0200
To: bug-Net-DNS [...] rt.cpan.org
From: Calle Dybedahl <calle [...] init.se>
On 30 sep 2013, at 14:36, Dick Franks via RT <bug-Net-DNS@rt.cpan.org> wrote: Show quoted text
> But your patch modifies packet->decode(), which is the wrong place to do it. > > The semantic of each section is different, so uniqueness can never stretch across more than one section. The obvious failing case being: > > $update->push( pre => yxrrset('foo.example 600 A 192.0.2.1') ); > $update->unique_push( upd => rr_add('foo.example 3600 A 192.0.2.1') ); >
As I understood the code, that distinction wasn't made before, and I was going for the smallest necessary change. It's quite possible that I misunderstood. As for the functionality, you are of course quite right. Show quoted text
> However, your idea that unique_push() should take account of what is already there is not unreasonable. Proper solution is already on SVN trunk.
Yes, I see it now. Thank you. I don't see tests for it in the commit, so I'm trying to attach a small patch with some to this email. Not sure what the email gateway will do to it, but I'll guess we'll see. -- Calle Dybedahl calle@init.se -*- +46 703 - 970 612

Message body is not shown because sender requested not to inline it.

  Mon Sep 30 16:26:45 2013 rwfranks [...] acm.org - Correspondence added
From: rwfranks [...] acm.org
On Mon Sep 30 09:24:32 2013, calle@init.se wrote: Show quoted text
> Yes, I see it now. Thank you. I don't see tests for it in the commit, > so I'm trying to attach a small patch with some to this email.
Test coverage of existing test script is sufficient.
  Tue Oct 01 02:03:34 2013 calle [...] init.se - Correspondence added
Subject: Re: [rt.cpan.org #88778] Net::DNS::Packet::unique_push does not work as advertised
Date: Tue, 1 Oct 2013 08:03:12 +0200
To: bug-Net-DNS [...] rt.cpan.org
From: Calle Dybedahl <calle [...] init.se>
On 30 sep 2013, at 22:26, "Dick Franks via RT" <bug-Net-DNS@rt.cpan.org> wrote: Show quoted text
> Test coverage of existing test script is sufficient.
They give a pass to the code (0.72 release) where the bug is present. But it's your code, you decide. -- Calle Dybedahl calle@init.se -*- +46 703 - 970 612
Download signature.asc
application/pgp-signature 163b

Message body not shown because it is not plain text.

  Tue Oct 01 05:54:26 2013 olaf [...] NLnetLabs.nl - Correspondence added
Subject: Re: [rt.cpan.org #88778] Net::DNS::Packet::unique_push does not work as advertised
Date: Tue, 1 Oct 2013 11:53:51 +0200
To: "bug-Net-DNS [...] rt.cpan.org" <bug-Net-DNS [...] rt.cpan.org>
From: Olaf Kolkman <olaf [...] NLnetLabs.nl>
My €0.02 contribution: I believe that tests that pass when users have identified bugs do not provide sufficient coverage. I'm with Calle on this. --Olaf ---- Composed on a mobile device: more typos guaranteed. Show quoted text
> On 1 okt. 2013, at 08:03, "calle@init.se via RT" <bug-Net-DNS@rt.cpan.org> wrote: > > Queue: Net-DNS > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=88778 > > >
>> On 30 sep 2013, at 22:26, "Dick Franks via RT" <bug-Net-DNS@rt.cpan.org> wrote: >> >> Test coverage of existing test script is sufficient.
> > They give a pass to the code (0.72 release) where the bug is present. But it's your code, you decide. > > -- > Calle Dybedahl > calle@init.se -*- +46 703 - 970 612 > > > > > <signature.asc>
  Tue Oct 01 11:23:37 2013 rwfranks [...] acm.org - Correspondence added
From: rwfranks [...] acm.org
On Tue Oct 01 05:54:26 2013, olaf@NLnetLabs.nl wrote: Show quoted text
> > > My €0.02 contribution: > > I believe that tests that pass when users have identified bugs do not > provide sufficient coverage. I'm with Calle on this.
Calle's proposal is an extension of function, more than a bug. Although it has to be said that the documentation was less than helpful. The uniqueness function is adequately tested by existing test. This tests all the new code. Extending code to include existing RRs is a trivial addition (to code, but not function). If the trivial addition did not work, all pre-existing RRs would be deleted from section. Existing test also detects that (fails 25 out of 77). Existing test therefore has adequate coverage.
  Wed Nov 13 09:36:36 2013 NLNETLABS [...] cpan.org - Correspondence added
This is fixed in the upcoming 0.73 release
  Wed Nov 13 09:36:37 2013 NLNETLABS [...] cpan.org - Status changed from 'open' to 'resolved'