BIND 10 #2275: work on valgrind backlog

[BIND 10 Development]

#2275: work on valgrind backlog
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 jelte]:

 > > - I'd use boost::scope_ptr instead of auto_ptr.  In this case the
 > >   former should be sufficient.
 >
 > TBH, I think either are fine, but no strong opinion so I've changed it

 In this particular case (where the scope is very small and it's
 obvious that auth_ptr is used like scoped_ptr), it may not matter much
 in practice.  But, in general, I prefer using the least powerful tool
 for a specific goal because it would be generally less error-prone and
 often more efficient.  Since in this case we don't have to transfer the
 ownership of the pointer, we don't need that part of the auto_ptr
 features, and in that sense auto_ptr has unnecessary feature for us.

 Anyway, I saw it changed, so of course I'm fine with that.

 > > '''labelsequence_unittest.cc'''
 > >
 > > My valgrind (3.8.1) doesn't complain about this.  What's wrong with
 > > the original code?
 >
 > uninitialized memory; the constructor checks both offsets and label
 length in one loop; so even though it failed on the offset being wrong,
 the label data memory was uninitialized.

 Ah, right.  The original code was really buggy and valgrind correctly
 identified it.

 > Come to think of it, this test only checks one of the error conditions
 (wrong offset, not max_labellen). Added a second case there to hit the
 other one.

 This is fine.

 > > '''socket_requestor_test.cc'''
 > >
 > > My valgrind (3.8.1) doesn't complain about this.  What's wrong with
 > > the original code?
 >
 > another uninitialized memory error; I think it is not a difference in
 valgrind, but that my system accesses more (possibly all) of the path data
 bytes when binding a domain socket

 Hmm, on a closer look, I suspect the original code was really buggy,
 and the *right* fix is this:

 {{{#!cpp
         strncpy(socket_address.sun_path, path_,
                 sizeof(socket_address.sun_path));
 }}}
 (without the explicit memset)

 The previous version used the length (strlen) of path_, so sun_path
 could actually be non-nul terminated even if path_ is short enough.
 (another reason why we should generally avoid such low-level APIs).

 I believe with this change valgrind stops complaining about this.

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