Re: [PATCH] SBuf c-string comparisons

On 16/04/2014 5:07 a.m., Alex Rousskov wrote:
> On 04/15/2014 08:55 AM, Amos Jeffries wrote:
>
>> The attached patch passes all the tests including \0 embeded in the strings.
>
> If I am reading the code correctly, there is a new bug:
>
>> It also avoids the s[] access by using strlen(s) != byteCompareLen.
>
>> + if (byteCompareLen < n && strlen(s) != byteCompareLen)
>
> s is guaranteed to be 0-terminated when n == npos only. For other cases,
> we do not have such a guarantee and, hence, cannot use strlen(). Using
> strlen(s) when n is not npos may lead to s overreads.

I don't see any way around this without hand-crafing a full byte-by-byte
strncmp replacement. Which would appear to be overkill for this
transitional method (only needed until all I/O and globals/constants are
SBuf based).

I will comment the new methods with "Requires S to be null-terminated.".
Since the default n=npos case also requires that it may as well be a
constant requirement.
 NP: callers playing with un-termianted C-strings in old Squid code are
usually broken as-is. Not to say there aren't any though.

(NP: also adding the compare n=npos default which I seem to have missed
out still.)

>
> Please add the following test case, just in case (it does not expose any
> bugs right now AFAICT):
>
> testComparisonStdOneWay("", "");

Done.

>
> Please note that I get a warning when compiling your patch on a 64 bit
> OS because our MemBlob/SBuf::size_type is smaller than size_t:
>
>> tests/testSBuf.cc: In member function ‘void testSBuf::testComparisons()’:
>> tests/testSBuf.cc:360: error: no matching function for call to ‘min(uint32_t, size_t)’
>
> Did we consciously made MemBlob/SBuf::size_type 32 bit for some reason
> or was it an accident?

I dont recall exactly.

Casting this strlen() to size_type.

Is this acceptible to you after those minor changes?

Amos
Received on Wed Apr 16 2014 - 06:06:04 MDT