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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:5 jinmei]:
 > '''acl/dns.cc'''
 >
 > ...hmm, but on further thought it's not specific to this case...this
 > can be a potential issue wherever we use this type of static proxy
 > objects.  I'd be still concerned about it with such a complicated
 > class than a mostly straightforward value-class like `RRType`, but
 > that'd be a separate issue anyway.
 >

 Yeah it is solely for end-of-program destruction. Originally I tried
 another approach; make a (locally namespaced) cleanup method, and use
 atexit(), but that seemed overkill for this purpose, and less clear than a
 RAII-style smart pointer encapsulation.

 With careful use of atexit(), I do believe we can solve the issue, but it
 would be fragile; If we really do care about destruction order, we may
 need to create some CleanupRegister class, that knows internal class
 dependencies, and register singletons there, so that it can clean up those
 objects in the right order (or possible even a number of them, depending
 on library layout).

 > So I'm okay with the change with a couple of suggestions:
 >
 > - I'm afraid the intent of the use of smart pointer is not so obvious,
 >   so I'd comment why we do it.

 ok

 > - 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

 (btw, i just noticed the includes were not in the right order, so changed
 that too)

 > '''sqlite3_accessor_unittest.cc'''
 >
 > Not really related to the branch, but this comment doesn't seem to be
 > correct:
 > {{{#!cpp
 >     // should work now that we closed it
 >     SQLite3Accessor accessor3(SQLITE_NEW_DBFILE, "IN");
 > }}}
 > Looks like a copy-paste error.
 >

 and the original was not exactly right either :p Updated.

 > '''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.

 I've reverted it to just add the one byte to the fixed array.

 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.

 > '''recursive_query_xxx'''
 >
 > Not directly related, I'd make the 'const char*' variables 'const
 > char* const'.  I'd also define things in an unnamed namescope.
 >

 ok done

 > '''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

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