[PATCH 1/2] Revert "lib: string: Fix strlcpy return value", fix callers
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@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(-)
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;
strlcat returns min(strlen(dest), count)+strlen(src). Make u16_strlcat's behaviour the same for consistency.
Fixes: eca08ce94ceb ("lib/charset: add u16_strlcat() function") Signed-off-by: Matthias Schiffer matthias.schiffer@ew.tq-group.com --- lib/charset.c | 8 ++++---- test/unicode_ut.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/lib/charset.c b/lib/charset.c index b1842755eb1..5e4c4f948a4 100644 --- a/lib/charset.c +++ b/lib/charset.c @@ -444,14 +444,14 @@ u16 *u16_strdup(const void *src)
size_t u16_strlcat(u16 *dest, const u16 *src, size_t count) { - size_t destlen = u16_strlen(dest); + size_t destlen = u16_strnlen(dest, count); size_t srclen = u16_strlen(src); - size_t ret = destlen + srclen + 1; + size_t ret = destlen + srclen;
if (destlen >= count) return ret; - if (ret > count) - srclen -= ret - count; + if (ret >= count) + srclen -= (ret - count + 1); memcpy(&dest[destlen], src, 2 * srclen); dest[destlen + srclen] = 0x0000;
diff --git a/test/unicode_ut.c b/test/unicode_ut.c index b27d7116b9e..a9356e2b60d 100644 --- a/test/unicode_ut.c +++ b/test/unicode_ut.c @@ -808,12 +808,12 @@ static int unicode_test_u16_strlcat(struct unit_test_state *uts) /* dest and src are empty string */ memset(buf, 0, sizeof(buf)); ret = u16_strlcat(buf, &null_src, sizeof(buf)); - ut_asserteq(1, ret); + ut_asserteq(0, ret);
/* dest is empty string */ memset(buf, 0, sizeof(buf)); ret = u16_strlcat(buf, src, sizeof(buf)); - ut_asserteq(5, ret); + ut_asserteq(4, ret); ut_assert(!unicode_test_u16_strcmp(buf, src, 40));
/* src is empty string */ @@ -821,14 +821,14 @@ static int unicode_test_u16_strlcat(struct unit_test_state *uts) buf[39] = 0; memcpy(buf, dest, sizeof(dest)); ret = u16_strlcat(buf, &null_src, sizeof(buf)); - ut_asserteq(6, ret); + ut_asserteq(5, ret); ut_assert(!unicode_test_u16_strcmp(buf, dest, 40));
for (i = 0; i <= 40; i++) { memset(buf, 0xCD, (sizeof(buf) - sizeof(u16))); buf[39] = 0; memcpy(buf, dest, sizeof(dest)); - expected = 10; + expected = min(5, i) + 4; ret = u16_strlcat(buf, src, i); ut_asserteq(expected, ret); if (i <= 6) {
On Fri, Jul 14, 2023 at 01:24:51PM +0200, Matthias Schiffer wrote:
strlcat returns min(strlen(dest), count)+strlen(src). Make u16_strlcat's behaviour the same for consistency.
Fixes: eca08ce94ceb ("lib/charset: add u16_strlcat() function") Signed-off-by: Matthias Schiffer matthias.schiffer@ew.tq-group.com
Applied to u-boot/next, thanks!
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.schiffer@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;@@ -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;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);
On Wed, Aug 02, 2023 at 12:06:26PM +0200, Matthias Schiffer wrote:
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.schiffer@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.
I intend to pull this in to -next when I open it post -rc2, thanks.
Hi Matthias,
On Fri, 14 Jul 2023 at 05:25, Matthias Schiffer matthias.schiffer@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@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@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
On 7/14/23 07:24, 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.schiffer@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(-)
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;@@ -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;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);
I remembered that something was off with this patch, but I never went back to fix it...
Reviewed-by: Sean Anderson sean.anderson@seco.com
On 14/07/2023 13.24, 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).
So while we're fixing non-standard string function behaviour, can we _please_ finally fix strncpy() so it follows C/POSIX?
https://lore.kernel.org/u-boot/52bc92d4-8651-48df-3577-043aa2e16722@prevas.d...
Note in particular the very last paragraph.
To rephrase and give a concrete example, what I'm worried about is us importing some file system code that (correctly) uses strncpy() to fully initialize some memory buffer, then writes that out to disk - but with our crippled strncpy(), the result is potential garbage on-disk, which both our own read implementation (which is likely also just copied) and the kernel's may subsequently choke on.
Correctness first, please. If there are any performance problems, those should be identified individually and perhaps rewritten to not use strncpy() after verifying that not zeroing the tail is ok for that call site.
Rasmus
On Mon, Aug 07, 2023 at 09:45:37AM +0200, Rasmus Villemoes wrote:
On 14/07/2023 13.24, 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).
So while we're fixing non-standard string function behaviour, can we _please_ finally fix strncpy() so it follows C/POSIX?
https://lore.kernel.org/u-boot/52bc92d4-8651-48df-3577-043aa2e16722@prevas.d...
Note in particular the very last paragraph.
To rephrase and give a concrete example, what I'm worried about is us importing some file system code that (correctly) uses strncpy() to fully initialize some memory buffer, then writes that out to disk - but with our crippled strncpy(), the result is potential garbage on-disk, which both our own read implementation (which is likely also just copied) and the kernel's may subsequently choke on.
Correctness first, please. If there are any performance problems, those should be identified individually and perhaps rewritten to not use strncpy() after verifying that not zeroing the tail is ok for that call site.
Yes, I agree it would be good to make this change.
On Fri, Jul 14, 2023 at 01:24:50PM +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.schiffer@ew.tq-group.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Sean Anderson sean.anderson@seco.com
Applied to u-boot/next, thanks!
participants (5)
-
Matthias Schiffer -
Rasmus Villemoes -
Sean Anderson -
Simon Glass -
Tom Rini