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

[Alex Rousskov]

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.


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


> +1 if you update the patch to do the above. +0 otherwise  - ie. go ahead
> with commit if you want, but I'd rather see the fixed caller(s) fixed to
> use a more long-ish term fix.

Understood. We disagree regarding the "more long-ish" fix desirability.


Thank you,

Alex.