Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2267 closed defect (fixed)

new generateRRsetFromIterator doesn't handle RRSIGs correctly

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120925
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 7 Internal?: no

Description (last modified by jinmei)

The new (memory-efficient) version of in memory loader from another
data source has a bug. It cannot load RRSIGs correctly if a single
name has multiple types of RRsets and their RRSIGs.

It can be confirmed by applying the following patch to the unit test.
load() will fail with an exception.

 const char* rrset_data[] = {
     "example.org. 3600 IN SOA   ns1.example.org. bugs.x.w.example.org. 68 3600 300 3600000 3600",
     "a.example.org.                     3600 IN A      192.168.0.1",
+    "a.example.org. 3600 IN RRSIG A 5 3 3600 20150420235959 20051021000000 "
+    "40430 example.org. FAKEFAKEFAKE",
     "a.example.org.                     3600 IN MX     10 mail.example.org.",
     NULL
 };

Even ignoring this bug, the current implementation doesn't seem to be
very understandable due to the last_rrset_ member variable. It's
quite difficult to keep track of this thing.

My suggested fix is this:

  • simplify generateRRsetFromIterator() so it now just passes each RRset in the returned order.
  • implement the buffer logic in InMemoryClientImpl::addFromLoad(). it ensures a matched pair of RRset and (if signed) its RRSIG RRset is generated. (it can assume if the different names of RRsets aren't interleaved.) To implement this, it's probably better to introduce a helper Loader class.
  • revise InMemoryClientImpl::addFromLoad() so it takes RRset and its RRSIG. Then we won't need the last_rrset_ magic.
  • InMemoryClient::add() (and maybe some tests) will need to be updated.

Also, we should merge the older loadFromIterator test.

Subtickets

Change History (16)

comment:1 Changed 7 years ago by jinmei

  • Component changed from Unclassified to data source

comment:2 Changed 7 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:3 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120925

comment:4 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:5 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:6 Changed 7 years ago by jinmei

trac2267 is ready for review.

This is basically a straightforward implementation of the description.
I ended up introducing a helper "Loader" class. The important
behavior is implemented in that class.

I've made some additional cleanups. See commits that have "cleanup"
in the commit log.

no plan to add changelog as the bug didn't exist in released versions.

comment:7 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:8 Changed 7 years ago by jinmei

forgot to mention this: I tried to incorporate the old
loadFromIterator test from datasrc/tests, but then found that it
required the sqlite3 loadable module or compiling the .cc. I thought
it's too much and skipped it for now. In theory, the tests using the
mock iterator should cover all features, and hopefully system tests
or other test cases in datasrc/tests will cover the more practical
setup.

comment:9 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:10 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Looks ok, just some minor comments on memory_client.cc:

while you are cleaning up, in void add() (lines 445-474), the second half could be refactored a tiny bit, since the addWildcards is the 'exceptional' thing (addRdataSet happens both for NSEC3 and non-NSEC3), i.e. make it something like:

        if (rrset->getType() != RRType::NSEC3()) {
            // Add wildcards possibly contained in the owner name to the
            // domain tree.
            // Note: this can throw an exception, breaking strong exception
            // guarantee.  (see also the note for the call to contextCheck()
            // above).
            addWildcards(zone_name, zone_data, rrset->getName());
        }

        addRdataSet(zone_name, zone_data, rrset, sig_rrset);

I also suggest you slightly change the comments above the Loader class; 'if the owner name is changed' suggests to me something like a setName() function. So I would prefer saying something akin to 'if a new owner name is encountered, no subsequent RRset will have the previous owner name'.

comment:11 in reply to: ↑ 10 Changed 7 years ago by jinmei

Thanks for the review.

Replying to jelte:

Looks ok, just some minor comments on memory_client.cc:

while you are cleaning up, in void add() (lines 445-474), the second half could be refactored a tiny bit, since the addWildcards is the 'exceptional' thing (addRdataSet happens both for NSEC3 and non-NSEC3), i.e. make it something like:

        if (rrset->getType() != RRType::NSEC3()) {
            // Add wildcards possibly contained in the owner name to the
            // domain tree.
            // Note: this can throw an exception, breaking strong exception
            // guarantee.  (see also the note for the call to contextCheck()
            // above).
            addWildcards(zone_name, zone_data, rrset->getName());
        }

        addRdataSet(zone_name, zone_data, rrset, sig_rrset);

You mean, instead of checking the type NSEC3 at the first call to
addRdataSet? Updated the code assuming so.

I also suggest you slightly change the comments above the Loader class; 'if the owner name is changed' suggests to me something like a setName() function. So I would prefer saying something akin to 'if a new owner name is encountered, no subsequent RRset will have the previous owner name'.

Okay, done. I also noticed some editorial errors around there, and
fixed them.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:13 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

IMO the empty return statement can also be removed, but I don't need to see that anymore.

This can be merged

comment:14 in reply to: ↑ 13 Changed 7 years ago by jinmei

Replying to jelte:

IMO the empty return statement can also be removed, but I don't need to see that anymore.

Ah, right, good catch. Removed it, and merged.

Now closing the ticket.

comment:15 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 7
Note: See TracTickets for help on using tickets.