BIND 10 #957: IntervalTimerTest.destructIntervalTimer segfaults

[BIND 10 Development]

#957: IntervalTimerTest.destructIntervalTimer segfaults
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  y-aharen                           |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20110614
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  0.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:9 y-aharen]:
 > Replying to [comment:7 jinmei]:
 > > Normally we should first add a test case that could trigger the bug
 > > without the proposed fix, fix the problem in the main code, and then
 > > confirm the fix actually solves the problem with the previously added
 > > test.
 > >
 > > Isn't it possible in this case?
 > The segfaults will occur when !IntervalTimer is destructed while the
 timer has been expired and the invocation of the callback function is
 pending. I couldn't make such circumstances continually in unittest.

 I'd suggest adding some assertion check like the attached one.  This
 would make the bug more likely to happen without this fix.

 > I'd like to fix this to correctly manage the lifetime of an instance of
 !IntervalTimer by using boost::shared_ptr and shared_from_this(), which
 seems to be best practice.

 The code basically looks okay.  I have some comments and suggestions
 however:

 - I'd like to see more detailed comment about why we need
   shared_from_this, and, if that's a common practice, with a reference
   to this technique along with ASIO.  I myself didn't even know the
   existence of shared_from_this much less the use of it with ASIO.  If
   it's not only me, we need deeper explanation to help others
   understand the code.

 - I suspect this should never happen because the access to
   IntervalTimerImpl is limited in interval_timer.cc.  If this ever
   happens, it means a severe internal bug.  So, I'd rather use
   assert() here.
 {{{
 +    } catch (const boost::bad_weak_ptr& e) {
 +        isc_throw(isc::Unexpected, "Failed to update timer");
      }
 }}}

 - And, although it's not for this branch, I'd suggest using more
   informative error, preferably convert the asio error to string:
 {{{
      } catch (const asio::system_error& e) {
          isc_throw(isc::Unexpected, "Failed to update timer");
 }}}
   Or, if it's not feasible, the unused 'e' should be removed.

 Finally, I think the technique using shared_from_this is useful in
 other cases (specifically for the resolver code) and is uncommon.  So,
 could you introduce it to the bind10-dev list so that everyone will be
 aware of it?

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