[PATCH 0/5] lib/string_helpers: fixes and test cases for string_unescape()

[PATCH 0/5] lib/string_helpers: fixes and test cases for string_unescape()

[Jonas Rebmann]

This series fixes two bugs in string_unescape() regarding the
destination buffer length. Both fixes are accompanied with kunit tests
which would fail without the fixes.

To make this possible, preparatory patches 1 and 2 improve and clean up
testing helpers and 3 introduces test_unescape_one() which allows for
targeted testing of the string_unescape() function.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
Jonas Rebmann (5):
      lib/tests: string_helpers: check null terminator too
      lib/tests: string_helpers: drop unused parameters
      lib/tests: string_helpers: introduce test_string_unescape_one
      lib/string_helpers: use full destination buffer in string_unescape()
      lib/string_helpers: fix counting of remaining bytes in string_unescape()

 lib/string_helpers.c             |  5 +++--
 lib/tests/string_helpers_kunit.c | 46 +++++++++++++++++++++++++++++-----------
 2 files changed, 37 insertions(+), 14 deletions(-)
---
base-commit: 587858367581b9c55c3690f4e63382ad622719d4
change-id: 20260915-string_unescape-c056aacc90cb

Best regards,
--  
Jonas Rebmann <jre@pengutronix.de>

[PATCH 1/5] lib/tests: string_helpers: check null terminator too

[Jonas Rebmann]

string_unescape() returns the number of character written to dst, not
counting the null terminator which is always written.

Ensure string_unescape has included the null terminator by adding a
separate check.

While at it, improve output for failed tests by showing the memory dump
even if the length differs.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 lib/tests/string_helpers_kunit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/tests/string_helpers_kunit.c b/lib/tests/string_helpers_kunit.c
index 9fbe91079c7e..9bc3acffaf2f 100644
--- a/lib/tests/string_helpers_kunit.c
+++ b/lib/tests/string_helpers_kunit.c
@@ -22,7 +22,7 @@ static void test_string_check_buf(struct kunit *test,
 				  char *out_real, size_t q_real,
 				  char *out_test, size_t q_test)
 {
-	KUNIT_ASSERT_EQ_MSG(test, q_real, q_test, "name:%s", name);
+	KUNIT_EXPECT_EQ_MSG(test, q_real, q_test, "name:%s", name);
 	KUNIT_EXPECT_MEMEQ_MSG(test, out_test, out_real, q_test,
 			       "name:%s", name);
 }
@@ -103,6 +103,7 @@ static void test_string_unescape(struct kunit *test,
 
 	test_string_check_buf(test, name, flags, in, p - 1, out_real, q_real,
 			      out_test, q_test);
+	KUNIT_EXPECT_EQ_MSG(test, out_real[q_real], '\0', "name:%s", name);
 }
 
 struct test_string_1 {

-- 
2.56.0.rc0.108.gf0ef1b96a0

[PATCH 2/5] lib/tests: string_helpers: drop unused parameters

[Jonas Rebmann]

Drop the unused parameters from test_string_check_buf() to improve
readability.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 lib/tests/string_helpers_kunit.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/tests/string_helpers_kunit.c b/lib/tests/string_helpers_kunit.c
index 9bc3acffaf2f..1ed652f762d1 100644
--- a/lib/tests/string_helpers_kunit.c
+++ b/lib/tests/string_helpers_kunit.c
@@ -18,7 +18,6 @@
 
 static void test_string_check_buf(struct kunit *test,
 				  const char *name, unsigned int flags,
-				  char *in, size_t p,
 				  char *out_real, size_t q_real,
 				  char *out_test, size_t q_test)
 {
@@ -101,8 +100,7 @@ static void test_string_unescape(struct kunit *test,
 		q_real = string_unescape(in, out_real, q_real, flags);
 	}
 
-	test_string_check_buf(test, name, flags, in, p - 1, out_real, q_real,
-			      out_test, q_test);
+	test_string_check_buf(test, name, flags, out_real, q_real, out_test, q_test);
 	KUNIT_EXPECT_EQ_MSG(test, out_real[q_real], '\0', "name:%s", name);
 }
 
@@ -457,8 +455,7 @@ static void test_string_escape(struct kunit *test, const char *name,
 
 	q_real = string_escape_mem(in, p, out_real, out_size, flags, esc);
 
-	test_string_check_buf(test, name, flags, in, p, out_real, q_real, out_test,
-			      q_test);
+	test_string_check_buf(test, name, flags, out_real, q_real, out_test, q_test);
 
 	test_string_escape_overflow(test, in, p, flags, esc, q_test, name);
 }

-- 
2.56.0.rc0.108.gf0ef1b96a0

[PATCH 3/5] lib/tests: string_helpers: introduce test_string_unescape_one

[Jonas Rebmann]

The existing test_string_unescape() function follows a complex procedure
where it, given a set of UNESCAPE flags, appends multiple test fragments
and predicts their unescape result for the chosen set of flags. Rename
test_string_unescape() to a more descriptive
test_string_unescape_combined

In preparation to add simple regression tests, introduce
test_string_unescape_one() which asserts on exactly one call to
string_unescape.

Add some tests for corner cases which already pass.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 lib/tests/string_helpers_kunit.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/lib/tests/string_helpers_kunit.c b/lib/tests/string_helpers_kunit.c
index 1ed652f762d1..3c6fa7324965 100644
--- a/lib/tests/string_helpers_kunit.c
+++ b/lib/tests/string_helpers_kunit.c
@@ -55,9 +55,9 @@ static const struct test_string strings[] = {
 	},
 };
 
