BIND 10 #513: b10-auth hangs in submitting statistics

[BIND 10 Development]

#513: b10-auth hangs in submitting statistics
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:           |            Milestone:  A-Team-
  critical                           |  Sprint-20110126
                Component:           |           Resolution:
  b10-auth                           |            Sensitive:  0
                 Keywords:           |  Add Hours to Ticket:  0
Estimated Number of Hours:  0.0      |          Total Hours:  0
                Billable?:  1        |
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 y-aharen]:
 > The branch and changelog entry are OK. It is ready to merge into trunk.

 Okay, thanks.  I'm going to close this ticket.
 >
 > Replying to [comment:9 jinmei]:
 > > Although I agree it should be rejected at the frontend of
 configuration
 > > parser (so I fixed it), I also believe this is primarily a problem of
 > > the interface of IntervalTimer::setupTimer().  It takes an unsigned
 > > integer and naively passes to boost::posix_time::seconds(), which
 assumes
 > > a signed long value.  We should at least use a fully compatible type
 > > (simple 'long' would be best because we cannot even assume it's 32
 bit),
 > > and, if we want to reject negative timer intervals as part of the
 > > IntervalTimer interface, we should explicitly check and reject them
 > > in its setupTimer().  But I wouldn't go into that level within this
 ticket
 > > anyway.  It would have to go to a separate ticket/task.
 > I intended to make sure IntervalTimer::setupTimer() only accepts
 positive value or 0 by that, although boost::posix_time::seconds() is for
 generic purpose and accepts negative value. If it is a bad idea, I will
 create a ticket to address that.

 The problem is that by (implicitly) converting uint32_t to (signed)
 long we lose information.  More specifically, think about the case
 where someone passes 0xffffffff to setupTimer() (perhaps expecting it
 effectively works as "infinity".)  But it will be converted to -1
 within asio, and we'll see some surprising effects.  Implicit conversion
 into a type with smaller capacity should generally be avoided to
 avoid this kind of surprise.

 Note also that it's quite fragile to rely on a specific size (32bits
 in the case of current code) of 'long'.  In theory we could even see
 an environment where long is 16 bit signed integer.  For such
 (mostly imaginary) systems, even more sensible interval like 32768
 would cause an overflow.

 So, the safest way is to use the same (long) or non larger type (such as
 signed int) in the interface to setupTimer() and (if you don't want to
 allow
 negative interval) explicitly reject negative values as part of
 validation.
 A suboptimal alternative would be to use an unsigned type whose max value
 is normally still positive for 'long' (this would be the case with
 uint16_t
 for most systems we are supporting), but as already explained it's
 much, much suboptimal than the safest way.

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