On Fri, 2023-07-14 at 13:24 +0200, Matthias Schiffer 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.schif...@ew.tq-group.com>
Ping~ strlcpy and strlcat are now also in glibc and might be added to POSIX, so it would be great if we could get the U-Boot implementation to match the common behaviour. Regards, Matthias > --- > 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(-) > > 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; > 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/