BIND 10 #363: "H" and "I" are probably harmful for PyArg_ParseTuple()

[BIND 10 Development]

#363: "H" and "I" are probably harmful for PyArg_ParseTuple()
-------------------------------------+-------------------------------------
                 Reporter:  jinmei   |                Owner:  jinmei
                     Type:  defect   |               Status:  reviewing
                 Priority:  minor    |            Milestone:  A-Team-
                Component:           |  Sprint-20110316
  DNSPacket API                      |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  0.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:12 zzchen_pku]:

 > > - As far as I can see, some of the long int variables don't have to be
 > >   so (they can be a plain int): Message_init, Message_clear,
 > >   MessageRenderer_setCompressMode. (although being long is not
 > >   necessarily incorrect; it just consumes more (stack) space
 > >   unnecessarily).  Actually, in these cases I think it make more sense
 > >   to do this check within the corresponding C++ class (Message,
 > >   MessageRenderer, etc) and delegate the check to it.

 The "Actually..." part hasn't been addressed (but to address that
 we'll also need to update the C++ Message/MessageRenderer
 implementation.  So this set of changes would probably go to a
 separate ticket).

 > > - Name_init: I wouldn't bother to check '> 0xffff'.  In fact, the
 > >   buffer could store longer data than 64KB.  Invalid (positive)
 > >   positions will be rejected in setPosition() anyway.  (with removing
 > >   that check) I'd also consider this case a "type error" rather than
 > >   overflow, that is, a negative integer type is passed where an
 > >   unsigned is expected.  Same comment applies to other similar cases.

 Did you clarify the 'type error' vs 'overflow' for negative values
 throughout the interfaces (not only for Name_init)?  The error policy
 should be consistent.

 > > - Name_at, Name_split: likewise.  And, in this case it doesn't even
 have to be
 > >   long (int should be sufficient).

 These don't seem to be addressed in that it still checks 0xffff (and
 it still returns 'overflow' for a negative 'pos', see above).

 > > - RRTTL_init: this doesn't seem to be correct:

 > I think ignore overflow is fine for this specific case, so I'll keep
 "k", is this ok?

 If I were you I'd rather provide consistent policy on overflow checks
 at the minor risk of portability issue, but if you have a reason that
 it's better to ignore, I'd not necessarily object to that.  In that
 case, however, please leave comment that this case is different from
 other checks of this wrapper with your reason, and add the pydoc style
 API documentation about the difference.

 Also, in that case I wonder why this didn't cause a problem in your
 32-bit environment:
 {{{
     unsigned long i;
 ...
             if (i > 0xffffffff) {
 }}}

 I suspect g++ would normally warn about this situation like this:
 {{{
 warning: comparison is always false due to limited range of data type
 }}}
 which subsequently makes the build fail due to -Werror.

 Besides, since "k" doesn't perform overflow checking, the check is
 already quite moot.

 > > - message_python_test.py: the change makes the following comment
 > >   incorrect:
 > > {{{
 > >          # this would cause overflow and result in a "valid" flag
 > > }}}
 > Updated.
 > > - tests in general.  I've not closely checked the test coverage, but
 > >   from a quick look there seems to be some cases that were not tested
 > >   (e.g. for RRTTL).  Please make sure all new code is covered by tests
 > >   (note also that the coverage problem is unlikely to happen if it was
 > >   developed test driven); or, if I miss something and the code was
 > >   fully covered already, please say so.
 > It should has been covered by original unittest:
 > {{{
 > self.assertRaises(InvalidRRTTL, RRTTL, "4294967296")
 > }}}

 No, because "4294967296" is a string.  Also, this answer seems to
 suggest that the way of development may not really honor the spirit of
 TDD.  Since the previous code used "I", which allows overflow, a test
 for 4294967296 (as an integer) would have had failed.  When we wanted
 to fix that, we first add a test that actually makes it fail, add code
 that should fix it, and then confirm it really fixes the problem (but
 with "k" we wouldn't be able to be sure about it in the end).

 Besides, simply responding to the RRTTL case doesn't really address my
 main concern.  My question was more general, not only specific to
 RRTTL: whether you confirmed all of your new code was really covered
 by tests (preferably in the way of TDD).  As I said, this was probably
 not the case.  Please make sure everything was really covered.

 And one more thing regarding tests: now we tightened the range check
 at the level of python wrapper, I'd also confirm the check isn't too
 tight with the possible min/max values.

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