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

[Amos Jeffries]

On 27/04/17 10:58, Alex Rousskov wrote:
> Hello,
>
>      I accidentally discovered that our xstrndup() documentation lies and
> its implementation is dangerously nothing like the standard one. The
> attached patch fixes documentation and three callers. Please see the
> patch preamble for details, including lack of ESI testing.
>
> The attached patch is NOT the right long-term solution because folks
> will, naturally, continue to use xstrndup(SQUID) as if it is strndup(3),
> leading to new caller bugs.
>
> Unfortunately, just fixing xstrndup() implementation and callers is not
> enough! If we fix xstrndup() implementation (and existing callers) in
> v5, then any new caller code ported to an older Squid version may break
> in subtle ways (because that old Squid version assumes 0-termination and
> requires "+1s" to work correctly). No human will be able to spot such
> porting errors reliably.
>
> Thus, it looks like we have to rename xstrndup(), forcing porting humans
> to notice the change in the API and adjust the callers. I can suggest
> xstrndupe() or bufDupe() as the new name.
>
> Would anybody volunteer to fix this for the long-term?

In the long term the code using it will be refactored use SBuf or 
std::string anyway.  I think it would be best if anyone taking this 
request up try to do that migration early rather than trying to fix what 
is just a portability hack - and for a convenience function at that.

If that is too hard, making callers do the allocate bit themselves and 
xstrncpy() instead should be a good medium-term workaround.


> Thank you,
>
> Alex.
> 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() ?

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

Amos