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

 * owner:  zzchen_pku => jinmei


Comment:

 Replying to [comment:10 jinmei]:
 > On thinking about it further, I now agree long is better than int, but
 > not for that reason.  The above citation is about internal
 > representation of integers in the python language, and regardless of
 > the internal representation PyArg_ParseTuple() can convert them to
 > various C integer types, so the question of int vs long (vs short,
 > etc) still holds.
 >
 > In our usage many integers are unsigned 16-bit ones.  To convert them
 > safely (without worrying about overflow), we need at least 32-bit
 > signed integers.  That's why I agree long (which is ensured to be at
 > least 32-bit) is better than int (which can be e.g. 16-bit).  For this
 > rationale, in some cases we really don't need long, and in some other
 > cases even long is not safe enough.  See specific cases below.
 Make sense.
 > Now, specific comments about the code:
 >
 > - this may not specific to this patch, but some calls to PyErr_Clear()
 >   seem to be redundant.  example:
 > {{{
 >     if (messageflag < 0 || messageflag > 0xffff) {
 >         PyErr_Clear();
 >         PyErr_SetString(PyExc_OverflowError, "Message header flag out of
 range");
 > }}}
 Removed redundant PyErr_Clear()
 > - 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.
 >
 > - on the other hand, in this case the integer has to be long:
 >  - Message_setQid() (because size of int may be 16 bits)
 >
 > - Message_addRRset: I'd avoid using hardcoded range here.  Actually,
 >   in this case I think it make more sense to delegate the check to the
 >   C++ code.
 >
 > - 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.
 > - Name_at, Name_split: likewise.  And, in this case it doesn't even have
 to be
 >   long (int should be sufficient).
 Done.
 > - Name_split: is this case tested? (catching it seems to be correct and
 good)
 > {{{
 > +    PyErr_Clear();
 > +    PyErr_SetString(PyExc_TypeError,
 > +                    "No valid type in split argument");
 > }}}
 It has been covered by:
 {{{
 self.assertRaises(TypeError, self.name1.split, "wrong", 1)
 self.assertRaises(TypeError, self.name1.split, 1, "wrong")
 }}}

 > - Rcode_init: ext_code doesn't have to be long.
 Done.
 > - RRTTL_init: this doesn't seem to be correct:
 > {{{
 > -        } else if (PyArg_ParseTuple(args, "I", &i)) {
 > +        } else if (PyArg_ParseTuple(args, "k", &i)) {
 > }}}
 >  "k" omits overflow check, so it essentially has the same problem as
 >  "I".  One solution is to use "L" with long long (assuming our target
 >  system has it; this is probably acceptable.  We already rely on
 >  uint64_t etc, anyway).  Another solution is to ignore overflow in
 >  this specific case.
 I think ignore overflow is fine for this specific case, so I'll keep "k",
 is this ok?
 > - 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")
 }}}
 Hopefully I haven't missed anything.
 Thanks.

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