Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1667 closed defect (fixed)

dns::masterLoad should accept comment at EOL

Reported by: jinmei Owned by: jinmei
Priority: high Milestone: Sprint-20120306
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 2.18 Internal?: no

Description

Right now masterLoad() rejects a zone file even if it was preprocessed
by named-compilezone if the zone is signed, because it contains an
in-line comment for DNSKEYs:

example.				      3600 IN DNSKEY	256 3 7 AwEAAaetidLzsKWUt4swWR8yu0wPHPiUi8LUsAD0QPWU+wzt89epO6tH zkMBVDkC7qphQO2hTY4hHn9npWFRw5BYubE=  ; key id = 40430

Of course, masterLoad() should eventually be more flexible and should
be able to accept any valid form of zone files. But since it's
unlikely that we have enough time to do that by the end of y3, I think
we should at least accept this particular form, even by adding some
ad-hoc special case handling.

Subtickets

Change History (20)

comment:1 Changed 8 years ago by jinmei

removing from next sprint proposed. I thought we had one more sprint before the
next release, and intended to include this quick hack in it. but we should probably
consider the possibility of introducing more complete loader.

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Next-Sprint-Proposed to Year 3 Task Backlog

comment:3 Changed 8 years ago by jinmei

  • Priority changed from critical to major

comment:4 Changed 8 years ago by jinmei

  • Estimated Difficulty changed from 0 to 4

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Sprint-20120306

comment:6 Changed 8 years ago by jelte

  • Priority changed from major to critical

comment:7 Changed 8 years ago by jinmei

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

comment:8 Changed 8 years ago by jinmei

trac1667 is ready for review.

This is not a complete support for such style of comments (for
example, it won't be able to handle comments with double-semicolon),
but should be good enough for our current purpose. In any case
this should be removed when we make this loader more complete.

This is the proposed changelog entry:

384.?	[bug]		jinmei
	libdns++: masterLoad() didn't accept comments placed at the end of
	an RR.  Due to this the in-memory data source cannot load a master
	file for a signed zone even if it's preprocessed with BIND 9's
	named-compilezone.
	Note: this fix is considered temporary and still only accepts some
	limited form of such comments.  The main purpose is to allow the
	in-memory data source to load any signed or unsigned zone files as
	long as they are at least normalized with named-compilezone.
	(Trac #1667, git TBD)

comment:9 Changed 8 years ago by jinmei

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

comment:10 Changed 8 years ago by haikuo

  • Owner changed from UnAssigned to haikuo

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

I reviewed your changes in this ticket,and it works for the RDATA formats you mentioned.
but I have a question about the method you defined(stripComment), why you find the last semicolon but not the first one? As I know, the semicolon means that the later words after it should be treated as the comments. If you find the first semicolon and then erase all the later words, do you think it will be better than your method? Of cause no body want to modify the zonefile which was signed firstly,but I think our software will be robust if we do that.


For the function "masterLoad" you modified,you deal with the semicolon by using an exception, I think it is good for the performance although it looks like weird.
thanks

comment:12 Changed 8 years ago by haikuo

  • Owner changed from haikuo to jinmei

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

Replying to haikuo:

I reviewed your changes in this ticket,and it works for the RDATA formats you mentioned.
but I have a question about the method you defined(stripComment), why you find the last semicolon but not the first one? As I know, the semicolon means that the later words after it should be treated as the comments. If you find the first semicolon and then erase all the later words, do you think it will be better than your method? Of cause no body want to modify the zonefile which was signed firstly,but I think our software will be robust if we do that.

The first semicolon could be in the middle of RDATA, not at the start
of a comment like the one I used in the test:

example.com. 3600 IN TXT "aaa;bbb" ; this part is a comment

To handle such cases correctly we need a more complete parser with a
more complete "tokenizer", which would correctly extract '"aaa;bbb"'
as a quoted string. But we don't have time for that right now, so the
purpose of the ticket is a quickest (though incomplete, and sometimes
even incorrect) workaround that can handle a specific form of EOL
comment:

example.com. 3600 IN DNSKEY 256 3 7 <base64> ; key id = 40430

For the function "masterLoad" you modified,you deal with the semicolon by using an exception, I think it is good for the performance although it looks like weird.

Yeah I know. Please consider this one also a short term workaround.
When we support more complete parser, this hack will be gone.

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to haikuo

comment:15 follow-up: Changed 8 years ago by haikuo

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

As the quickest workaround,this ticket could be fixed,but my recommendation is that you'd better to create a new ticket to work as the complicated lexical parser for our RDATA.

comment:16 Changed 8 years ago by jinmei

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:17 Changed 8 years ago by jinmei

  • Owner changed from haikuo to jinmei
  • Status changed from reopened to reviewing

comment:18 in reply to: ↑ 15 Changed 8 years ago by jinmei

Replying to haikuo:

As the quickest workaround,this ticket could be fixed,but my recommendation is that you'd better to create a new ticket to work as the complicated lexical parser for our RDATA.

There's no explicit ticket for this specific issue, but there's a meta
ticket about completing the loader in general:
http://bind10.isc.org/ticket/1712
I believe we'll solve this issue in that context.

BTW: please do not close the ticket until the branch has been merged.
We close the ticket when the task is fully completed, and unless the
branch is merged it's not really completed yet. Also, in general,
it's the developer who is expected to close the ticket, not the
reviewer (it's a natural consequence that normally the developer does
the final merge). See the "Completing the Review Process" section of
http://bind10.isc.org/wiki/CodeReviewProcedure

I've merged the branch to master, now closing.

Thanks,

comment:19 Changed 8 years ago by jinmei

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

comment:20 Changed 8 years ago by jinmei

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