[PATCH v2 0/5] string: strl(cat|cpy)
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
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
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
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
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
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.
*/ size_t strlcpy(char *dest, const char *src, size_t size) {
- Return: the number of bytes copied
- 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
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!
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
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
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!
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);
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
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!
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,
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!
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()
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
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!
participants (3)
-
Sean Anderson -
Simon Glass -
Tom Rini