Re: [PATCH] SBuf c-string comparisons

From: Amos Jeffries <squid3_at_treenet.co.nz>
Date: Wed, 16 Apr 2014 02:55:35 +1200

On 15/04/2014 7:35 a.m., Alex Rousskov wrote:
> On 04/14/2014 11:07 AM, Amos Jeffries wrote:
>> On 15/04/2014 2:56 a.m., Alex Rousskov wrote:
>>> On 04/14/2014 04:49 AM, Amos Jeffries wrote:
>>>> I've added a unit test to catch the rare \0 mid-string case I spoke of:
>>>> * SBuf("foo\0test").compare("foo", 9);
>>>>
>>>> It works fine up to the point N(4) > strlen("foo"). After that point our
>>>> function returns 1 to indicate that the SBuf is a longer string, whereas
>>>> strncmp/strncasecmp return 0 up to infinity.
>>>
>>> Yes, this is related to the large-n handling bug I keep talking about.
>>> IMO, this must be fixed as previously discussed: C-string API should not
>>> look past the first null character.
>>>
>>>
>>>> The code as presented earlier copes with the weirdness fine.
>>>
>>>
>>> AFAICT, the latest posted patch accesses non-existent c-string bytes
>>> under certain conditions (large n, large SBuf with trailing null
>>> characters, short s matching the c-string in SBuf). Do you agree? If you
>>> do agree, please post a fixed patch. If not, I would have to spend time
>>> writing a test case to prove my point against the last patch posted.
>>
>> I agree.
>
> Great.
>
>
>> Attached patch implements what we agreed on in IRC.
>
> Please note that we discussed only one aspect of the code; there was no
> blueprint on how to get everything working, and there are many ways to
> do that.
>
>
>> It produces wrong return value in two case states. Marked with "BUG 1"
>> and "BUG 2" in the patch.
>
>
>> + // BUG 1: when length() < n - buffer overruns on buf().
>
> byteCompareLen cannot exceed length() so there should not be buf()
> overruns. However, there are other bugs as discussed below.
>
>
>> + // BUG 2: when length() == strlen(s) < n, no terminator to match against in buf()
>
>
> Yes, the lack of buf() terminator still needs to be fixed for all cases
> where the terminator would have been used by a c-string-based
> implementation. Please see the IRC log for some specific suggestions.

The attached patch passes all the tests including \0 embeded in the strings.
 It also avoids the s[] access by using strlen(s) != byteCompareLen.

Amos

Received on Tue Apr 15 2014 - 14:55:43 MDT

This archive was generated by hypermail 2.2.0 : Tue Apr 15 2014 - 12:00:30 MDT