Re: [PATCH] SBuf c-string comparisons

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.

Thank you,

Alex.
Received on Mon Apr 14 2014 - 19:35:49 MDT