[PATCH 1/2] Revert "lib: string: Fix strlcpy return value", fix callers

[Simon Glass]

Hi Matthias,

On Fri, 14 Jul 2023 at 05:25, Matthias Schiffer
<matthias.schiffer at ew.tq-group.com> wrote:
>
> Both the Linux kernel and libbsd agree that strlcpy() should always
> return strlen(src) and not include the NUL termination. The incorrect
> U-Boot implementation makes it impossible to check the return value for
> truncation, and breaks code written with the usual implementation in
> mind (for example, fdtdec_add_reserved_memory() was subtly broken).
>
> I reviewed all callers of strlcpy() and strlcat() and fixed them
> according to my understanding of the intended function.
>
> This reverts commit d3358ecc54be0bc3b4dd11f7a63eab0a2842f772 and adds
> related fixes.
>
> Fixes: d3358ecc54be ("lib: string: Fix strlcpy return value")
> Signed-off-by: Matthias Schiffer <matthias.schiffer at ew.tq-group.com>
> ---
>  board/amlogic/vim3/vim3.c    |  6 +++---
>  drivers/fastboot/fb_getvar.c |  2 +-
>  lib/string.c                 | 14 +++++++-------
>  test/lib/strlcat.c           |  4 ++--
>  4 files changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

nit below

>
> diff --git a/board/amlogic/vim3/vim3.c b/board/amlogic/vim3/vim3.c
> index fcd60ab1e05..8bdfb302f72 100644
> --- a/board/amlogic/vim3/vim3.c
> +++ b/board/amlogic/vim3/vim3.c
> @@ -104,8 +104,8 @@ int meson_ft_board_setup(void *blob, struct bd_info *bd)
>                 }
>
>                 /* Update PHY names (mandatory to disable USB3.0) */
> -               len = strlcpy(data, "usb2-phy0", 32);
> -               len += strlcpy(&data[len], "usb2-phy1", 32 - len);
> +               len = strlcpy(data, "usb2-phy0", 32) + 1;
> +               len += strlcpy(&data[len], "usb2-phy1", 32 - len) + 1;
>                 ret = fdt_setprop(blob, node, "phy-names", data, len);
>                 if (ret < 0) {
>                         printf("vim3: failed to update usb phy names property (%d)\n", ret);
> @@ -132,7 +132,7 @@ int meson_ft_board_setup(void *blob, struct bd_info *bd)
>                 }
>
>                 /* Enable PCIe */
> -               len = strlcpy(data, "okay", 32);
> +               len = strlcpy(data, "okay", 32) + 1;
>                 ret = fdt_setprop(blob, node, "status", data, len);
>                 if (ret < 0) {
>                         printf("vim3: failed to enable pcie node (%d)\n", ret);
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index dd3475e0a8b..d9f0f07b2bc 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -183,7 +183,7 @@ static void __maybe_unused getvar_has_slot(char *part_name, char *response)
>
>         /* part_name_wslot = part_name + "_a" */
>         len = strlcpy(part_name_wslot, part_name, PART_NAME_LEN - 3);
> -       if (len > PART_NAME_LEN - 3)
> +       if (len >= PART_NAME_LEN - 3)
>                 goto fail;
>         strcat(part_name_wslot, "_a");
>
> diff --git a/lib/string.c b/lib/string.c
> index ecea755f405..f2c61471288 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -116,20 +116,18 @@ char * strncpy(char * dest,const char *src,size_t count)
>   * of course, the buffer size is zero). It does not pad
>   * out the result like strncpy() does.
>   *
> - * Return: the number of bytes copied
> + * Return: strlen(src)
>   */
>  size_t strlcpy(char *dest, const char *src, size_t size)
>  {
> -       if (size) {
> -               size_t srclen = strlen(src);
> -               size_t len = (srclen >= size) ? size - 1 : srclen;
> +       size_t ret = strlen(src);
>
> +       if (size) {
> +               size_t len = (ret >= size) ? size - 1 : ret;

blank line here


>                 memcpy(dest, src, len);
>                 dest[len] = '\0';
> -               return len + 1;
>         }
> -
> -       return 0;
> +       return ret;
>  }
>  #endif
>
> @@ -191,6 +189,8 @@ char * strncat(char *dest, const char *src, size_t count)
>   * Compatible with *BSD: the result is always a valid NUL-terminated string that
>   * fits in the buffer (unless, of course, the buffer size is zero). It does not
>   * write past @size like strncat() does.
> + *
> + * Return: min(strlen(dest), size) + strlen(src)
>   */
>  size_t strlcat(char *dest, const char *src, size_t size)
>  {
> diff --git a/test/lib/strlcat.c b/test/lib/strlcat.c
> index a0ec037388b..d8453fe78e2 100644
> --- a/test/lib/strlcat.c
> +++ b/test/lib/strlcat.c
> @@ -43,11 +43,11 @@ static int do_test_strlcat(struct unit_test_state *uts, int line, size_t align1,
>                 s2[i] = 32 + 23 * i % (127 - 32);
>         s2[len2 - 1] = '\0';
>
> -       expected = len2 < n ? min(len1 + len2 - 1, n) : n;
> +       expected = min(strlen(s2), n) + strlen(s1);
>         actual = strlcat(s2, s1, n);
>         if (expected != actual) {
>                 ut_failf(uts, __FILE__, line, __func__,
> -                        "strlcat(s2, s1, 2) == len2 < n ? min(len1 + len2, n) : n",
> +                        "strlcat(s2, s1, n) == min(len2, n) + len1",
>                          "Expected %#zx (%zd), got %#zx (%zd)",
>                          expected, expected, actual, actual);
>                 return CMD_RET_FAILURE;
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
>

Regards,
Simon