-static void test_string_unescape(struct kunit *test,
-				 const char *name, unsigned int flags,
-				 bool inplace)
+static void test_string_unescape_combined(struct kunit *test,
+					  const char *name, unsigned int flags,
+					  bool inplace)
 {
 	int q_real = 256;
 	char *in = kunit_kzalloc(test, q_real, GFP_KERNEL);
@@ -596,14 +596,33 @@ static void test_upper_lower(struct kunit *test)
 	}
 }
 
+static void test_string_unescape_one(struct kunit *test,
+				     const char *name, unsigned int flags,
+				     char *src, size_t len,
+				     char *out_test, size_t q_test)
+{
+	char *out_real = kunit_kzalloc(test, len, GFP_KERNEL);
+	int q_real;
+
+	q_real = string_unescape(src, out_real, len, flags);
+	test_string_check_buf(test, name, flags, out_real, q_real, out_test, q_test);
+}
+
 static void test_unescape(struct kunit *test)
 {
 	unsigned int i;
 
 	for (i = 0; i < UNESCAPE_ALL_MASK + 1; i++)
-		test_string_unescape(test, "unescape", i, false);
-	test_string_unescape(test, "unescape inplace",
-			     get_random_u32_below(UNESCAPE_ALL_MASK + 1), true);
+		test_string_unescape_combined(test, "unescape", i, false);
+	test_string_unescape_combined(test, "unescape inplace",
+				      get_random_u32_below(UNESCAPE_ALL_MASK + 1), true);
+
+	test_string_unescape_one(test, "simple case", UNESCAPE_HEX | UNESCAPE_SPECIAL, "ABC", 6, "ABC", 3);
+	test_string_unescape_one(test, "single escape", UNESCAPE_HEX | UNESCAPE_SPECIAL, "A\\x42C", 6, "ABC", 3);
+	test_string_unescape_one(test, "escape before end", UNESCAPE_HEX, "B\\qX", 4, "B\\q", 3);
+	test_string_unescape_one(test, "escape at end", UNESCAPE_HEX, "a\\qX", 3, "a\\", 2);
+	test_string_unescape_one(test, "backslash before escape", UNESCAPE_HEX, "\\\\x41B", 12, "\\\\x41B", 6);
+	test_string_unescape_one(test, "backslash escape", UNESCAPE_HEX | UNESCAPE_SPECIAL, "\\\\x41B", 16, "\\x41B", 5);
 }
 
 static void test_escape(struct kunit *test)

