On 2023-04-05 04:20, Florian Weimer via Libc-alpha wrote:

> The fortified strlcat implementation does not raise SIGABRT if the
> destination buffer does not contain a null terminator, it just
> inheritis the non-failing regular strlcat behavior.  Maybe this
> should be changed to catch secondary overflows because the string
> appears longer than the buffer.

Shouldn't it should work like fortified strcat, which raises SIGABRT if 
the destination is not null-terminated? (Admittedly this isn't important.)


> +* The strlcpy and strlcat functions have been added.  They are derived
> +  from OpenBSD, and are expected to be added to a future POSIX versions.

versions → version

Please see the attached patch for a fix for this.


> +* The functions strlcpy and strlcat have been added.

That's a duplicate in NEWS.


> +extern __typeof (strlcpy) __strlcpy;
> +libc_hidden_proto (__strlcpy)
> +extern __typeof (strlcat) __strlcat;
> +libc_hidden_proto (__strlcat)

Glibc shouldn't call these functions internally, so let's not export 
them to elsewhere in glibc.

manual/maint.texi needs changing too (see where it discusses strncpy).


> --- a/manual/string.texi
> +++ b/manual/string.texi

When the manual discusses strcat/strncat's O(N**2) drawback it should do 
the same for strlcat.


> +This function is similar to @code{strcpy}, but copies at most
> +@var{size} bytes from the string @var{from} into the destination
> +array @var{to}, including a terminating null byte.

This should mention draft POSIX's recommendation that the caller should 
ensure that SIZE has room for the terminating null. Also, it shouldn't 
say it copies at most SIZE bytes because it's really at most (MAX (1, 
SIZE) - 1). There are several errors of this sort later on - for 
example, the overlapping move constraint doesn't apply to the null 
terminator. I think I caught the errors in the attached patch but 
another pair of eyes wouldn't hurt.

Also, let's use similar wording for strlcpy and strlcat to avoid having 
the readers think that there are differences when there aren't any.


> +The return value @var{result} of @code{strlcpy} is the length of the
> +string @var{from}.  This means that @samp{@var{result} >= @var{size}} is
> +true whenever truncation occurs.

It's not just "whenever" (i.e., if); it's if and only if.


> +The behavior of @code{strlcpy} is undefined if @var{size} is zero, or if
> +the source string and the first @var{size} bytes of the destination
> +array overlap.

Actually, the behavior is well-defined if SIZE is zero. It's also 
well-defined in some cases when the first SIZE bytes overlap: the only 
constraint is that the region copied from cannot overlap with the region 
copied to (in both cases, not counting any terminating null). However, 
behavior is undefined if either pointer is null, or if the destination 
array isn't big enough.

The description should allude to the truncation-warning discussion below.

There are similar issues for the strlcat doc, which the attached patch 
also attempts to fix.


> +  /* If the sum wraps around, we have more than SIZE_MAX + 2 bytes in
> +     the two input strings (including both null terminators).  If each
> +     byte in the address space can be assigned a unique size_t value
> +     (which the static_assert checks), then by the pigeonhole
> +     principle, the two input strings must overlap, which is
> +     undefined.  */
> +  _Static_assert (sizeof (uintptr_t) == sizeof (size_t),
> +		  "theoretical maximum object size covers address space");

Change the "==" to "<=" (since that's what is meant here) and clarify 
the comment which is a bit confusing. Better yet, let's omit this and 
all related code and go with the OpenBSD implementation.


> +	  /* Copy the leading portion of the string.  The last
> +	     character is subsequently overwritten with the NUL
> +	     terminator, but the destination size is usually a
> +	     multiple of a small power of two, so writing it twice
> +	     should be more efficient than copying an odd number of
> +	     bytes.  */
> +	  memcpy (dest, src, size);
> +	  dest[size - 1] = '\0';

This micro-optimization is incorrect, as it's valid for dest to equal 
src + size - 1, and that means the memcpy overlaps which is undefined. 
Change it to memcpy (dest, src, size - 1) and lose the comment. Or 
change it to memmove and lengthen the comment. Or better yet, get rid of 
all code like this (there are other instances), and use the simple 
OpenBSD implementation which will more likely match what callers expect 
(in the rare cases where the behaviors differ) and will possibly be 
faster despite not using memcpy.


Proposed patch for NEWS and manual attached; the other problems remain 
unfixed.