* [PATCH v2 0/5] string: strl(cat|cpy) @ 2021-03-11 5:15 Sean Anderson 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Sean Anderson @ 2021-03-11 5:15 UTC (permalink / raw) To: u-boot This series adds support for strl(cat|cpy), and brings their implementations in-line with what is documented in the Linux man pages. It also fixes some potential (actual) fastboot bugs. Lastly, it adds a patman check to suggest using these functions over strn(cat|cpy). I think these functions provide a much better interface, which removes some footguns from U-Boot. Changes in v2: - Fix strlcpy return value - Add implementation of strlcat - Add test for strlcat - Fix bug in fastboot - Move check to u_boot_line Sean Anderson (5): lib: string: Fix strlcpy return value lib: string: Implement strlcat test: Add test for strlcat fastboot: Fix possible buffer overrun checkpatch: Add warnings for using strn(cat|cpy) drivers/fastboot/fb_mmc.c | 6 +- drivers/usb/dwc3/linux-compat.h | 6 -- include/linux/string.h | 3 + lib/string.c | 31 +++++++- scripts/checkpatch.pl | 6 ++ test/lib/Makefile | 1 + test/lib/strlcat.c | 126 ++++++++++++++++++++++++++++++++ tools/patman/test_checkpatch.py | 14 +++- 8 files changed, 179 insertions(+), 14 deletions(-) create mode 100644 test/lib/strlcat.c -- 2.30.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] lib: string: Fix strlcpy return value 2021-03-11 5:15 [PATCH v2 0/5] string: strl(cat|cpy) Sean Anderson @ 2021-03-11 5:15 ` Sean Anderson 2021-03-25 0:38 ` Simon Glass ` (2 more replies) 2021-03-11 5:15 ` [PATCH v2 2/5] lib: string: Implement strlcat Sean Anderson ` (3 subsequent siblings) 4 siblings, 3 replies; 18+ messages in thread From: Sean Anderson @ 2021-03-11 5:15 UTC (permalink / raw) To: u-boot strlcpy should always return the number of bytes copied. We were accidentally missing the nul-terminator. We also always used to return a non-zero value, even if we did not actually copy anything. Fixes: 23cd138503 ("Integrate USB gadget layer and USB CDC driver layer") Signed-off-by: Sean Anderson <seanga2@gmail.com> --- Changes in v2: - New lib/string.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/string.c b/lib/string.c index 73b984123d..1b867ac09d 100644 --- a/lib/string.c +++ b/lib/string.c @@ -114,17 +114,21 @@ char * strncpy(char * dest,const char *src,size_t count) * NUL-terminated string that fits in the buffer (unless, * of course, the buffer size is zero). It does not pad * out the result like strncpy() does. + * + * Return: the number of bytes copied */ size_t strlcpy(char *dest, const char *src, size_t size) { - size_t ret = strlen(src); - if (size) { - size_t len = (ret >= size) ? size - 1 : ret; + size_t srclen = strlen(src); + size_t len = (srclen >= size) ? size - 1 : srclen; + memcpy(dest, src, len); dest[len] = '\0'; + return len + 1; } - return ret; + + return 0; } #endif -- 2.30.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] lib: string: Fix strlcpy return value 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson @ 2021-03-25 0:38 ` Simon Glass 2021-03-25 0:54 ` Sean Anderson 2021-03-25 0:53 ` Sean Anderson 2021-04-13 14:28 ` Tom Rini 2 siblings, 1 reply; 18+ messages in thread From: Simon Glass @ 2021-03-25 0:38 UTC (permalink / raw) To: u-boot Hi Sean, On Thu, 11 Mar 2021 at 18:15, Sean Anderson <seanga2@gmail.com> wrote: > > strlcpy should always return the number of bytes copied. We were > accidentally missing the nul-terminator. We also always used to return a > non-zero value, even if we did not actually copy anything. > > Fixes: 23cd138503 ("Integrate USB gadget layer and USB CDC driver layer") > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v2: > - New > > lib/string.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) Please can you add a test while you are here? Might be easier on the -next branch. Regards, Simon ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] lib: string: Fix strlcpy return value 2021-03-25 0:38 ` Simon Glass @ 2021-03-25 0:54 ` Sean Anderson 2021-03-25 2:40 ` Simon Glass 0 siblings, 1 reply; 18+ messages in thread From: Sean Anderson @ 2021-03-25 0:54 UTC (permalink / raw) To: u-boot On 3/24/21 8:38 PM, Simon Glass wrote: > Hi Sean, > > On Thu, 11 Mar 2021 at 18:15, Sean Anderson <seanga2@gmail.com> wrote: >> >> strlcpy should always return the number of bytes copied. We were >> accidentally missing the nul-terminator. We also always used to return a >> non-zero value, even if we did not actually copy anything. >> >> Fixes: 23cd138503 ("Integrate USB gadget layer and USB CDC driver layer") >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> Changes in v2: >> - New >> >> lib/string.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) > > Please can you add a test while you are here? Might be easier on the > -next branch. This is tested in patch 3 as strlcat is implemented with strlcpy. Though it looks like I will need to change the implementation... --Sean > > Regards, > Simon > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] lib: string: Fix strlcpy return value 2021-03-25 0:54 ` Sean Anderson @ 2021-03-25 2:40 ` Simon Glass 0 siblings, 0 replies; 18+ messages in thread From: Simon Glass @ 2021-03-25 2:40 UTC (permalink / raw) To: u-boot Hi Sean, On Thu, 25 Mar 2021 at 13:54, Sean Anderson <seanga2@gmail.com> wrote: > > On 3/24/21 8:38 PM, Simon Glass wrote: > > Hi Sean, > > > > On Thu, 11 Mar 2021 at 18:15, Sean Anderson <seanga2@gmail.com> wrote: > >> > >> strlcpy should always return the number of bytes copied. We were > >> accidentally missing the nul-terminator. We also always used to return a > >> non-zero value, even if we did not actually copy anything. > >> > >> Fixes: 23cd138503 ("Integrate USB gadget layer and USB CDC driver layer") > >> > >> Signed-off-by: Sean Anderson <seanga2@gmail.com> > >> --- > >> > >> Changes in v2: > >> - New > >> > >> lib/string.c | 12 ++++++++---- > >> 1 file changed, 8 insertions(+), 4 deletions(-) > > > > Please can you add a test while you are here? Might be easier on the > > -next branch. > > This is tested in patch 3 as strlcat is implemented with strlcpy. Though > it looks like I will need to change the implementation... Yes but I still think it is good to have a few simple test for this function .Transitive tests are quite a bit harder to understand. Perhaps just 3-4 cases? Regards, Simon ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] lib: string: Fix strlcpy return value 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson 2021-03-25 0:38 ` Simon Glass @ 2021-03-25 0:53 ` Sean Anderson 2021-04-13 14:28 ` Tom Rini 2 siblings, 0 replies; 18+ messages in thread From: Sean Anderson @ 2021-03-25 0:53 UTC (permalink / raw) To: u-boot On 3/11/21 12:15 AM, Sean Anderson wrote: > strlcpy should always return the number of bytes copied. We were > accidentally missing the nul-terminator. We also always used to return a It looks like I was a bit bullish in assuming a mistake. After reviewing the man page, it looks like the nul-terminator is intentionally excluded. > The strlcpy() and strlcat() functions return the total length of the > string they tried to create. For strlcpy() that means the length of > src. For strlcat() that means the initial length of dst plus the > length of src. While this may seem somewhat confusing, it was done to > make truncation detection simple. In particular, we should return the length of the string, which may be different from the amount of bytes copied. However, the non-zero return value should be fixed. > non-zero value, even if we did not actually copy anything. > > Fixes: 23cd138503 ("Integrate USB gadget layer and USB CDC driver layer") > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v2: > - New > > lib/string.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/lib/string.c b/lib/string.c > index 73b984123d..1b867ac09d 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -114,17 +114,21 @@ char * strncpy(char * dest,const char *src,size_t count) > * NUL-terminated string that fits in the buffer (unless, > * of course, the buffer size is zero). It does not pad > * out the result like strncpy() does. > + * > + * Return: the number of bytes copied > */ > size_t strlcpy(char *dest, const char *src, size_t size) > { > - size_t ret = strlen(src); > - > if (size) { > - size_t len = (ret >= size) ? size - 1 : ret; > + size_t srclen = strlen(src); > + size_t len = (srclen >= size) ? size - 1 : srclen; > + > memcpy(dest, src, len); > dest[len] = '\0'; > + return len + 1; > } > - return ret; > + > + return 0; > } > #endif > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/5] lib: string: Fix strlcpy return value 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-03-25 0:53 ` Sean Anderson @ 2021-04-13 14:28 ` Tom Rini 2 siblings, 0 replies; 18+ messages in thread From: Tom Rini @ 2021-04-13 14:28 UTC (permalink / raw) To: u-boot On Thu, Mar 11, 2021 at 12:15:41AM -0500, Sean Anderson wrote: > strlcpy should always return the number of bytes copied. We were > accidentally missing the nul-terminator. We also always used to return a > non-zero value, even if we did not actually copy anything. > > Fixes: 23cd138503 ("Integrate USB gadget layer and USB CDC driver layer") > > Signed-off-by: Sean Anderson <seanga2@gmail.com> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/5b008d73/attachment.sig> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/5] lib: string: Implement strlcat 2021-03-11 5:15 [PATCH v2 0/5] string: strl(cat|cpy) Sean Anderson 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson @ 2021-03-11 5:15 ` Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:28 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 3/5] test: Add test for strlcat Sean Anderson ` (2 subsequent siblings) 4 siblings, 2 replies; 18+ messages in thread From: Sean Anderson @ 2021-03-11 5:15 UTC (permalink / raw) To: u-boot This introduces strlcat, which provides a safer interface than strncat. It never copies more than its size bytes, including the terminating nul. In addition, it never reads past dest[size - 1], even if dest is not nul-terminated. This also removes the stub for dwc3 now that we have a proper implementation. Signed-off-by: Sean Anderson <seanga2@gmail.com> --- Changes in v2: - New drivers/usb/dwc3/linux-compat.h | 6 ------ include/linux/string.h | 3 +++ lib/string.c | 19 +++++++++++++++++++ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/usb/dwc3/linux-compat.h b/drivers/usb/dwc3/linux-compat.h index 82793765be..3bb0bda5a6 100644 --- a/drivers/usb/dwc3/linux-compat.h +++ b/drivers/usb/dwc3/linux-compat.h @@ -13,10 +13,4 @@ #define dev_WARN(dev, format, arg...) debug(format, ##arg) -static inline size_t strlcat(char *dest, const char *src, size_t n) -{ - strcat(dest, src); - return strlen(dest) + strlen(src); -} - #endif diff --git a/include/linux/string.h b/include/linux/string.h index d67998e5c4..dd255f2163 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -35,6 +35,9 @@ extern char * strcat(char *, const char *); #ifndef __HAVE_ARCH_STRNCAT extern char * strncat(char *, const char *, __kernel_size_t); #endif +#ifndef __HAVE_ARCH_STRLCAT +size_t strlcat(char *, const char *, size_t); +#endif #ifndef __HAVE_ARCH_STRCMP extern int strcmp(const char *,const char *); #endif diff --git a/lib/string.c b/lib/string.c index 1b867ac09d..a0cff8fe88 100644 --- a/lib/string.c +++ b/lib/string.c @@ -180,6 +180,25 @@ char * strncat(char *dest, const char *src, size_t count) } #endif +#ifndef __HAVE_ARCH_STRLCAT +/** + * strlcat - Append a length-limited, %NUL-terminated string to another + * @dest: The string to be appended to + * @src: The string to append to it + * @size: The size of @dest + * + * 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. + */ +size_t strlcat(char *dest, const char *src, size_t size) +{ + size_t len = strnlen(dest, size); + + return len + strlcpy(dest + len, src, size - len); +} +#endif + #ifndef __HAVE_ARCH_STRCMP /** * strcmp - Compare two strings -- 2.30.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/5] lib: string: Implement strlcat 2021-03-11 5:15 ` [PATCH v2 2/5] lib: string: Implement strlcat Sean Anderson @ 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:28 ` Tom Rini 1 sibling, 0 replies; 18+ messages in thread From: Simon Glass @ 2021-03-25 0:38 UTC (permalink / raw) To: u-boot On Thu, 11 Mar 2021 at 18:15, Sean Anderson <seanga2@gmail.com> wrote: > > This introduces strlcat, which provides a safer interface than strncat. It > never copies more than its size bytes, including the terminating nul. In > addition, it never reads past dest[size - 1], even if dest is not > nul-terminated. > > This also removes the stub for dwc3 now that we have a proper > implementation. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v2: > - New > > drivers/usb/dwc3/linux-compat.h | 6 ------ > include/linux/string.h | 3 +++ > lib/string.c | 19 +++++++++++++++++++ > 3 files changed, 22 insertions(+), 6 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/5] lib: string: Implement strlcat 2021-03-11 5:15 ` [PATCH v2 2/5] lib: string: Implement strlcat Sean Anderson 2021-03-25 0:38 ` Simon Glass @ 2021-04-13 14:28 ` Tom Rini 1 sibling, 0 replies; 18+ messages in thread From: Tom Rini @ 2021-04-13 14:28 UTC (permalink / raw) To: u-boot On Thu, Mar 11, 2021 at 12:15:42AM -0500, Sean Anderson wrote: > This introduces strlcat, which provides a safer interface than strncat. It > never copies more than its size bytes, including the terminating nul. In > addition, it never reads past dest[size - 1], even if dest is not > nul-terminated. > > This also removes the stub for dwc3 now that we have a proper > implementation. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/99e7e408/attachment.sig> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/5] test: Add test for strlcat 2021-03-11 5:15 [PATCH v2 0/5] string: strl(cat|cpy) Sean Anderson 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson 2021-03-11 5:15 ` [PATCH v2 2/5] lib: string: Implement strlcat Sean Anderson @ 2021-03-11 5:15 ` Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:29 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 4/5] fastboot: Fix possible buffer overrun Sean Anderson 2021-03-11 5:15 ` [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) Sean Anderson 4 siblings, 2 replies; 18+ messages in thread From: Sean Anderson @ 2021-03-11 5:15 UTC (permalink / raw) To: u-boot This test is adapted from glibc, which is very concerned about alignment. It also tests strlcpy by dependency. Signed-off-by: Sean Anderson <seanga2@gmail.com> --- Changes in v2: - New test/lib/Makefile | 1 + test/lib/strlcat.c | 126 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 test/lib/strlcat.c diff --git a/test/lib/Makefile b/test/lib/Makefile index 97c11e35a8..685d1c95d8 100644 --- a/test/lib/Makefile +++ b/test/lib/Makefile @@ -10,6 +10,7 @@ obj-y += lmb.o obj-$(CONFIG_CONSOLE_RECORD) += test_print.o obj-$(CONFIG_SSCANF) += sscanf.o obj-y += string.o +obj-y += strlcat.o obj-$(CONFIG_ERRNO_STR) += test_errno_str.o obj-$(CONFIG_UT_LIB_ASN1) += asn1.o obj-$(CONFIG_UT_LIB_RSA) += rsa.o diff --git a/test/lib/strlcat.c b/test/lib/strlcat.c new file mode 100644 index 0000000000..ee61684c40 --- /dev/null +++ b/test/lib/strlcat.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: GPL-2.1+ +/* + * Copyright (C) 2021 Sean Anderson <seanga2@gmail.com> + * Copyright (C) 2011-2021 Free Software Foundation, Inc. + * + * These tests adapted from glibc's string/test-strncat.c + */ + +#include <common.h> +#include <test/lib.h> +#include <test/test.h> +#include <test/ut.h> + +#define BUF_SIZE 4096 +char buf1[BUF_SIZE], buf2[BUF_SIZE]; + +static int do_test_strlcat(struct unit_test_state *uts, int line, size_t align1, + size_t align2, size_t len1, size_t len2, size_t n) +{ + char *s1, *s2; + size_t i, len, expected, actual; + + align1 &= 7; + if (align1 + len1 >= BUF_SIZE) + return 0; + if (align1 + n > BUF_SIZE) + return 0; + + align2 &= 7; + if (align2 + len1 + len2 >= BUF_SIZE) + return 0; + if (align2 + len1 + n > BUF_SIZE) + return 0; + + s1 = buf1 + align1; + s2 = buf2 + align2; + + for (i = 0; i < len1 - 1; i++) + s1[i] = 32 + 23 * i % (127 - 32); + s1[len1 - 1] = '\0'; + + for (i = 0; i < len2 - 1; i++) + s2[i] = 32 + 23 * i % (127 - 32); + s2[len2 - 1] = '\0'; + + expected = len2 < n ? min(len1 + len2 - 1, n) : n; + 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", + "Expected %#lx (%ld), got %#lx (%ld)", + expected, expected, actual, actual); + return CMD_RET_FAILURE; + } + + len = min3(len1, n - len2, (size_t)0); + if (memcmp(s2 + len2, s1, len)) { + ut_failf(uts, __FILE__, line, __func__, + "s2 + len1 == s1", + "Expected \"%.*s\", got \"%.*s\"", + (int)len, s1, (int)len, s2 + len2); + return CMD_RET_FAILURE; + } + + i = min(n, len1 + len2 - 1) - 1; + if (len2 < n && s2[i] != '\0') { + ut_failf(uts, __FILE__, line, __func__, + "n < len1 && s2[len2 + n] == '\\0'", + "Expected s2[%ld] = '\\0', got %d ('%c')", + i, s2[i], s2[i]); + return CMD_RET_FAILURE; + } + + return 0; +} + +#define test_strlcat(align1, align2, len1, len2, n) do { \ + int ret = do_test_strlcat(uts, __LINE__, align1, align2, len1, len2, \ + n); \ + if (ret) \ + return ret; \ +} while (0) + +static int lib_test_strlcat(struct unit_test_state *uts) +{ + size_t i, n; + + test_strlcat(0, 2, 2, 2, SIZE_MAX); + test_strlcat(0, 0, 4, 4, SIZE_MAX); + test_strlcat(4, 0, 4, 4, SIZE_MAX); + test_strlcat(0, 0, 8, 8, SIZE_MAX); + test_strlcat(0, 8, 8, 8, SIZE_MAX); + + for (i = 1; i < 8; i++) { + test_strlcat(0, 0, 8 << i, 8 << i, SIZE_MAX); + test_strlcat(8 - i, 2 * i, 8 << i, 8 << i, SIZE_MAX); + test_strlcat(0, 0, 8 << i, 2 << i, SIZE_MAX); + test_strlcat(8 - i, 2 * i, 8 << i, 2 << i, SIZE_MAX); + + test_strlcat(i, 2 * i, 8 << i, 1, SIZE_MAX); + test_strlcat(2 * i, i, 8 << i, 1, SIZE_MAX); + test_strlcat(i, i, 8 << i, 10, SIZE_MAX); + } + + for (n = 2; n <= 2048; n *= 4) { + test_strlcat(0, 2, 2, 2, n); + test_strlcat(0, 0, 4, 4, n); + test_strlcat(4, 0, 4, 4, n); + test_strlcat(0, 0, 8, 8, n); + test_strlcat(0, 8, 8, 8, n); + + for (i = 1; i < 8; i++) { + test_strlcat(0, 0, 8 << i, 8 << i, n); + test_strlcat(8 - i, 2 * i, 8 << i, 8 << i, n); + test_strlcat(0, 0, 8 << i, 2 << i, n); + test_strlcat(8 - i, 2 * i, 8 << i, 2 << i, n); + + test_strlcat(i, 2 * i, 8 << i, 1, n); + test_strlcat(2 * i, i, 8 << i, 1, n); + test_strlcat(i, i, 8 << i, 10, n); + } + } + + return 0; +} +LIB_TEST(lib_test_strlcat, 0); -- 2.30.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/5] test: Add test for strlcat 2021-03-11 5:15 ` [PATCH v2 3/5] test: Add test for strlcat Sean Anderson @ 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:29 ` Tom Rini 1 sibling, 0 replies; 18+ messages in thread From: Simon Glass @ 2021-03-25 0:38 UTC (permalink / raw) To: u-boot Hi Sean, On Thu, 11 Mar 2021 at 18:15, Sean Anderson <seanga2@gmail.com> wrote: > > This test is adapted from glibc, which is very concerned about alignment. > It also tests strlcpy by dependency. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v2: > - New > > test/lib/Makefile | 1 + > test/lib/strlcat.c | 126 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 127 insertions(+) > create mode 100644 test/lib/strlcat.c Reviewed-by: Simon Glass <sjg@chromium.org> Gosh, it certainly is. I do wonder whether you are better just using ut_asserteq_str() since people get the line number and that is normally enough to repeat and find the failure. - Simon ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/5] test: Add test for strlcat 2021-03-11 5:15 ` [PATCH v2 3/5] test: Add test for strlcat Sean Anderson 2021-03-25 0:38 ` Simon Glass @ 2021-04-13 14:29 ` Tom Rini 1 sibling, 0 replies; 18+ messages in thread From: Tom Rini @ 2021-04-13 14:29 UTC (permalink / raw) To: u-boot On Thu, Mar 11, 2021 at 12:15:43AM -0500, Sean Anderson wrote: > This test is adapted from glibc, which is very concerned about alignment. > It also tests strlcpy by dependency. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/82430c3b/attachment.sig> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/5] fastboot: Fix possible buffer overrun 2021-03-11 5:15 [PATCH v2 0/5] string: strl(cat|cpy) Sean Anderson ` (2 preceding siblings ...) 2021-03-11 5:15 ` [PATCH v2 3/5] test: Add test for strlcat Sean Anderson @ 2021-03-11 5:15 ` Sean Anderson 2021-04-13 14:29 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) Sean Anderson 4 siblings, 1 reply; 18+ messages in thread From: Sean Anderson @ 2021-03-11 5:15 UTC (permalink / raw) To: u-boot This fixes several uses of strn(cpy|cat) which did not terminate their destinations properly. Fixes de1728ce4c ("fastboot: Allow u-boot-style partitions") Reported-by: Coverity Scan Signed-off-by: Sean Anderson <seanga2@gmail.com> --- Changes in v2: - New drivers/fastboot/fb_mmc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c index 8e74e50e91..2f3837e559 100644 --- a/drivers/fastboot/fb_mmc.c +++ b/drivers/fastboot/fb_mmc.c @@ -40,7 +40,7 @@ static int raw_part_get_info_by_name(struct blk_desc *dev_desc, /* check for raw partition descriptor */ strcpy(env_desc_name, "fastboot_raw_partition_"); - strncat(env_desc_name, name, PART_NAME_LEN); + strlcat(env_desc_name, name, PART_NAME_LEN); raw_part_desc = strdup(env_get(env_desc_name)); if (raw_part_desc == NULL) return -ENODEV; @@ -61,7 +61,7 @@ static int raw_part_get_info_by_name(struct blk_desc *dev_desc, info->start = simple_strtoul(argv[0], NULL, 0); info->size = simple_strtoul(argv[1], NULL, 0); info->blksz = dev_desc->blksz; - strncpy((char *)info->name, name, PART_NAME_LEN); + strlcpy((char *)info->name, name, PART_NAME_LEN); if (raw_part_desc) { if (strcmp(strsep(&raw_part_desc, " "), "mmcpart") == 0) { @@ -114,7 +114,7 @@ static int part_get_info_by_name_or_alias(struct blk_desc **dev_desc, /* check for alias */ strcpy(env_alias_name, "fastboot_partition_alias_"); - strncat(env_alias_name, name, PART_NAME_LEN); + strlcat(env_alias_name, name, PART_NAME_LEN); aliased_part_name = env_get(env_alias_name); if (aliased_part_name != NULL) ret = do_get_part_info(dev_desc, aliased_part_name, -- 2.30.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 4/5] fastboot: Fix possible buffer overrun 2021-03-11 5:15 ` [PATCH v2 4/5] fastboot: Fix possible buffer overrun Sean Anderson @ 2021-04-13 14:29 ` Tom Rini 0 siblings, 0 replies; 18+ messages in thread From: Tom Rini @ 2021-04-13 14:29 UTC (permalink / raw) To: u-boot On Thu, Mar 11, 2021 at 12:15:44AM -0500, Sean Anderson wrote: > This fixes several uses of strn(cpy|cat) which did not terminate their > destinations properly. > > Fixes de1728ce4c ("fastboot: Allow u-boot-style partitions") > > Reported-by: Coverity Scan > Signed-off-by: Sean Anderson <seanga2@gmail.com> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/5cb22c8a/attachment.sig> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) 2021-03-11 5:15 [PATCH v2 0/5] string: strl(cat|cpy) Sean Anderson ` (3 preceding siblings ...) 2021-03-11 5:15 ` [PATCH v2 4/5] fastboot: Fix possible buffer overrun Sean Anderson @ 2021-03-11 5:15 ` Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:29 ` Tom Rini 4 siblings, 2 replies; 18+ messages in thread From: Sean Anderson @ 2021-03-11 5:15 UTC (permalink / raw) To: u-boot strn(cat|cpy) has a bad habit of not nul-terminating the destination, resulting in constructions like strncpy(foo, bar, sizeof(foo) - 1); foo[sizeof(foo) - 1] = '\0'; However, it is very easy to forget about this behavior and accidentally leave a string unterminated. This has shown up in some recent coverity scans [1, 2] (including code recently touched by yours truly). Fortunately, the guys at OpenBSD came up with strl(cat|cpy), which always nul-terminate strings. These functions are already in U-Boot, so we should encourage new code to use them instead of strn(cat|cpy). [1] https://lists.denx.de/pipermail/u-boot/2021-March/442888.html [2] https://lists.denx.de/pipermail/u-boot/2021-January/438073.html Signed-off-by: Sean Anderson <seanga2@gmail.com> --- Changes in v2: - Move check to u_boot_line scripts/checkpatch.pl | 6 ++++++ tools/patman/test_checkpatch.py | 14 +++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 755f4802a4..4e047586a6 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2365,6 +2365,12 @@ sub u_boot_line { "Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible\n" . $herecurr); } + # prefer strl(cpy|cat) over strn(cpy|cat) + if ($line =~ /\bstrn(cpy|cat)\s*\(/) { + WARN("STRL", + "strl$1 is preferred over strn$1 because it always produces a nul-terminated string\n" . $herecurr); + } + # use defconfig to manage CONFIG_CMD options if ($line =~ /\+\s*#\s*(define|undef)\s+(CONFIG_CMD\w*)\b/) { ERROR("DEFINE_CONFIG_CMD", diff --git a/tools/patman/test_checkpatch.py b/tools/patman/test_checkpatch.py index a4fec1d4c1..56af5265cc 100644 --- a/tools/patman/test_checkpatch.py +++ b/tools/patman/test_checkpatch.py @@ -353,7 +353,7 @@ index 0000000..2234c87 Args: pm: PatchMaker object to use - msg" Expected message (e.g. 'LIVETREE') + msg: Expected message (e.g. 'LIVETREE') pmtype: Type of problem ('error', 'warning') """ result = pm.run_checkpatch() @@ -439,6 +439,18 @@ index 0000000..2234c87 self.check_struct('per_device_auto', '_priv', 'DEVICE_PRIV_AUTO') self.check_struct('per_device_plat_auto', '_plat', 'DEVICE_PLAT_AUTO') + def check_strl(self, func): + """Check one of the checks for strn(cpy|cat)""" + pm = PatchMaker() + pm.add_line('common/main.c', "strn%s(foo, bar, sizeof(foo));" % func) + self.checkSingleMessage(pm, "STRL", + "strl%s is preferred over strn%s because it always produces a nul-terminated string\n" + % (func, func)) + + def testStrl(self): + """Check for uses of strn(cat|cpy)""" + self.check_strl("cat"); + self.check_strl("cpy"); if __name__ == "__main__": unittest.main() -- 2.30.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) 2021-03-11 5:15 ` [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) Sean Anderson @ 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:29 ` Tom Rini 1 sibling, 0 replies; 18+ messages in thread From: Simon Glass @ 2021-03-25 0:38 UTC (permalink / raw) To: u-boot On Thu, 11 Mar 2021 at 18:16, Sean Anderson <seanga2@gmail.com> wrote: > > strn(cat|cpy) has a bad habit of not nul-terminating the destination, > resulting in constructions like > > strncpy(foo, bar, sizeof(foo) - 1); > foo[sizeof(foo) - 1] = '\0'; > > However, it is very easy to forget about this behavior and accidentally > leave a string unterminated. This has shown up in some recent coverity > scans [1, 2] (including code recently touched by yours truly). > > Fortunately, the guys at OpenBSD came up with strl(cat|cpy), which always > nul-terminate strings. These functions are already in U-Boot, so we should > encourage new code to use them instead of strn(cat|cpy). > > [1] https://lists.denx.de/pipermail/u-boot/2021-March/442888.html > [2] https://lists.denx.de/pipermail/u-boot/2021-January/438073.html > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v2: > - Move check to u_boot_line > > scripts/checkpatch.pl | 6 ++++++ > tools/patman/test_checkpatch.py | 14 +++++++++++++- > 2 files changed, 19 insertions(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) 2021-03-11 5:15 ` [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) Sean Anderson 2021-03-25 0:38 ` Simon Glass @ 2021-04-13 14:29 ` Tom Rini 1 sibling, 0 replies; 18+ messages in thread From: Tom Rini @ 2021-04-13 14:29 UTC (permalink / raw) To: u-boot On Thu, Mar 11, 2021 at 12:15:45AM -0500, Sean Anderson wrote: > strn(cat|cpy) has a bad habit of not nul-terminating the destination, > resulting in constructions like > > strncpy(foo, bar, sizeof(foo) - 1); > foo[sizeof(foo) - 1] = '\0'; > > However, it is very easy to forget about this behavior and accidentally > leave a string unterminated. This has shown up in some recent coverity > scans [1, 2] (including code recently touched by yours truly). > > Fortunately, the guys at OpenBSD came up with strl(cat|cpy), which always > nul-terminate strings. These functions are already in U-Boot, so we should > encourage new code to use them instead of strn(cat|cpy). > > [1] https://lists.denx.de/pipermail/u-boot/2021-March/442888.html > [2] https://lists.denx.de/pipermail/u-boot/2021-January/438073.html > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210413/13bfa119/attachment.sig> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-04-13 14:29 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-11 5:15 [PATCH v2 0/5] string: strl(cat|cpy) Sean Anderson 2021-03-11 5:15 ` [PATCH v2 1/5] lib: string: Fix strlcpy return value Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-03-25 0:54 ` Sean Anderson 2021-03-25 2:40 ` Simon Glass 2021-03-25 0:53 ` Sean Anderson 2021-04-13 14:28 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 2/5] lib: string: Implement strlcat Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:28 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 3/5] test: Add test for strlcat Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:29 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 4/5] fastboot: Fix possible buffer overrun Sean Anderson 2021-04-13 14:29 ` Tom Rini 2021-03-11 5:15 ` [PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy) Sean Anderson 2021-03-25 0:38 ` Simon Glass 2021-04-13 14:29 ` Tom Rini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.