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

[Amos Jeffries]

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. We need to solve the handful of 
actual uses in Squid code, then add the risky function to the forbidden 
list which is enforced by sourcemaintenance.sh.


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

For strdup() replacement we absolutely can do strlen(). It requires 
nul-termination explicitly.

The xstrndup(F.c_str(), ...) pattern appears exactly once. In this 
exception handler under discussion.

My question was about that one use-case you highlighted as example, not 
the general solution. It appears to me that while there is some 
variation caused by the message contents being filename and line numbers 
- they are still short enough that over-allocation is not a huge problem 
and whatever exception handler catches this will end with deallocation 
quite quickly.

The relevant lines being these:
  src/sbuf/Exceptions.cc:    SBuf explanatoryText("OutOfBoundsException");
  src/sbuf/Exceptions.cc:        explanatoryText.appendf(" at line %d", 
aLineNo);
  src/sbuf/Exceptions.cc:        explanatoryText.appendf(" in file %s", 
aFileName);
  src/sbuf/Exceptions.cc:    explanatoryText.appendf(" while accessing 
position %d in a SBuf long %d",
  src/sbuf/Exceptions.cc:    message = 
xstrndup(explanatoryText.c_str(),explanatoryText.length());

Over-allocation appears to only be possible if/when the aFileName 
contains null characters internally. Then rawContent() instead of 
c_str() drops the extra allocation.

xstrncpy() still halts on the input \0 character if it happens early 
regardless of whether c_str() or rawContent() is used to supply the 
input. And passing it length()+1 safely handles non-terminated input 
without an length()+1 byte allocated because xstrncpy ends on the n'th 
byte for terminating *dst instead of copying it from input.


PS. appendf() is probably causing a re-allocation as well to grow the 
MemBlob. So we are looking at probably three allocations here. If this 
exception handler is stable enough now to be worth optimizing we should 
probably also look at ways to reserve enough memory for explanatoryText 
before putting anything into it.

>> +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.
>
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev