Opened 7 years ago

Closed 7 years ago

#2586 closed defect (fixed)

sqlite3 data source doesn't seem to handle escaped name correctly

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

Description

#2480 revealed a couple of bugs with SQLite3 data source:

TEST_P(QueryTest, nxdomainWithNSEC2) {
    // there seems to be a bug in the SQLite3 (or database in general) data
    // source and this doesn't work.
}
TEST_P(QueryTest, nxdomainWithNSECDuplicate) {
    // there seems to be a bug in the SQLite3 (or database in general) data
    // source and this doesn't work.  This is probably the same type of bug
    // as nxdomainWithNSEC2
}

I've not fully looked into the cause, but these cases involve a tricky
owner name of ").no.example.com." (the first label is normally escaped
by a backslash in text), and I suspect that's related to this issue.
And, if so, it may not be easily solved - I suspect that would revisit
the data representation at the database layer.

So, in this ticket, I propose we first figure out the real reason. If
it turns out to be solvable without touching the database layer, solve
it within this ticket; otherwise, just record the fact and close the
ticket.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by shane

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

comment:2 Changed 7 years ago by jelte

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

comment:3 Changed 7 years ago by jinmei

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

comment:4 Changed 7 years ago by jinmei

trac2586 is ready for review.

As I guessed, it was because of the use of escaped labels. In the
original test scenarios, it (correctly) assumed the test names were
sorted in the following order:

mx.example.com. (exist)
(.no.example.com. (qname, NXDOMAIN)
).no.example.com. (exist)
*.no.example.com. (best possible wildcard, not exist)

But '(' and ')' are escaped in the sqlite3 DB file, and since the
ascii code of '\' (92) is larger than that of '*' (42), the data
source would sort them as follows:

mx.example.com. (exist)
*.no.example.com. (best possible wildcard, not exist)
\(.no.example.com. (qname, NXDOMAIN)
\).no.example.com. (exist)

and produced the wrong result in identifying the "previous name" for
finding negative proof records.

As described in the ticket description, this is a fundamental (known)
issue of the current design, and requires substantial changes.
However, we can avoid the failure in the query test by choosing labels
that are not escaped. So I made such change in the branch and enable
the tests for the SQLite3 data source, too. Note that escape handling
is not the subject of this test, so we are not hiding anything by
doing so.

This shouldn't need a changelog.

comment:5 Changed 7 years ago by jinmei

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

comment:6 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to jinmei

hmz, it fails for me:

dnsmessage_test.cc:52: Failure
Value of: message.getRcode()
  Actual: NOERROR
Expected: rcode
Which is: NXDOMAIN
dnsmessage_test.cc:71: Failure
Value of: message.getRRCount(Message::SECTION_AUTHORITY)
  Actual: 4
Expected: nscount
Which is: 6
../../../../src/lib/testutils/dnsmessage_test.h:270: Failure
Value of: actual_rrsets.size()
  Actual: 4
Expected: expected_rrsets.size()
Which is: 6
Google Test trace:
../../../../src/lib/testutils/dnsmessage_test.h:267: Comparing two RRset lists:
Actual:
example.com. 0 IN SOA . . 1 0 0 0 0
example.com. 0 IN RRSIG SOA 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE
mx.example.com. 3600 IN NSEC &.no.example.com. MX RRSIG NSEC
mx.example.com. 3600 IN RRSIG NSEC 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE
Expected:
example.com. 0 IN SOA . . 1 0 0 0 0
example.com. 0 IN RRSIG SOA 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE
mx.example.com. 3600 IN NSEC &.no.example.com. MX RRSIG NSEC
mx.example.com. 3600 IN RRSIG NSEC 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE
&.no.example.com. 3600 IN NSEC nz.no.example.com. AAAA RRSIG NSEC
&.no.example.com. 3600 IN RRSIG NSEC 5 3 3600 20000101000000 20000201000000 12345 example.com. FAKEFAKEFAKE

(the other one works)

If we just want to 'fix' the test, we could use '!.no.example.com' instead of '%.no.example.com', which passes here. But it would appear there is more wrong in the handling here (unpexpected behaviour around '%' in sql context is worrying)...

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

Replying to jelte:

hmz, it fails for me:

[...]

(the other one works)

If we just want to 'fix' the test, we could use '!.no.example.com' instead of '%.no.example.com', which passes here. But it would appear there is more wrong in the handling here (unpexpected behaviour around '%' in sql context is worrying)...

Ah, okay. It didn't happen to me, so it may depend on the version of
SQLite3, but I can see the possibility. I just changed it to ! in the
test. This test is not for checking behavior with these unusual
characters, so it makes sense to use safer characters anyway.

As for the use of % in SQL-based data sources, I guess we'll need to
defer it to revisiting the label representation in SQLs.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

ack, now it passes for me, please go ahead and merge

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

Replying to jelte:

ack, now it passes for me, please go ahead and merge

Thanks, merge done, closing.

comment:12 Changed 7 years ago by jinmei

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