[PATCH v7] Implement strlcpy, strlcat [BZ #178]

[Paul Eggert]

Florian Weimer wrote:

> I got conflicting advice (on-list and off-list)

It might be helpful to see a summary of the off-list advice, as it appears to 
have influenced the proposed patch without leaving much trace of what was 
discussed. Did the off-list advisors consider the points raised on libc-alpha?

> One suggestion is to make a non-terminated buffer undefined, but that breaks
> the snprintf analogy for size 0 inputs.

Sorry, what analogy is that? snprintf does not concatenate to a buffer directly. 
What is the practical use case here? How does the use case ignore the principle 
that strlcpy should null-terminate its output?

> Another option is to leave the
> array untouched (which is what I implemented and documented).

It is OK to *implement* it that way. But *documenting* this weird corner case 
would raise a can of worms. For example, it would mean strlcat's destination 
might not be a string, which would mean that the proposed documentation's talk 
about behavior being "undefined if the strings overlap" is not worded correctly. 
These problems are discussed in more detail below.

> I did not pick up
> the full criticism because glibc has repeatedly demonstrated that its
> recommended approach to string processing is rather error-prone, so we
> should not judge other approaches too harshly.

Fair enough. However, the wording in the latest proposal has the opposite 
problem: when documenting strncpy it inserts a seemingly-partisan suggestion to 
use strlcpy instead. Furthermore, its strlcpy and strlcat sections both promote 
these functions as ways to avoid buffer overruns even though strlcpy and strlcat 
are not on their face more effective at that than the other truncation 
functions. The strlcpy/strlcat documentation should be like the other truncation 
functions, and defer to the general wording about buffer overruns that is 
already in that section of the manual, rather than contain special advocacy for 
that particular approach.

Here are some other (in some cases, related) issues with the documentation 
change just proposed:

* It starts off by saying strcpy copies data that is "including a terminating 
null byte" but this is not true if SIZE is zero.

* The paragraph that specifies strlcpy semantics "If @var{size} is less than or 
equal to the the length of the string" is incorrect when SIZE is zero. The 
wording should be something more like "If @var{size} is nonzero and less than or 
equal to the the length of the string".

* It says strlcpy's behavior is undefined if the strings overlap, but if SIZE is 
zero there is no destination string. It should say the behavior is undefined if 
SIZE is nonzero and the strings overflap.

* When giving the differences between strncpy and strlcpy, it does not mention 
that strlcpy requires the source string to be null-terminated, nor that it 
computes the source string's length. Similarly when giving the differences 
between strncat and strlcat.

* The initial sentence describing strlcat assumes that the destination is a 
string, which is not necessarily the case.

* Typo: "If array @var{to}" should be "If the array @var{to}".

* The phrase "the length of the untruncated string" does not apply if the 
destination is not a string.

* On multiple occasions, the description for strlcat talks about "the length of 
the string @var{to}" without making it clear whether it is talking about the old 
length of TO, or about the new length.

* It does not mention that truncation in strlcat occurs when the result is 
greater than or equal to the size.

* It says that strlcat's behavior is undefined if the strings overlap, but the 
destination is not necessarily a string, and the description of overlap does not 
make it clear whether it's talking about the old destination string or array, 
the new destination string or array, or the modified part of the destination.

* When giving the differences between strncat and strlcat, it does not mention 
that strlcat ensures that an already-null-terminated destination stays 
null-terminated.

* The documentation does not state that it is OK for the destination pointer to 
be NULL if SIZE is zero.

* In some places the documentation says "length" (not counting the null 
terminator) when it is referring to size (counting the null terminator).


Fixing these problems would take some work and would complicate the 
documentation. Instead, I'll update my earlier patch (which leaves these weird 
corner case behavior undefined; this is considerably simpler), by removing the 
"full criticisms" that you mentioned and by adopting many of the other textual 
changes you made. This revised doc patch is shorter than before, and is 
compatible with the most-recently-proposed implementation.

As before I hope that the strlcpy/strlcat code changes do not go in, but if they 
do go in they will need proper documentation.

The first attachment is a patch relative to master; the second is a diff of 
string.texi compared to the text you just proposed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Document-strlcpy-strlcat.patch
Type: text/x-diff
Size: 4934 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20151230/8d2cad9e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: string.texi.patch
Type: text/x-diff
Size: 5939 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20151230/8d2cad9e/attachment-0001.bin>