-- 
2.56.0.rc0.108.gf0ef1b96a0

[PATCH 4/5] lib/string_helpers: use full destination buffer in string_unescape()

[Jonas Rebmann]

Although all of the available sequences expand to exactly one byte, the
current implementation decrements the remaining bytes in the destination
buffer twice, effectively shortening it by one byte per each unescaped
character.

The extra decrement is only needed in the one case where a single loop
iteration produces two output bytes: when the sequence turns out not to
be a valid escape sequence, the previously skipped backslash has to be
emitted before the character is copied verbatim.

Add a kunit regression-test that unescapes into a barely long enough 3
buffer.

Fixes: 16c7fa05829e ("lib/string_helpers: introduce generic string_unescape")
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 lib/string_helpers.c             | 2 +-
 lib/tests/string_helpers_kunit.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 98d6ed0eaab7..cb41ef9d8c5b 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -331,7 +331,6 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
 	while (*src && --size) {
 		if (src[0] == '\\' && src[1] != '\0' && size > 1) {
 			src++;
-			size--;
 
 			if (flags & UNESCAPE_SPACE &&
 					unescape_space(&src, &out))
@@ -350,6 +349,7 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
 				continue;
 
 			*out++ = '\\';
+			size--;
 		}
 		*out++ = *src++;
 	}
diff --git a/lib/tests/string_helpers_kunit.c b/lib/tests/string_helpers_kunit.c
index 3c6fa7324965..2e02c680cbb2 100644
--- a/lib/tests/string_helpers_kunit.c
+++ b/lib/tests/string_helpers_kunit.c
@@ -623,6 +623,9 @@ static void test_unescape(struct kunit *test)
 	test_string_unescape_one(test, "escape at end", UNESCAPE_HEX, "a\\qX", 3, "a\\", 2);
 	test_string_unescape_one(test, "backslash before escape", UNESCAPE_HEX, "\\\\x41B", 12, "\\\\x41B", 6);
 	test_string_unescape_one(test, "backslash escape", UNESCAPE_HEX | UNESCAPE_SPECIAL, "\\\\x41B", 16, "\\x41B", 5);
+
+	test_string_unescape_one(test, "short buffer", UNESCAPE_HEX, "\\x41\\x41B", 4, "AAB", 3);
+	test_string_unescape_one(test, "unrecognized escape at end", UNESCAPE_HEX, "B\\qX", 4, "B\\q", 3);
 }
 
 static void test_escape(struct kunit *test)

-- 
2.56.0.rc0.108.gf0ef1b96a0

[PATCH 5/5] lib/string_helpers: fix counting of remaining bytes in string_unescape()

[Jonas Rebmann]

All of the available sequences expand to exactly one byte, the size
check in the loop condition is sufficient for the case of an escaped
character too.

Otherwise, an escape sequence that should be unescaped to the last
character before terminating with null in the destination buffer will be
output as backslash instead of the escaped character.

The only exception is when encountering a backslash that turns out to
not start a valid escape sequence and both the backslash and the
character following are handled in one iteration. Move the check there.

Add a kunit regression-test that unescapes a character to right in front
of the null terminator of the destination buffer.

Fixes: 16c7fa05829e ("lib/string_helpers: introduce generic string_unescape")
Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 lib/string_helpers.c             | 5 +++--
 lib/tests/string_helpers_kunit.c | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index cb41ef9d8c5b..4a621f884bde 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -329,7 +329,7 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
 		size = SIZE_MAX;
 
 	while (*src && --size) {
-		if (src[0] == '\\' && src[1] != '\0' && size > 1) {
+		if (src[0] == '\\' && src[1] != '\0') {
 			src++;
 
 			if (flags & UNESCAPE_SPACE &&
@@ -349,7 +349,8 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags)
 				continue;
 
 			*out++ = '\\';
-			size--;
+			if (!--size)
+				break;
 		}
 		*out++ = *src++;
 	}
diff --git a/lib/tests/string_helpers_kunit.c b/lib/tests/string_helpers_kunit.c
index 2e02c680cbb2..10763a01be83 100644
--- a/lib/tests/string_helpers_kunit.c
+++ b/lib/tests/string_helpers_kunit.c
@@ -626,6 +626,8 @@ static void test_unescape(struct kunit *test)
 
 	test_string_unescape_one(test, "short buffer", UNESCAPE_HEX, "\\x41\\x41B", 4, "AAB", 3);
 	test_string_unescape_one(test, "unrecognized escape at end", UNESCAPE_HEX, "B\\qX", 4, "B\\q", 3);
+
+	test_string_unescape_one(test, "end of buffer", UNESCAPE_HEX, "B\\x41", 3, "BA", 2);
 }
 
 static void test_escape(struct kunit *test)

