[PATCH v2 0/5] string: strl(cat|cpy)

[PATCH v2 0/5] string: strl(cat|cpy)

[Sean Anderson]

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

[PATCH v2 1/5] lib: string: Fix strlcpy return value

[Sean Anderson]

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

[PATCH v2 1/5] lib: string: Fix strlcpy return value

[Simon Glass]

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

[PATCH v2 1/5] lib: string: Fix strlcpy return value

[Sean Anderson]

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
> 

[PATCH v2 1/5] lib: string: Fix strlcpy return value

[Simon Glass]

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

[PATCH v2 1/5] lib: string: Fix strlcpy return value

[Sean Anderson]

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
>   
> 

[PATCH v2 1/5] lib: string: Fix strlcpy return value

[Tom Rini]

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>

[PATCH v2 2/5] lib: string: Implement strlcat

[Sean Anderson]

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

[PATCH v2 2/5] lib: string: Implement strlcat

[Simon Glass]

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>

[PATCH v2 2/5] lib: string: Implement strlcat

[Tom Rini]

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>

[PATCH v2 3/5] test: Add test for strlcat

[Sean Anderson]

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

[PATCH v2 3/5] test: Add test for strlcat

[Simon Glass]

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

[PATCH v2 3/5] test: Add test for strlcat

[Tom Rini]

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>

[PATCH v2 4/5] fastboot: Fix possible buffer overrun

[Sean Anderson]

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

[PATCH v2 4/5] fastboot: Fix possible buffer overrun

[Tom Rini]

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>

[PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy)

[Sean Anderson]

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

[PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy)

[Simon Glass]

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>

[PATCH v2 5/5] checkpatch: Add warnings for using strn(cat|cpy)

[Tom Rini]

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>