[squid-dev] [PATCH] xstrndup has to go

[Alex Rousskov]

On 04/30/2017 11:26 PM, Amos Jeffries wrote:
> On 30/04/17 04:17, Alex Rousskov wrote:
>> On 04/29/2017 02:33 AM, Amos Jeffries wrote:
>>
>>> If that is too hard, making callers do the allocate bit themselves and
>>> xstrncpy() instead should be a good medium-term workaround.
>> In general, it is impossible to correctly replace strndup() with a
>> simple allocate and strncpy() pair. I bet this impossibility is why
>> strndup() exists! strndup() is not a convenience function, but an
>> irreplaceable tool in a performance toolbox.
>>
>> Why is it impossible in general? Because you do not know how many bytes
>> to allocate (that number may be much smaller than n) and you cannot use
>> strlen() to find out because the array may not be 0-terminated. You need
>> strnlen(), which is usually not available where strndup() is not.

> We don't need to solve the general case. 

By "general", I meant "an average or typical use case in Squid code".
Some callers can probably allocate correctly for themselves, but some
cannot (or will break after surrounding code changes). The need for
strndup() is genuine and will remain for the foreseeable future.


>>>> P.S. The OutOfBoundsException use case code fixed by this patch needs a
>>>> different fix (the one that does not allocate memory twice):
>>>>
>>>>     xstrndup(explanatoryText.rawContent(), explanatoryText.length());
>>>>
>>>> but we cannot do the above until xstrndup() implementation is fixed.
>>> Not even by moving that caller to xmalloc() for itself and use
>>> xstrncpy() ?

>> xmalloc()+xstrncpy() is not "doing the above" it is doing something
>> else. I only claimed that we "cannot do the above" (literally). There
>> are other ways to achieve the same result, of course. None of those
>> other ways that I can think of (in the current code context) are better
>> than the proposed double-copy IMO.
>>
>> Sure, it is possible to do xmalloc()+memcpy() in this context instead of
>> xstrndup(), but, in this context, that would be arguably worse than
>> copying twice because it would replace a clear high-level code with
>> risky low-level manipulations on an error handling path.
>>
>> If strdup(buf.c_str()) pattern appears often, we should add
>> SBuf::c_str_dup() that does xmalloc()+memcpy().

> strdup != strndup.

I know. I meant exactly what I said -- "strdup" (and "If").


> My question was about that one use-case you highlighted as example, not
> the general solution.

I know. I believe I have answered your question -- yes, we can xmalloc()
and strncpy() in that case, but we do not want to do that.

And it was not highlighted as an "example". I mentioned it (in P.S.!)
only because the current fix there looks wrong and is slow. I wanted to
avoid a discussion of why I could not write a more efficient code there.
I naturally regret mentioning that at all now!


> over-allocation is not a huge problem
> and whatever exception handler catches this will end with deallocation
> quite quickly.

Agreed. We should not do xmalloc+strncpy there because we should not
replace a clear high-level code with risky low-level manipulations on an
error handling path. Overallocation is not a problem in this case.


> If this exception handler is stable enough now to be worth optimizing 

OT: In general, exception handlers performance is not a primary concern
because exceptions should not be used for something that is expected to
happen very often on a performance-sensitive path.


Cheers,

Alex.