-- 
2.56.0.rc0.108.gf0ef1b96a0

Re: [PATCH 0/5] lib/string_helpers: fixes and test cases for string_unescape()

[Andrew Morton]

On Wed, 16 Sep 2026 19:38:05 +0200 Jonas Rebmann <jre@pengutronix.de> wrote:

> This series fixes two bugs in string_unescape() regarding the
> destination buffer length. Both fixes are accompanied with kunit tests
> which would fail without the fixes.
> 
> To make this possible, preparatory patches 1 and 2 improve and clean up
> testing helpers and 3 introduces test_unescape_one() which allows for
> targeted testing of the string_unescape() function.

Sashiko complains about uncheched kunit_kzalloc() return val.

	https://sashiko.dev/#/patchset/20260916-string_unescape-v1-0-7f8bd986fa33@pengutronix.de

I wonder whether we really need the usual allocation-failure semantics
in kunit.  Can we just make the kunit memory allocation functions abort
on error?

Re: [PATCH 0/5] lib/string_helpers: fixes and test cases for string_unescape()

[Eric Biggers]

On Wed, Sep 16, 2026 at 02:36:23PM -0700, Andrew Morton wrote:
> On Wed, 16 Sep 2026 19:38:05 +0200 Jonas Rebmann <jre@pengutronix.de> wrote:
> 
> > This series fixes two bugs in string_unescape() regarding the
> > destination buffer length. Both fixes are accompanied with kunit tests
> > which would fail without the fixes.
> > 
> > To make this possible, preparatory patches 1 and 2 improve and clean up
> > testing helpers and 3 introduces test_unescape_one() which allows for
> > targeted testing of the string_unescape() function.
> 
> Sashiko complains about uncheched kunit_kzalloc() return val.
> 
> 	https://sashiko.dev/#/patchset/20260916-string_unescape-v1-0-7f8bd986fa33@pengutronix.de
> 
> I wonder whether we really need the usual allocation-failure semantics
> in kunit.  Can we just make the kunit memory allocation functions abort
> on error?

FWIW, having KUNIT_ASSERT_NOT_NULL built-in to kunit_kmalloc() et al
sounds good to me.  It's not clear to me why it wasn't done that way.
Maybe so that assertion failures show the correct file and line number?
But that can be solved by making them macros.

As-is, every test has to check for NULL, which is unnecessary
boilerplate.  It's also resulted in test-specific wrapper functions that
work around this, like alloc_buf() in lib/crypto/tests/test-utils.h.

If a fallible version is needed in rare cases, it could be underscored:
__kunit_kmalloc().  But I'm not sure any test needs that.

- Eric

Re: [PATCH 0/5] lib/string_helpers: fixes and test cases for string_unescape()

[Andrew Morton]

On Wed, 16 Sep 2026 23:13:36 +0000 Eric Biggers <ebiggers@kernel.org> wrote:

