BIND 10 #838: "string iterator is not dereferencable" issue

[BIND 10 Development]

#838: "string iterator is not dereferencable" issue
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  ocean
  fdupont                            |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20110531
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   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 ocean):

 Replying to [comment:20 jinmei]:
 > Replying to [comment:18 ocean]:
 >
 > > > Finally, regarding the conversion of char to signed int for
 isspace():
 > > > I don't think checking it with >0 is the cleanest fix.  Even though
 I
 > > > generally want to avoid relying on casts, it seems to be one of the
 > > > (rare) cases where a cast makes most sense (and in this case the
 case
 > > > should be safe).
 > > >
 > > Consider the input is a value in range between [-128, 0), it will be
 mapped
 > > to [0x80, 0xFF) after {{{static_cast<unsigned char>(c)}}}, which are
 > > control and Latin-1 supplement in Unicode. For example, 0xA0 is
 > > defined as "non-break space", so whether isspace(0xA0) returned true
 or false
 > > depends on the implementation. I have tested on my pc that it returns
 0 for
 > > [0x80, 0xFF], but I'm afraid that future or other implementation may
 break it.
 >
 > Hmm, point taken.  On thinking about it with the rationale, I now tend
 > to agree that >0 is better than the naive cast.  If you like please
 > feel free to revert that part.  But in that case please leave some
 > comment about the implication.  I suspect it's not trivial.
 >
 I have reverted to the original fix and add some comments.

 > Also, with other fixes I suspect this bug cannot be triggered via our
 > current tests.  Please confirm that (or confirm I'm wrong:-) and if my
 > observation is correct, please add an explicit test that would
 > re-introduce this problem, confirm the new code without the additional
 > {{{> 0}}} check triggers it, and the additional check actually prevents
 that.
 I'm a little confused for this. For even we don't add the < 0 check, most
 of the {{{isspace()}}} implementation will just accept and return zero.
 It will only cause problem on windows platform in debug mode because it
 will
 do the check of

 {{{
  _ASSERTE((unsigned)(c + 1) <= 256);
 }}}

 This will be trigger in unit test of {{{HexTest::decodeMap}}}

 {{{
 input[1] = 128
 }}}

 since input[1] is a char type, it will be converted to -128 and then
 trigger it.

 > > > I've just committed my counter proposal based on the above comments
 > > > and pushed it.  I also made a couple of unrelated (but STL-related)
 > > > bugs in the test code.  Please check it (it's just a "proposal" and
 > > > I've pushed it so that we can easily share it.)  Also, please
 consider
 > > > adding more tests to base64 and hex tests that highlight the problem
 > > > as I did it for base32hex.
 > > >
 > > OK, I will check it.
 >
 > I've not seen you modify it, so are you okay with them (except the
 > cast for isspace())?
 Yes, I think it is ok.

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