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

[BIND 10 Development]

#838: "string iterator is not dereferencable" issue
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  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 jinmei):

 Replying to [comment:22 ocean]:

 > > > 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.

 Okay, but the comment doesn't seem to be sufficient to me.  It doesn't
 explain why we cannot simply cast it to unsigned char.  Also, it
 doesn't match the implementation: while the comment says "larger than
 0", the test is ">= 0".  Further, I suspect >= 0 could be harmful for
 systems with unsigned char.

 I'd note another minor style point.  The added parentheses seem just
 redundant to me.  While it's true we sometimes be redundant
 intentionally (for example, we always add {} even when they can
 omitted), and in some cases it might be rather preferred than relying
 on implicit priorities, but "((*base_) > 0)" seems way too awkward.
 It also makes the policy of when to use () inconsistent, even within
 that particular while condition.

 Finally, I just noticed the opening curly brace should be in the same
 line as that for while, according to our common style guideline.

 I'm attaching a diff that fixes all of the above.  Please check it and
 apply if you agree.

 > > 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.

 Ah, okay, I missed that test.  I incorrectly thought this was
 triggered as a side effect of overrun.

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