Opened 8 years ago

Closed 8 years ago

#1614 closed defect (complete)

in memory loader should be able to load RRSIGs correctly

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120207
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: 5 Add Hours to Ticket: 0
Total Hours: 5 Internal?: no

Description

We need to think about several things:

  • Correctly distinguish RRSIGs that have different covering types
  • How to store RRSIGs: whether storing them in the same table as other ordinary types of RRs or attaching them to the corresponding covered RRsets at load time. In the former case, find() needs to be able to construct returned RRsets with RRsigs on the fly - see also #1552. In the latter case, in general we need to be able to handle RRSIGs are passed to the loader before the covered RRsets.
  • const-ness: RRsets are passed to the loader (and currently maintained in the data source) as const data. So we cannot simply attach RRSIGs via addRRsig() unless we break the const or copy the original RRset every time.

My current suggestion is:

  • Attach RRSIGs at load time because it will make lookup performance more efficient. Ignore the reverse ordering for now - as long as the source is generated by something like dnssec-signzone this shouldn't happen.
  • As for const-ness, intentionally break cost at load time (i.e use const_cast and call addRRsig) for now. This is ugly and bad practice, but in practice the caller of the in-memory loader would normally transfer the ownership of the RRsets (this is the case for b10-auth), so it actually won't result in inconsistency. In future, we need to revise the internal representation anyway, not storing the data in the form of high level RRset objects to reduce memory footprint, and we should be able to solve the issue more cleanly at that point.

Subtickets

Attachments (1)

Trac1614-segfault.txt (8.2 KB) - added by stephen 8 years ago.
gdb stack trace

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:2 Changed 8 years ago by jelte

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

comment:3 Changed 8 years ago by jinmei

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

comment:4 Changed 8 years ago by jinmei

trac1614 is ready for review.

There's one, not directly related cleanup: 92295a5. But it should be
obviously correct and safe, and as noted in the commit log it helped
the main code a bit cleaner (or less ugly).

I also updated masterLoad() in libdns++ so that it can separate RRSIGs
of different type covered (aba3a76).

I'd consider this a kind of a bug fix, so it'd be worth a changelog
entry. This is the proposed one:

365.?	[bug]		jinmei
	libdatasrc: in-memory data source could incorrectly reject to load
	zones containing RRSIG records.  For example, it didn't allow
	RRSIG that covers a CNAME RR.  This fix also makes sure find()
	will return RRsets with RRSIGs if they are signed.
	(Trac #1614, git TBD)

comment:5 Changed 8 years ago by jinmei

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

comment:6 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to jinmei

Just one very minor point:

src/lib/dns/masterload.h
Suggest that the comment

RRSIGs are handled separate RRsets, that is, not set within the RRset
that they cover.

is perhaps better phrased as

RRSIGs are handled as separate RRsets, i.e. they are not included in
the RRset they cover.

The ChangeLog entry looks OK.

After updating the comment, please merge - there is no need for a re-review.

comment:8 Changed 8 years ago by stephen

Just after I clicked the "Submit" button to approve the ticket, I realised that I had not actually run the unit tests. I rebuilt BIND 10 from scratch, ran the test and got a segmentation fault in the addbadRRsig test - I'm running Ubuntu 10.10 with gcc 4.4.5 and have attached the gdb stack trace. I also got a segmentation fault in the same test with gcc 4.2.1 on the Mac.

Changed 8 years ago by stephen

gdb stack trace

comment:9 in reply to: ↑ 7 Changed 8 years ago by jinmei

Replying to stephen:

Thanks for the prompt review.

Just one very minor point:

After updating the comment, please merge - there is no need for a re-review.

Thanks, the original text had certainly an error, and the suggested
text looks good. Applied.

Just after I clicked the "Submit" button to approve the ticket, I
realised that I had not actually run the unit tests.

Oops, nice catch. This is a regression introduced by the additional
change to masterLoad(). I believe I fixed in the latest version.

I also re-confirmed all unit tests passed.

Last edited 8 years ago by jinmei (previous) (diff)

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:11 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to jinmei

Seems OK now, please merge.

comment:12 in reply to: ↑ 11 Changed 8 years ago by jinmei

Replying to stephen:

Seems OK now, please merge.

Thanks, merge done, closing.

comment:13 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 5
Note: See TracTickets for help on using tickets.