> On Wed, Sep 16, 2026 at 02:36:23PM -0700, Andrew Morton wrote:
> > On Wed, 16 Sep 2026 19:38:05 +0200 Jonas Rebmann <jre@pengutronix.de> wrote:
> > 
> > > This series fixes two bugs in string_unescape() regarding the
> > > destination buffer length. Both fixes are accompanied with kunit tests
> > > which would fail without the fixes.
> > > 
> > > To make this possible, preparatory patches 1 and 2 improve and clean up
> > > testing helpers and 3 introduces test_unescape_one() which allows for
> > > targeted testing of the string_unescape() function.
> > 
> > Sashiko complains about uncheched kunit_kzalloc() return val.
> > 
> > 	https://sashiko.dev/#/patchset/20260916-string_unescape-v1-0-7f8bd986fa33@pengutronix.de
> > 
> > I wonder whether we really need the usual allocation-failure semantics
> > in kunit.  Can we just make the kunit memory allocation functions abort
> > on error?
> 
> FWIW, having KUNIT_ASSERT_NOT_NULL built-in to kunit_kmalloc() et al
> sounds good to me.  It's not clear to me why it wasn't done that way.
> Maybe so that assertion failures show the correct file and line number?
> But that can be solved by making them macros.
> 
> As-is, every test has to check for NULL, which is unnecessary
> boilerplate.  It's also resulted in test-specific wrapper functions that
> work around this, like alloc_buf() in lib/crypto/tests/test-utils.h.
> 
> If a fallible version is needed in rare cases, it could be underscored:
> __kunit_kmalloc().  But I'm not sure any test needs that.

Sure.  

Simply doing kthread_exit() when we're in the middle of something is
rather rude - it'll leak things.  I doubt if anyone cares about that much
if they're engaged in poking around with kunit tests.  otoh, small GFP_KERNEL
allocation failures are close to can't-happen.

One thought is to create a fake struct device in kunit, use
devm_kzalloc everywhere then get kunit_try_catch_throw() to release the
device.  Reuse the devm_ code's tracking and garbage-collecting
infrastructure.  But it just isn't worthwhile.  Make the kunit layer
say "ha ha, you lost some memory" and leave it at that...

Re: [PATCH 1/5] lib/tests: string_helpers: check null terminator too

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 07:38:06PM +0200, Jonas Rebmann wrote:
> string_unescape() returns the number of character written to dst, not
> counting the null terminator which is always written.
> 
> Ensure string_unescape has included the null terminator by adding a
> separate check.

...

> -	KUNIT_ASSERT_EQ_MSG(test, q_real, q_test, "name:%s", name);
> +	KUNIT_EXPECT_EQ_MSG(test, q_real, q_test, "name:%s", name);
>  				  char *out_test, size_t q_test)

The commit message doesn't explain why it's safe to continue the test if the
first assertion fails. Care to elaborate?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] lib/tests: string_helpers: drop unused parameters

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 07:38:07PM +0200, Jonas Rebmann wrote:
> Drop the unused parameters from test_string_check_buf() to improve
> readability.

We need to find a way how to use them in the error messages. This is a
regression in fb57550fcbd8 ("string: Convert helpers selftest to KUnit")
that drops them.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] lib/tests: string_helpers: introduce test_string_unescape_one

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 07:38:08PM +0200, Jonas Rebmann wrote:
> The existing test_string_unescape() function follows a complex procedure
> where it, given a set of UNESCAPE flags, appends multiple test fragments
> and predicts their unescape result for the chosen set of flags. Rename
> test_string_unescape() to a more descriptive
> test_string_unescape_combined
> 
> In preparation to add simple regression tests, introduce
> test_string_unescape_one() which asserts on exactly one call to
> string_unescape.
> 
> Add some tests for corner cases which already pass.

So, this is two-in-one patch change. Needs a split, but I'm not sure why
we even need this. Can't your case be incorporated into the existing
"combined" variant?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] lib/string_helpers: use full destination buffer in string_unescape()

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 07:38:09PM +0200, Jonas Rebmann wrote:
> Although all of the available sequences expand to exactly one byte, the
> current implementation decrements the remaining bytes in the destination
> buffer twice, effectively shortening it by one byte per each unescaped
> character.
> 
> The extra decrement is only needed in the one case where a single loop
> iteration produces two output bytes: when the sequence turns out not to
> be a valid escape sequence, the previously skipped backslash has to be
> emitted before the character is copied verbatim.
> 
> Add a kunit regression-test that unescapes into a barely long enough 3
> buffer.

