[PATCH v4] Implement strlcpy [BZ #178]

[Florian Weimer]

On 10/29/2015 10:50 PM, Paul Eggert wrote:
> I'd rather we didn't add strlcpy to glibc, for reasons already
> discussed. If consensus goes against me, the patch still needs some
> work. I don't recall the details of our previous discussion; at the risk
> of reraising old issues here are some comments:
> 
> * If strlen(SRC) < DESTLEN, the documentation should clearly state that
> the contents of the bytes DEST[strlen(SRC) + 1] through DEST[DESTLEN -
> 1] are preserved.  The current documentation can be plausibly read that
> way, but it's not explicit.

I don't want to give any guarantees other implementations do not provide.

> * The proposed documentation can easily be misread as implying that
> strlcpy (DEST, SRC, DESTLEN) does O(DESTLEN) work, which is incorrect:
> strlcpy always does O(strlen(SRC)) work.  This point should be made
> clearly.  This is not merely a performance issue: it should be made
> crystal-clear that SRC must be null-terminated even if the source's
> trailing null byte is way after the bytes that strlcpy copies to DEST.

I added a sentence about null-termination just before the comment about
undefined behavior:

The string @var{from} must be null-terminated even if its length exceeds
that of the destination buffer.  The behavior of @code{strlcpy} is
undefined if the strings overlap or if the source or destination are
null pointers.

> * strlcpy's name should be prefixed by '__' by default. The names
> strlcpy and strlcpy_chk are both in the implementation namespace, and
> it's odd to have one without leading underscores and the other with
> them. I suggest a more cautious approach, in which both names are
> prefixed with '__' and unprefixed strlcpy is provided to the user only
> when GNU or BSD extensions are requested via _GNU_SOURCE etc.

This is what the patch always has done, the declaration is guarded by
__USE_MISC.

> This is less likely to break existing applications.

We'll see.  Compile-time breakage is one thing, but there could also be
incompatible interposed implementations.  For example, there are
implementations which specify int instead of size_t for the lengths.

> * strlcpy's implementation should use memmove instead of memcpy. The
> main motivations for strlcpy are safety and consistency and avoiding
> errors.  memmove obviously supports these goals better than memcpy
> does.  Efficiency is not a major concern with strlcpy (if it were,
> strlcpy wouldn't be O(strlen(SRC))).

The original OpenBSD implementation does not handle all overlapping
buffers in the same way memmove would.  Most BSDs now specify the
restrict qualify in the function prototype and documentation.

The existing *_chk functions do not check for overlapping inputs, and
they even have restrict pointer arguments, so such checks could be
futile anyway.  If we want to change this approach, I think this should
be a separate discussion.  (Right now, there is memstomp for this.)

>> +Not guaranteeing null termination and always overwriting the entire
>> +destination buffer makes @code{strncpy} rarely useful, but this behavior
>> +is specified by the @w{ISO C} standard.  See @code{strlcpy} below for an
>> +alternative.
> This quote is confusing, as it imples that strlcpy guarantees null
> termination, which strlcpy does not. I suggest rewording it to something
> like the following: "Often @code{strncpy} is not what you want, because
> it does not null-terminate the destination if the destination is smaller
> than the source, it always overwrites the entire destination buffer, it
> may truncate the destination, and it has undefined behavior if the
> source and destination overlap.  For alternatives, see the documentation
> below for @code{strlcpy}."

What about this?

Not guaranteeing null termination and always overwriting the entire
destination buffer makes @code{strncpy} rarely useful, but this behavior
is specified by the @w{ISO C} standard.  See @code{strlcpy} below for an
alternative which null-terminates the destination string as long
as the destination buffer does not have length zero.

> We can pair this with similar phrasing under strlcpy -- something like
> the following perhaps (this wording assumes strlcpy is changed to use
> memmove):"Often @code{strlcpy} is not what you want, because it does not
> null-terminate the destination if the destination's size is zero,it can
> leave junk data behind in the destination, it can do useless work when
> the source is long and the destination short, and it may truncate the
> destination.  Although one alternative is @code{strncpy}, it is usually
> better to use dynamic memory allocation and functions such as
> @code{strdup} or @code{asprintf} to construct strings."

The submitted patch already had this note directly following the bit you
criticized.

@strong{Note:} GNU programs should not use statically sized buffers for
storing strings.  @xref{Semantics, , Writing Robust Programs, standards,
The GNU Coding Standards}.  Instead of using @code{strlcpy}, it is
usually better to use dynamic memory allocation and functions such as
@code{strdup} or @code{asprintf} to construct strings.

I think this recommendation is clear enough.

Florian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-strlcpy-BZ-178.patch
Type: text/x-patch
Size: 29178 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20151102/ca4f602b/attachment.bin>