BIND 10 #1641: Further bug(s) in NSEC3 RDATA implementation

[BIND 10 Development]

#1641: Further bug(s) in NSEC3 RDATA implementation
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120221
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:  NSEC3  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 stephen]:

 Thanks for the review.

 > The branch failed to build on Ubuntu 10.10 (gcc 4.4.5) because "memset"
 was not defined in src/lib/dns/rdata/generic/detail/nsec_bitmap.cc.  I've
 included the header file (string.h) and pushed the change (commit
 9e3c7d5).

 Ah, okay, thanks.  I've changed it to <cstring> as it's probably a bit
 more C++-ish.

 > '''src/lib/dns/rdata/generic/detail/nsec_bitmap.cc'''[[BR]]
 > buildBitmapsFromText: in the search for the a non-empty window,
 inverting the sense of the test for "octet" after the loop avoids the need
 for a "continue".
 >
 > bitmapsToText: "continue"s can be avoided by inverting the sense of the
 tests.

 Do you mean

 {{{#!c++
         if (octet >= 0) {
             typebits.push_back(window);
             typebits.push_back(octet + 1);
             for (int i = 0; i <= octet; ++i) {
                 typebits.push_back(bitmap[window * 32 + i]);
             }
         }
 }}}
 instead of this (and do the similar thing for bitmapsToText)?
 {{{#!c++
         if (octet < 0) {
             continue;
         }
         typebits.push_back(window);
         ...
 }}}

 If so, hmm, I don't mind changing them as suggested, but for now I've
 not changed these parts.  On one hand, I generally prefer shorter
 code, and in that sense the suggest version is better.  On the other
 hand, especially when the "octet >= 0" case is long, I'd rather make
 it clearer that the simpler case is completed at an earlier part of
 code (otherwise, if I were first reading/reviewing the code I'd have
 to push the case of octet < 0 case in my brain stack while I'm reading
 the if block).  In these particular cases, it's not clear which point
 is more important: The code is quite short and comprehensive in either
 way, and the total number of lines are not different in either way.

 Also, this implementation is a quite straightforward port of BIND 9's
 implementation, and these "continue"s are derived from BIND 9, too.
 Considering the two styles are not so different in terms of
 readability (not at least to me), I think it makes sense to keep the
 organization as compatible as BIND 9 (e.g. so if we happen to find a
 bug in one version it'll be easier to apply the same fix to the
 other).

 In any case, these are not a strong opinion.  If you strongly feel the
 suggested version is better, I'll change that.

 > '''src/lib/dns/rdata/generic/nsec3_50.cc'''[[BR]]
 > compareVectors: Needs a header. (The content is obvious but it took me a
 little time to work out that your would sort short vectors first -
 regardless of content - when comparing numbers.)

 Do you mean descriptive comments by "a header"?  I added some.

 > compareVectors: when doing the length check, why not just return "len1 -
 len2" as is done later in the function?

 Good point, changed.

 > compareVectors: The "if (cmp != 0)" statement is not needed:
 > {{{
 > return(cmplen == 0 ? (len1 - len2) : memcmp(...));
 > }}}

 If you mean replacing:
 {{{#!c++
     const int cmp = cmplen == 0 ? 0 : memcmp(&v1.at(0), &v2.at(0),
 cmplen);
     if (cmp != 0) {
         return (cmp);
     } else {
         return (len1 - len2);
     }
 }}}
 with
 {{{#!c++
     return (cmplen == 0 ? (len1 - len2) : memcmp(&v1.at(0), &v2.at(0),
 cmplen));
 }}}

 then we cannot do this because it will incorrectly return 0 if cmplen
 > 0, len1 != len2 and memcmp returns 0 (and check_length_first is
 false).

 Or did you mean something else?

-- 
Ticket URL: <https://bind10.isc.org/ticket/1641#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development