Can you add a simple example here with the hexdump to show the point?
Yes, reading the code helps, but the above commit message is a bit
unclear (at least to me).

> Fixes: 16c7fa05829e ("lib/string_helpers: introduce generic string_unescape")
> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>

...

> +	test_string_unescape_one(test, "short buffer", UNESCAPE_HEX, "\\x41\\x41B", 4, "AAB", 3);
> +	test_string_unescape_one(test, "unrecognized escape at end", UNESCAPE_HEX, "B\\qX", 4, "B\\q", 3);

Still wondering if we can incorporate this into the "combined" loop?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] lib/string_helpers: fix counting of remaining bytes in string_unescape()

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 07:38:10PM +0200, Jonas Rebmann wrote:
> All of the available sequences expand to exactly one byte, the size
> check in the loop condition is sufficient for the case of an escaped
> character too.
> 
> Otherwise, an escape sequence that should be unescaped to the last
> character before terminating with null in the destination buffer will be
> output as backslash instead of the escaped character.
> 
> The only exception is when encountering a backslash that turns out to
> not start a valid escape sequence and both the backslash and the
> character following are handled in one iteration. Move the check there.
> 
> Add a kunit regression-test that unescapes a character to right in front
> of the null terminator of the destination buffer.

Without examples (and the respective hexdumps) it's hard to get.
I re-read the code and see no issues there. The size > 1 check is for the \\
and \0 at the end of the destination buffer.

So, if the sequence is not recognized it means that it's separate \\ in the
stream and has to be copied as is.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] lib/tests: string_helpers: check null terminator too

[Jonas Rebmann]

Hi,

On 2026-09-17 09:14, Andy Shevchenko wrote:
> On Wed, Sep 16, 2026 at 07:38:06PM +0200, Jonas Rebmann wrote:
>> string_unescape() returns the number of character written to dst, not
>> counting the null terminator which is always written.
>>
>> Ensure string_unescape has included the null terminator by adding a
>> separate check.
> 
> ...
> 
>> -	KUNIT_ASSERT_EQ_MSG(test, q_real, q_test, "name:%s", name);
>> +	KUNIT_EXPECT_EQ_MSG(test, q_real, q_test, "name:%s", name);
>>   				  char *out_test, size_t q_test)
> 
> The commit message doesn't explain why it's safe to continue the test if the
> first assertion fails. Care to elaborate?
> 

It is safe against out of bounds access because both buffers are
allocated by the callers, not by the function under test, and the
comparison is done for the number of bytes expected, not whatever the
function under test indicates was written.

I would add this explanation to the v2 commit message

-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

Re: [PATCH 2/5] lib/tests: string_helpers: drop unused parameters

[Jonas Rebmann]

On 2026-09-17 09:18, Andy Shevchenko wrote:
> On Wed, Sep 16, 2026 at 07:38:07PM +0200, Jonas Rebmann wrote:
>> Drop the unused parameters from test_string_check_buf() to improve
>> readability.
> 
> We need to find a way how to use them in the error messages. This is a
> regression in fb57550fcbd8 ("string: Convert helpers selftest to KUnit")
> that drops them.

In that case I suppose I drop this patch in v2.

-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

Re: [PATCH 3/5] lib/tests: string_helpers: introduce test_string_unescape_one

[Jonas Rebmann]

On 2026-09-17 09:21, Andy Shevchenko wrote:
> On Wed, Sep 16, 2026 at 07:38:08PM +0200, Jonas Rebmann wrote:
>> The existing test_string_unescape() function follows a complex procedure
>> where it, given a set of UNESCAPE flags, appends multiple test fragments
>> and predicts their unescape result for the chosen set of flags. Rename
>> test_string_unescape() to a more descriptive
>> test_string_unescape_combined
>>
>> In preparation to add simple regression tests, introduce
>> test_string_unescape_one() which asserts on exactly one call to
>> string_unescape.
>>
>> Add some tests for corner cases which already pass.
> 
> So, this is two-in-one patch change. Needs a split, but I'm not sure why
> we even need this. Can't your case be incorporated into the existing
> "combined" variant?

