BIND 10 #80: review: auth server incorrectly returns SERVFAIL to queries of class ANY

[BIND 10 Development]

#80: review: auth server incorrectly returns SERVFAIL to queries of class ANY
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  assigned
                 Priority:           |            Milestone:  A-Team-
  critical                           |  Sprint-20110316
                Component:           |           Resolution:
  b10-auth                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:
Estimated Number of Hours:  5.0      |          Total Hours:
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Branch trac80 is ready for review.

 For testing purposes I've cherry-picked a couple of changes from
 trac626, which are irrelevant to the fix itself and can be ignored.
 The main diff is
 {{{
 git diff 0f225ec^ 66e6ece
 }}}

 The bug was in retrieving RRsets from RRsetList, which stores the
 result of data source lookup.  The original code often did this:
 {{{
     result_list.findRRset(rrtype. query.qclass());
 }}}

 When it's a class ANY query, query.qclass() is ANY, but result_list
 stores the data from the data source, which is not normally ANY
 (typically it's class IN).  The buggy code sometimes naively assumes
 the result is non NULL, and sometimes incorrectly handles the case as
 it returns NULL.

 Within the data source implementation we ensure the RR class is
 already valid, so we don't have to match the class.  I've solved this
 issue by introducing a custom search routine.

 Various instances of this type of bug were fixed with added tests, but
 I've left open one final case: in getNsec3() and getNsec3Param().
 There didn't seem to be any test for these cases anyway, and we'd have
 to prepare NSEC3-signed zone to test it, which would make the diff too
 big.  Since this case of bug doesn't seem to be critical in that it
 won't trigger a crash, I'd leave it to a separate task.  I thought it
 would be more important to fix the crash bug as soon as possible.

 In tests, I've also introduced "no DNSSEC mode" to simplify tests
 where DNSSEC related results are not the essential part of the test.
 There's another unrelated cleanup: I've changed test domain name from
 "flame.org" to "example.net" to avoid using a real domain main used by
 someone.

 It would also make sense to check both class IN and ANY for all
 existing query tests like I did in this branch for WildcardCname.
 That would go to a separate task, too.

 This is the proposed changelog entry:
 {{{
   197.? [bug]           jinmei
         b10-auth, src/lib/datasrc: class ANY queries were not handled
         correctly in the generic data source (mainly for sqlite3).  It
         could crash b10-auth in the worst case, and could result in
         incorrect responses in some other cases.
         (Trace #80, git TBD)
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/80#comment:17>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development