Re: [PATCH 1/2] Implement strlcpy and strlcat [BZ #178]

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 05/04/23 08:20, Florian Weimer via Libc-alpha wrote:
> These functions are about to be added to POSIX, under Austin Group
> issue 986.
> 
> 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.

This is not a full review, just nits I found skimming the patch.

> diff --git a/NEWS b/NEWS
> index 83d082afad..60b40fabcf 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -21,6 +21,9 @@ Major new features:
>  
>  * PRIb* and PRIB* macros from C2X have been added to <inttypes.h>.
>  
> +* The strlcpy and strlcat functions have been added.  They are derived
> +  from OpenBSD, and are expected to be added to a future POSIX versions.
> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * In the Linux kernel for the hppa/parisc architecture some of the
> @@ -223,6 +226,8 @@ Major new features:
>  
>    The LoongArch ABI is 64-bit little-endian.
>  
> +* The functions strlcpy and strlcat have been added.

This NEWS entry seems to be a left-over from refactoring.

> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * Support for prelink will be removed in the next release; this includes
> diff --git a/debug/Makefile b/debug/Makefile
> index 52f9a7852c..404f93002f 100644
> --- a/debug/Makefile
> +++ b/debug/Makefile
> @@ -31,6 +31,7 @@ headers	:= execinfo.h
>  routines  = backtrace backtracesyms backtracesymsfd noophooks \
>  	    memcpy_chk memmove_chk mempcpy_chk memset_chk stpcpy_chk \
>  	    strcat_chk strcpy_chk strncat_chk strncpy_chk stpncpy_chk \
> +	    strlcpy_chk strlcat_chk \
>  	    sprintf_chk vsprintf_chk snprintf_chk vsnprintf_chk \
>  	    printf_chk fprintf_chk vprintf_chk vfprintf_chk \
>  	    gets_chk chk_fail readonly-area fgets_chk fgets_u_chk \
> diff --git a/debug/Versions b/debug/Versions
> index a6628db356..94dfa5f428 100644
> --- a/debug/Versions
> +++ b/debug/Versions
> @@ -58,6 +58,10 @@ libc {
>    GLIBC_2.25 {
>      __explicit_bzero_chk;
>    }
> +  GLIBC_2.38 {
> +    __strlcat_chk;
> +    __strlcpy_chk;
> +  }
>    GLIBC_PRIVATE {
>      __fortify_fail;
>    }
> diff --git a/debug/strlcat_chk.c b/debug/strlcat_chk.c
> new file mode 100644
> index 0000000000..be6e1942af
> --- /dev/null
> +++ b/debug/strlcat_chk.c
> @@ -0,0 +1,32 @@
> +/* Fortified version of strlcat.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <string.h>
> +#include <memcopy.h>

It does not require this header.

> +
> +/* Check that the user-supplied size does not exceed the
> +   compiler-determined size, and then forward to strlcat.  */
> +size_t
> +__strlcat_chk (char *__restrict s1, const char *__restrict s2,
> +	       size_t n, size_t s1len)
> +{
> +  if (__glibc_unlikely (s1len < n))
> +    __chk_fail ();
> +
> +  return __strlcat (s1, s2, n);
> +}
> diff --git a/debug/strlcpy_chk.c b/debug/strlcpy_chk.c
> new file mode 100644
> index 0000000000..0137886e91
> --- /dev/null
> +++ b/debug/strlcpy_chk.c
> @@ -0,0 +1,32 @@
> +/* Fortified version of strlcpy.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <string.h>
> +#include <memcopy.h>

Ditto.

> diff --git a/string/strlcat.c b/string/strlcat.c
> new file mode 100644
> index 0000000000..5b64072004
> --- /dev/null
> +++ b/string/strlcat.c
> @@ -0,0 +1,61 @@
> +/* Append a null-terminated string to another string, with length checking.
> +   Copyright (C) 2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#undef strlcat
> +
> +size_t
> +__strlcat (char *__restrict dest, const char *__restrict src, size_t size)
> +{
> +  size_t src_length = strlen (src);
> +
> +  /* Our implementation strlcat supports dest == NULL if size == 0
> +     (for consistency with snprintf and strlcpy), but strnlen does
> +     not, so we have to cover this case explicitly.  */
> +  if (size == 0)
> +    return src_length;
> +
> +  size_t dest_length = __strnlen (dest, size);
> +  if (dest_length != size)
> +    {
> +      /* Copy at most the remaining number of characters in the
> +	 destination buffer.  Leave for the NUL terminator.  */
> +      size_t to_copy = size - dest_length - 1;
> +      /* But not more than what is available in the source string.  */
> +      if (to_copy > src_length)
> +	to_copy = src_length;
> +
> +      char *target = dest + dest_length;
> +      memcpy (target, src, to_copy);
> +      target[to_copy] = '\0';
> +    }
> +
> +  /* 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");

Ins't garanteed by POSIX?