They can not be incorporated into the combined variant because that
makes it impossible to control the size of the destination buffer. This
can not be fixed because the idea of the combined test is that all test
strings are combined into one buffer. The bugs fixed in this series only
occur when the destination buffer is limited.

Furthermore, for the regression tests, I want each test tailored for a
specific case and when it fails, to show me which one failed.

I'll update the commit message and split the commit.

-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

Re: [PATCH 4/5] lib/string_helpers: use full destination buffer in string_unescape()

[Jonas Rebmann]

On 2026-09-17 09:45, Andy Shevchenko wrote:
> On Wed, Sep 16, 2026 at 07:38:09PM +0200, Jonas Rebmann wrote:
>> Although all of the available sequences expand to exactly one byte, the
>> current implementation decrements the remaining bytes in the destination
>> buffer twice, effectively shortening it by one byte per each unescaped
>> character.
>>
>> The extra decrement is only needed in the one case where a single loop
>> iteration produces two output bytes: when the sequence turns out not to
>> be a valid escape sequence, the previously skipped backslash has to be
>> emitted before the character is copied verbatim.
>>
>> Add a kunit regression-test that unescapes into a barely long enough 3
>> buffer.
> 
> Can you add a simple example here with the hexdump to show the point?
> Yes, reading the code helps, but the above commit message is a bit
> unclear (at least to me).
> 

My idea was that the regression tests below would serve as examples
here.

Maybe I could mention that "short buffer" without my patch yields "A\"
instead of "AAB"...

I'll add that to the commit messages

And I'll add the kunit outputs to the cover letter:

      # test_unescape: EXPECTATION FAILED at lib/tests/string_helpers_kunit.c:24
      Expected q_real == q_test, but
          q_real == 2 (0x2)
          q_test == 3 (0x3)
  name:short buffer
      # test_unescape: EXPECTATION FAILED at lib/tests/string_helpers_kunit.c:25
      Expected out_test == out_real, but
          out_test ==
           41 <41><42>
          out_real ==
           41 <5c><00>
  name:short buffer
      # test_unescape: EXPECTATION FAILED at lib/tests/string_helpers_kunit.c:25
      Expected out_test == out_real, but
          out_test ==
           42 <41>
          out_real ==
           42 <5c>
  name:end of buffer

>> Fixes: 16c7fa05829e ("lib/string_helpers: introduce generic string_unescape")
>> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
> 
> ...
> 
>> +	test_string_unescape_one(test, "short buffer", UNESCAPE_HEX, "\\x41\\x41B", 4, "AAB", 3);
>> +	test_string_unescape_one(test, "unrecognized escape at end", UNESCAPE_HEX, "B\\qX", 4, "B\\q", 3);
> 
> Still wondering if we can incorporate this into the "combined" loop?

Both these tests really only fail without my fixes because the
destination buffer is short, barely fitting the result.

With a buffer length of 4 it yields "A\", for 5 "AA" and for 6 "AAB",
a bug because "AAB" would fit the 4 byte buffer too.

-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

Re: [PATCH 1/5] lib/tests: string_helpers: check null terminator too

[Andy Shevchenko]

On Thu, Sep 17, 2026 at 10:58:10AM +0200, Jonas Rebmann wrote:
> On 2026-09-17 09:14, Andy Shevchenko wrote:
> > On Wed, Sep 16, 2026 at 07:38:06PM +0200, Jonas Rebmann wrote:
> > > string_unescape() returns the number of character written to dst, not
> > > counting the null terminator which is always written.
> > > 
> > > Ensure string_unescape has included the null terminator by adding a
> > > separate check.

...

> > > -	KUNIT_ASSERT_EQ_MSG(test, q_real, q_test, "name:%s", name);
> > > +	KUNIT_EXPECT_EQ_MSG(test, q_real, q_test, "name:%s", name);
> > >   				  char *out_test, size_t q_test)
> > 
> > The commit message doesn't explain why it's safe to continue the test if the
> > first assertion fails. Care to elaborate?
> 
> It is safe against out of bounds access because both buffers are
> allocated by the callers, not by the function under test, and the
> comparison is done for the number of bytes expected, not whatever the
> function under test indicates was written.

