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:
                Component:           |  Sprint-20110419
  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:15 zzchen_pku]:

 First off: I made a minor cleanup and pushed it to the branch:
 21718d6 [trac363] cleanup: removed a white space at EOL.

 > > 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).
 > Message_init, Message_clear, MessageRenderer_setCompressMode already
 have value check, for example:
 {{{
 if (i == Message::PARSE) {
     self->message = new Message(Message::PARSE);
     return (0);
 } else if (i == Message::RENDER) {
     self->message = new Message(Message::RENDER);
     return (0);
 } else {
     PyErr_SetString(PyExc_TypeError, "Message mode must be Message.PARSE
 or Message.RENDER");
     return (-1);
 }
 }}}

 Either you didn't take my point or I was not clear enough.  What I meant
 was that it would make more sense to update, e.g., MessageImpl::init() so
 that it would reject invalid "mode":

 {{{
 MessageImpl::init() {
     if (mode_ != Message::PARSE || mode_ != Message::RENDER) {
         isc_throw(something);
     }
     ...
 }
 }}}

 and remove the checks in the python wrapper (like the above if-else if-
 else).

 And, such updates would be beyond the scope of this ticket, and it would
 make more sense to leave them to a separate ticket.

 > > These don't seem to be addressed in that it still checks 0xffff (and
 > > it still returns 'overflow' for a negative 'pos', see above).
 > After checking python doc again, I think ValueError and IndexError are
 more suitable for clarifying the exceptions, how do you fell?

 I don't have a strong opinion, but after re-checking the python document,
 I tend to agree that IndexError is a bit better than ValueError for the
 cases you chaged.  So the latest code is okay for me on this point.

 One more thing.  I suspect the limitation in
 MessageRenderer_setLengthLimit() is too restrictive because the
 corresponding libdns++ backend expects size_t.  I'd simply reject negative
 value.

 > > 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.
 > Most of them just check the possible range for corresponding type, which
 should be ok, did you find something with too tight checking?

 From a quick eye-grep, no, but the point is that it would be safer to make
 it sure by tests.  Boundary conditions are one of the common points where
 human beings make mistakes.

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