BIND 10 #256: bugs in base32.cc

[BIND 10 Development]

#256: bugs in base32.cc
--------------------------------+-------------------------------------------
      Reporter:  jinmei         |        Owner:  jinmei                     
          Type:  defect         |       Status:  reviewing                  
      Priority:  major          |    Milestone:  06. 4th Incremental Release
     Component:  DNSPacket API  |   Resolution:                             
      Keywords:                 |    Sensitive:  0                          
Estimatedhours:                 |        Hours:                             
      Billable:  1              |   Totalhours:                             
      Internal:  0              |  
--------------------------------+-------------------------------------------
Changes (by stephen):

  * owner:  stephen => jinmei


Comment:

 Branch created: revision 2481.  Files checked:  revision 2508

 '''src/lib/datasrc/data_source.cc'''
 '''src/lib/exception/exceptions.h'''
 '''src/lib/dns/rdata/generic/rrsig_46.cc'''
 '''src/lib/dns/rdata/generic/nsec3param_51.cc'''
 '''src/lib/dns/rdata/generic/nsec_47.cc'''
 '''src/lib/dns/rdata/generic/dnskey_48.cc'''
 '''src/lib/dns/rdata/generic/ds_43.cc'''
 '''src/lib/dns/rdata/generic/nsec3_50.cc'''
 '''src/lib/dns/tests/rdata_nsec3param_unittest.cc'''
 '''src/lib/dns/tests/sha1_unittest.cc'''
 '''src/lib/dns/tests/base64_unittest.cc'''
 '''src/lib/dns/tests/rdata_dnskey_unittest.cc'''
 Changes are OK

 '''src/lib/dns/tests/rdata_rrsig_unittest.cc'''
 '''src/lib/dns/tests/rdata_nsec3_unittest.cc'''
 Differences OK.  Good idea to just put currently failing test cases in a
 disabled test rather than comment them out - it leaves them visible when
 the tests are run.

 '''src/lib/dns/tests/base32hex_unittest.cc'''
 '''src/lib/dns/tests/hex_unittest.cc'''
 Differences OK, although it took a few minutes to work out the logic for
 the decodeMap test (i.e. that a string with only the last character not
 equal to the zero character would result in a data stream with the last
 byte equal to the numeric value represented by the that character);
 additional comments here would be useful.  (Admittedly it would have been
 helpful to look at that test in hex_unittest.cc first, being the easier
 case.)

 '''src/lib/dns/util/sha1.h'''
 '''src/lib/dns/util/sha1.cc'''
 Not checked.  It is assumed that these are Donald Eastlake's
 implementation of SHA1 and have just been moved from their original
 location.

 '''dns/util/base_n.cc'''
 Header documentation in the file omits reference to the base16 classes.

 class !EncodeNormalizer
 __class !DecodeNormalizer__
 The intention of !EncodeNormalizer appears to be that when the iterator
 reaches base_end it will return a pad character, and continue to return a
 pad character no matter how many times it is subsequently incremented.
 However, when initialized with with both arguments equal (i.e. the current
 iterator being equal to the end iterator) a dereference operation will
 dereference the end iterator. Initializing in_pad_ to be equal to (base ==
 base_end) should fix this.

 A similar comment apply to !DecodeNormalizer; if initialized with base
 equal to base_beginpad, it should be regarded as being in the padding
 region.

 (Admittedly the only occasion where both constructor arguments are set to
 the same value is where the constructed object is used as a target of a
 comparison with another iterator, so correcting these items will have no
 effect on the operation of the existing code; the above comments are more
 for completeness should the classes be used elsewhere.)

 __struct BaseNTransformer__
 The documentation in the header could be a bit clearer as to why the third
 and fourth template parameters in the BaseNTransformer class are what they
 are.  It took a bit of rooting through the boost documentation to work out
 that they are correct.

 In the calculation of padbits in BaseNTransformer::decode, the use of "&
 ~7" is a neat trick to round down to the nearest multiple of 8.  It took
 me a few seconds to work out why it was there though - perhaps a comment
 would be in order?

 Not sure that the comment "Confirm the original BaseX text is the
 canonical encoding of the data" is completely correct - all the code does
 is check that the padding in the decoded data does not conflict with the
 padding in the baseN-encoded text.

 Not sure why this is a "struct" rather than a "class" like
 !EncodeNormalizer and !DecodeNormalizer.

 '''src/lib/dns/Makefile.am'''
 The files util/base16_from_binary.h and util/binary_from_base16.h are not
 in the list of libdns++ sources (but they are included in util/base_n.cc)

 '''src/lib/dns/tests/Makefile.am'''
 OK (but see "Other Comments" below).

 '''base16_from_binary.h'''
 '''base32hex_from_binary.h'''
 '''binary_from_base16.h'''
 '''binary_from_base32hex.h'''
 OK - in both these cases the differences from the original boost file seem
 reasonable.  The tables have been checked against those in
 [http://tools.ietf.org/html/rfc4648 RFC4648] and appear to be correct.

 Comment: The check on "t" in the "to_n_bit::operator()" method is "<= 127"
 in binary_from_base32hex.h and "< sizeof(lookup_table)" in
 binary_from_base16.h.  Both have the same effect (the lookup table is 128
 bytes long), although the latter is preferable.

 '''src/lib/dns/util/README'''
 OK.  Although it is not clear why the classes should not be used within
 other modules of BIND10.  So far the contents of util are
 binary<-->base16/32 converters and a copy of Donald Eastlake's
 implementation of the SHA1 code in [http://tools.ietf.org/html/rfc4634
 RFC4634].  These are not really DNS-specific, so could be in a general
 BIND-10 library.

 '''Other Comments'''
 The version of the code checked did not include any unit tests of the
 base16 classes.

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