BIND 10 #1430: Data source error with mismatched label counts

[BIND 10 Development]

#1430: Data source error with mismatched label counts
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  jinmei
                       Type:         |                Status:  assigned
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120110
  critical                           |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  2
            Defect Severity:         |           Total Hours:  0
  Medium                             |
Feature Depending on Ticket:  none   |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 trac1430 is ready for review.

 First, self comment on my previous comments: On further look I
 realized this couldn't trigger an exception:
 {{{#!c++
     const size_t remove_labels(current_label_count - origin_label_count);

     // Now go trough all superdomains from origin down
     for (int i(remove_labels); i > 0; --i) {
         Name superdomain(name.split(i));
 }}}

 If overflow happens "i" would be a negative value so the entire for
 loop should be skipped.

 Second, as a result of refactoring the find() method an exception
 couldn't happen anyway.  But, more fundamentally, we should have
 caught and rejected out-of-zone query name earlier in the find()
 method; otherwise expressions like `name.getLabelCount() -
 origin_label_count` would have a bogus value and confuse the rest of
 the code.  Even if it doesn't trigger an exception the end result is
 also garbage (for example, it could be NXRRSET, which is obviously
 incorrect).

 So, in this branch I added an explicit check for an out-of-zone name
 and made sure it's rejected with NXDOMAIN (which is compatible with
 the inmemory data source behavior in this case).

 As for the application, I suspect it was actually xfrout, not xfrin.
 xfrout naively calls find() for NS names in notify_out (which was
 actually written by myself, but the older code using the older API had
 the same problem.  It just didn't result in an exception).  As I noted
 in my previous comment, this behavior of application is also a bug,
 and should be fixed even if it doesn't die due to an exception.  I'll
 make a separate ticket for this (right now it won't die so it's not
 urgent).

 After fixing notify_out (for xfrout), I think we should actually
 rather throw an exception for out-of-zone qname for find().  At least
 returning NXDOMAIN doesn't seem to be the best choice, because then
 the caller may misunderstand the name could belong to the zone but
 actually doesn't exist.  Also, since this is basically an
 application's bug should it happen, it would be better to signal it
 more explicitly rather than allowing the application to be unaware of
 it.  I plan to make a separate ticket for this, too.

 The proposed changelog entry is:
 {{{
 350.?   [bug]           jinmei
         ZoneFinder::find() for database based data sources didn't
         correctly identify out-of-zone query name and could return a
         confusing result such as NXRRSET.  It now returns NXDOMAIN with an
         empty RRset.  Note: we should rather throw an exception in such a
         case, which should be revisited later.
         (Trac #1430, git TBD)
 }}}

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