Yes, but assertion aborts the test, while expect just fail the certain
condition and moves on.

> I would add this explanation to the v2 commit message

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] lib/tests: string_helpers: introduce test_string_unescape_one

[Andy Shevchenko]

On Thu, Sep 17, 2026 at 10:58:27AM +0200, Jonas Rebmann wrote:
> On 2026-09-17 09:21, Andy Shevchenko wrote:
> > On Wed, Sep 16, 2026 at 07:38:08PM +0200, Jonas Rebmann wrote:
> > > The existing test_string_unescape() function follows a complex procedure
> > > where it, given a set of UNESCAPE flags, appends multiple test fragments
> > > and predicts their unescape result for the chosen set of flags. Rename
> > > test_string_unescape() to a more descriptive
> > > test_string_unescape_combined
> > > 
> > > In preparation to add simple regression tests, introduce
> > > test_string_unescape_one() which asserts on exactly one call to
> > > string_unescape.
> > > 
> > > Add some tests for corner cases which already pass.
> > 
> > So, this is two-in-one patch change. Needs a split, but I'm not sure why
> > we even need this. Can't your case be incorporated into the existing
> > "combined" variant?
> 
> They can not be incorporated into the combined variant because that
> makes it impossible to control the size of the destination buffer. This
> can not be fixed because the idea of the combined test is that all test
> strings are combined into one buffer. The bugs fixed in this series only
> occur when the destination buffer is limited.

We may modify the loop to provide the length of the destination buffer to be
exactly what we expect, no?

> Furthermore, for the regression tests, I want each test tailored for a
> specific case and when it fails, to show me which one failed.

> I'll update the commit message and split the commit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] lib/tests: string_helpers: introduce test_string_unescape_one

[Jonas Rebmann]

On 2026-09-17 11:08, Andy Shevchenko wrote:
> On Thu, Sep 17, 2026 at 10:58:27AM +0200, Jonas Rebmann wrote:
>> On 2026-09-17 09:21, Andy Shevchenko wrote:
>>> On Wed, Sep 16, 2026 at 07:38:08PM +0200, Jonas Rebmann wrote:
>>>> The existing test_string_unescape() function follows a complex procedure
>>>> where it, given a set of UNESCAPE flags, appends multiple test fragments
>>>> and predicts their unescape result for the chosen set of flags. Rename
>>>> test_string_unescape() to a more descriptive
>>>> test_string_unescape_combined
>>>>
>>>> In preparation to add simple regression tests, introduce
>>>> test_string_unescape_one() which asserts on exactly one call to
>>>> string_unescape.
>>>>
>>>> Add some tests for corner cases which already pass.
>>>
>>> So, this is two-in-one patch change. Needs a split, but I'm not sure why
>>> we even need this. Can't your case be incorporated into the existing
>>> "combined" variant?
>>
>> They can not be incorporated into the combined variant because that
>> makes it impossible to control the size of the destination buffer. This
>> can not be fixed because the idea of the combined test is that all test
>> strings are combined into one buffer. The bugs fixed in this series only
>> occur when the destination buffer is limited.
> 
> We may modify the loop to provide the length of the destination buffer to be
> exactly what we expect, no?

The problem with test_string_unescape_combined is this array crudely
named "strings":

static const struct test_string strings[] = {
	{
		.in = "\\f\\ \\n\\r\\t\\v",
		.out = "\f\\ \n\r\t\v",
		.flags = UNESCAPE_SPACE,
	},

	...

it is not as one may expect a list of the individual tests which
string_unescape is subjected to.

Instead all of the in strings are concatenated and then string_unescape
is called on that for each possible set of flags:

string_unescape(\f\ \n\r\t\v\40\1\387\0064\05\040\8a\110\777\xv\xa\x2c\xD\x6f2\h\\\"\a\e\, out_real, 256, flags)

So there is no way how extending "strings" could test the code paths I
want to test.

-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |