[PATCH next 0/8] test_hexdump: Improve test coverage

[PATCH next 0/8] test_hexdump: Improve test coverage

[David Laight]

It was suggested that I add some addition tests for hex_dump_to_buffer().
This required reworking the test code so that could verify arbitrary data.

16 of the tests (those with zero len and bufsize) fail on the current
kernel (the return value should be zero but is ascii ? 3 * rowsize : -1)

There are now 2000 tests that include more buffer overflow conditions and
all 256 bytes values.

David Laight (8):
  Change rowsize and groupsize to size_t
  Create test output data from the binary input data buffer
  Use longer variable names
  Check for buffer overrun of sample output buffer
  Fix sample output if zero bytes requested
  If a test fails print all the parameters
  Run fixed not random tests
  Test all 256 byte values

 lib/test_hexdump.c | 256 ++++++++++++++++++++-------------------------
 1 file changed, 113 insertions(+), 143 deletions(-)

-- 
2.39.5

[PATCH next 1/8] test_hexdump: Change rowsize and groupsize to size_t

[David Laight]

Both refer to positive values, size_t matches the other parameters.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 751645645988..502768e56e4e 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -64,14 +64,14 @@ static const char * const test_data_8_be[] __initconst = {
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 
-static void __init test_hexdump_prepare_test(size_t len, int rowsize,
-					     int groupsize, char *test,
+static void __init test_hexdump_prepare_test(size_t len, size_t rowsize,
+					     size_t groupsize, char *test,
 					     size_t testlen, bool ascii)
 {
 	char *p;
 	const char * const *result;
 	size_t l = len;
-	int gs = groupsize, rs = rowsize;
+	size_t gs = groupsize, rs = rowsize;
 	unsigned int i;
 	const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
 
@@ -122,7 +122,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
 
 #define TEST_HEXDUMP_BUF_SIZE		(32 * 3 + 2 + 32 + 1)
 
-static void __init test_hexdump(size_t len, int rowsize, int groupsize,
+static void __init test_hexdump(size_t len, size_t rowsize, size_t groupsize,
 				bool ascii)
 {
 	char test[TEST_HEXDUMP_BUF_SIZE];
@@ -139,16 +139,16 @@ static void __init test_hexdump(size_t len, int rowsize, int groupsize,
 				  ascii);
 
 	if (memcmp(test, real, TEST_HEXDUMP_BUF_SIZE)) {
-		pr_err("Len: %zu row: %d group: %d\n", len, rowsize, groupsize);
+		pr_err("Len: %zu row: %zu group: %zu\n", len, rowsize, groupsize);
 		pr_err("Result: '%s'\n", real);
 		pr_err("Expect: '%s'\n", test);
 		failed_tests++;
 	}
 }
 
-static void __init test_hexdump_set(int rowsize, bool ascii)
+static void __init test_hexdump_set(size_t rowsize, bool ascii)
 {
-	size_t d = min_t(size_t, sizeof(data_b), rowsize);
+	size_t d = min(sizeof(data_b), rowsize);
 	size_t len = get_random_u32_inclusive(1, d);
 
 	test_hexdump(len, rowsize, 4, ascii);
@@ -158,13 +158,13 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
 }
 
 static void __init test_hexdump_overflow(size_t buflen, size_t len,
-					 int rowsize, int groupsize,
+					 size_t rowsize, size_t groupsize,
 					 bool ascii)
 {
 	char test[TEST_HEXDUMP_BUF_SIZE];
 	char buf[TEST_HEXDUMP_BUF_SIZE];
-	int rs = rowsize, gs = groupsize;
-	int ae, he, e, f, r;
+	size_t rs = rowsize, gs = groupsize;
+	size_t ae, he, e, f, r;
 	bool a;
 
 	total_tests++;
@@ -185,7 +185,7 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 	else
 		e = he;
 
-	f = min_t(int, e + 1, buflen);
+	f = min(e + 1, buflen);
 	if (buflen) {
 		test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
 		test[f - 1] = '\0';
@@ -199,8 +199,8 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 	if (!a) {
 		pr_err("Len: %zu buflen: %zu strlen: %zu\n",
 			len, buflen, strnlen(buf, sizeof(buf)));
-		pr_err("Result: %d '%s'\n", r, buf);
-		pr_err("Expect: %d '%s'\n", e, test);
+		pr_err("Result: %zu '%s'\n", r, buf);
+		pr_err("Expect: %zu '%s'\n", e, test);
 		failed_tests++;
 	}
 }
@@ -208,10 +208,10 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
 {
 	unsigned int i = 0;
-	int rs = get_random_u32_inclusive(1, 2) * 16;
+	size_t rs = get_random_u32_inclusive(1, 2) * 16;
 
 	do {
-		int gs = 1 << i;
+		size_t gs = 1 << i;
 		size_t len = get_random_u32_below(rs) + gs;
 
 		test_hexdump_overflow(buflen, rounddown(len, gs), rs, gs, ascii);
@@ -221,7 +221,7 @@ static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
 static int __init test_hexdump_init(void)
 {
 	unsigned int i;
-	int rowsize;
+	size_t rowsize;
 
 	rowsize = get_random_u32_inclusive(1, 2) * 16;
 	for (i = 0; i < 16; i++)
-- 
2.39.5

Re: [PATCH next 1/8] test_hexdump: Change rowsize and groupsize to size_t

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 09:34:45AM +0000, David Laight wrote:
> Both refer to positive values, size_t matches the other parameters.

But why? The original code is written to mimic the parameters of
the hexdump_to_buffer().

You probably want to say that this matches it after your other patch being
applied (which is not yet the case).

-- 
With Best Regards,
Andy Shevchenko

[PATCH next 2/8] test_hexdump: Create test output data from the binary input data buffer

[David Laight]

Using the same data that is passed to hex_dump_to_buffer() lets
the tests be expanded to test different input data bytes.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 80 +++++++++-------------------------------------
 1 file changed, 15 insertions(+), 65 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 502768e56e4e..e142b11c36c6 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -3,6 +3,7 @@
  */
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/ctype.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -16,49 +17,6 @@ static const unsigned char data_b[] = {
 	'\x4c', '\xd1', '\x19', '\x99', '\x43', '\xb1', '\xaf', '\x0c',	/* 18 - 1f */
 };
 
-static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
-
-static const char * const test_data_1[] __initconst = {
-	"be", "32", "db", "7b", "0a", "18", "93", "b2",
-	"70", "ba", "c4", "24", "7d", "83", "34", "9b",
-	"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
-	"4c", "d1", "19", "99", "43", "b1", "af", "0c",
-};
-
-static const char * const test_data_2_le[] __initconst = {
-	"32be", "7bdb", "180a", "b293",
-	"ba70", "24c4", "837d", "9b34",
-	"9ca6", "ad31", "0f9c", "e9ac",
-	"d14c", "9919", "b143", "0caf",
-};
-
-static const char * const test_data_2_be[] __initconst = {
-	"be32", "db7b", "0a18", "93b2",
-	"70ba", "c424", "7d83", "349b",
-	"a69c", "31ad", "9c0f", "ace9",
-	"4cd1", "1999", "43b1", "af0c",
-};
-
-static const char * const test_data_4_le[] __initconst = {
-	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
-	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
-};
-
-static const char * const test_data_4_be[] __initconst = {
-	"be32db7b", "0a1893b2", "70bac424", "7d83349b",
-	"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
-};
-
-static const char * const test_data_8_le[] __initconst = {
-	"b293180a7bdb32be", "9b34837d24c4ba70",
-	"e9ac0f9cad319ca6", "0cafb1439919d14c",
-};
-
-static const char * const test_data_8_be[] __initconst = {
-	"be32db7b0a1893b2", "70bac4247d83349b",
-	"a69c31ad9c0face9", "4cd1199943b1af0c",
-};
-
 #define FILL_CHAR	'#'
 
 static unsigned total_tests __initdata;
@@ -69,11 +27,9 @@ static void __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 					     size_t testlen, bool ascii)
 {
 	char *p;
-	const char * const *result;
 	size_t l = len;
 	size_t gs = groupsize, rs = rowsize;
-	unsigned int i;
-	const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
+	size_t bs, i, j;
 
 	if (rs != 16 && rs != 32)
 		rs = 16;
@@ -83,26 +39,18 @@ static void __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 
 	if (!is_power_of_2(gs) || gs > 8 || (len % gs != 0))
 		gs = 1;
-
-	if (gs == 8)
-		result = is_be ? test_data_8_be : test_data_8_le;
-	else if (gs == 4)
-		result = is_be ? test_data_4_be : test_data_4_le;
-	else if (gs == 2)
-		result = is_be ? test_data_2_be : test_data_2_le;
-	else
-		result = test_data_1;
+	bs = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ? 0 : gs - 1;
 
 	/* hex dump */
 	p = test;
-	for (i = 0; i < l / gs; i++) {
-		const char *q = *result++;
-		size_t amount = strlen(q);
-
-		memcpy(p, q, amount);
-		p += amount;
-
-		*p++ = ' ';
+	for (i = 0, j = 0; i < l; i++) {
+		unsigned char b = data_b[i ^ bs];
+		*p++ = "0123456789abcdef"[b >> 4];
+		*p++ = "0123456789abcdef"[b & 15];
+		if (++j == gs) {
+			j = 0;
+			*p++ = ' ';
+		}
 	}
 	if (i)
 		p--;
@@ -113,8 +61,10 @@ static void __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 			*p++ = ' ';
 		} while (p < test + rs * 2 + rs / gs + 1);
 
-		memcpy(p, data_a, l);
-		p += l;
+		for (i = 0; i < l; i++) {
+			unsigned char b = data_b[i];
+			*p++ = (isascii(b) && isprint(b)) ? b : '.';
+		}
 	}
 
 	*p = '\0';
-- 
2.39.5

Re: [PATCH next 2/8] test_hexdump: Create test output data from the binary input data buffer

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 09:34:46AM +0000, David Laight wrote:
> Using the same data that is passed to hex_dump_to_buffer() lets
> the tests be expanded to test different input data bytes.

Do we really need to kill the static test cases?
Are they anyhow harmful?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next 2/8] test_hexdump: Create test output data from the binary input data buffer

[David Laight]

On Mon, 10 Mar 2025 10:31:10 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Mar 08, 2025 at 09:34:46AM +0000, David Laight wrote:
> > Using the same data that is passed to hex_dump_to_buffer() lets
> > the tests be expanded to test different input data bytes.  
> 
> Do we really need to kill the static test cases?
> Are they anyhow harmful?
> 

I was asked to add some extra tests for other byte values.
The static result buffers just get in the way.

They are also not necessary since the tests are comparing the output
of two (hopefully) different implementations and checking they are
the same.

	David

Re: [PATCH next 2/8] test_hexdump: Create test output data from the binary input data buffer

[Andy Shevchenko]

On Wed, Mar 12, 2025 at 07:28:11PM +0000, David Laight wrote:
> On Mon, 10 Mar 2025 10:31:10 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, Mar 08, 2025 at 09:34:46AM +0000, David Laight wrote:
> > > Using the same data that is passed to hex_dump_to_buffer() lets
> > > the tests be expanded to test different input data bytes.  
> > 
> > Do we really need to kill the static test cases?
> > Are they anyhow harmful?
> 
> I was asked to add some extra tests for other byte values.

Right and thanks for doing that!

> The static result buffers just get in the way.
> 
> They are also not necessary since the tests are comparing the output
> of two (hopefully) different implementations and checking they are
> the same.

Not necessary doesn't mean harmful or working wrong. I would leave them as is
and just add a dynamic test cases on top. Static data is kinda randomised, but
at the same time it's always the same through the test runs. IIRC your dynamic
case generates the expected output and hence uses the code that also needs to
be tested strictly speaking.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next 2/8] test_hexdump: Create test output data from the binary input data buffer

[David Laight]

On Wed, 12 Mar 2025 21:36:04 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 12, 2025 at 07:28:11PM +0000, David Laight wrote:
> > On Mon, 10 Mar 2025 10:31:10 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Sat, Mar 08, 2025 at 09:34:46AM +0000, David Laight wrote:  
> > > > Using the same data that is passed to hex_dump_to_buffer() lets
> > > > the tests be expanded to test different input data bytes.    
> > > 
> > > Do we really need to kill the static test cases?
> > > Are they anyhow harmful?  
> > 
> > I was asked to add some extra tests for other byte values.  
> 
> Right and thanks for doing that!
> 
> > The static result buffers just get in the way.
> > 
> > They are also not necessary since the tests are comparing the output
> > of two (hopefully) different implementations and checking they are
> > the same.  
> 
> Not necessary doesn't mean harmful or working wrong. I would leave them as is
> and just add a dynamic test cases on top. Static data is kinda randomised, but
> at the same time it's always the same through the test runs. IIRC your dynamic
> case generates the expected output and hence uses the code that also needs to
> be tested strictly speaking.
> 

The old data wasn't really randomised at all.
It tended to run a selected tests lots of times.
As an example there is no point randomly selecting between running the
'rowsize == 16' and 'rowsize == 32' tests several times when you can just
run each test once.

While that can make sense for some kinds of tests it is pointless here.
Much better to always run the same tests and to always cover the 'interesting'
cases - there are lots of very boring cases that all go through the same code
paths pretty much regardless of the implementation.
It is the corner cases that matter.

At one point I was testing all 'bufsize * len * rowsize * groupsize' tests.
Doesn't actually take long even though there are a lot of tests.
But there is just no point testing all the cases where the buffer is long
enough for the data. The boundary conditions are enough - and there are
actually under a dozen of them. The proposed tests do about 1000 just to
save working out which ones matter.

	David

[PATCH next 3/8] test_hexdump: Use longer variable names

[David Laight]

We're not so short of memory space for the compiler symbol table
that variable names have to be limited to 2 characters.
Use groupsize and rowsize (not gs and rs).
There is also no need to copy the function parameters to locals.
Change test_hexdump_prepare_test() (the function that gernerate the
sample output) to return the length - saves calculating it separately.

Test reports 'all 1184 tests passed'.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 81 +++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 45 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index e142b11c36c6..743ea5c78f9e 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -22,32 +22,30 @@ static const unsigned char data_b[] = {
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 
-static void __init test_hexdump_prepare_test(size_t len, size_t rowsize,
-					     size_t groupsize, char *test,
-					     size_t testlen, bool ascii)
+static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
+					       size_t groupsize, char *test,
+					       size_t testlen, bool ascii)
 {
 	char *p;
-	size_t l = len;
-	size_t gs = groupsize, rs = rowsize;
-	size_t bs, i, j;
+	size_t byteswap, i, j;
 
-	if (rs != 16 && rs != 32)
-		rs = 16;
+	if (rowsize != 16 && rowsize != 32)
+		rowsize = 16;
 
-	if (l > rs)
-		l = rs;
+	if (len > rowsize)
+		len = rowsize;
 
-	if (!is_power_of_2(gs) || gs > 8 || (len % gs != 0))
-		gs = 1;
-	bs = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ? 0 : gs - 1;
+	if (!is_power_of_2(groupsize) || groupsize > 8 || (len % groupsize != 0))
+		groupsize = 1;
+	byteswap = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ? 0 : groupsize - 1;
 
 	/* hex dump */
 	p = test;
-	for (i = 0, j = 0; i < l; i++) {
-		unsigned char b = data_b[i ^ bs];
+	for (i = 0, j = 0; i < len; i++) {
+		unsigned char b = data_b[i ^ byteswap];
 		*p++ = "0123456789abcdef"[b >> 4];
 		*p++ = "0123456789abcdef"[b & 15];
-		if (++j == gs) {
+		if (++j == groupsize) {
 			j = 0;
 			*p++ = ' ';
 		}
@@ -59,15 +57,16 @@ static void __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 	if (ascii) {
 		do {
 			*p++ = ' ';
-		} while (p < test + rs * 2 + rs / gs + 1);
+		} while (p < test + rowsize * 2 + rowsize / groupsize + 1);
 
-		for (i = 0; i < l; i++) {
+		for (i = 0; i < len; i++) {
 			unsigned char b = data_b[i];
 			*p++ = (isascii(b) && isprint(b)) ? b : '.';
 		}
 	}
 
 	*p = '\0';
+	return p - test;
 }
 
 #define TEST_HEXDUMP_BUF_SIZE		(32 * 3 + 2 + 32 + 1)
@@ -113,44 +112,35 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 {
 	char test[TEST_HEXDUMP_BUF_SIZE];
 	char buf[TEST_HEXDUMP_BUF_SIZE];
-	size_t rs = rowsize, gs = groupsize;
-	size_t ae, he, e, f, r;
-	bool a;
+	size_t expected;
+	size_t f, result;
+	bool ok;
 
 	total_tests++;
 
 	memset(buf, FILL_CHAR, sizeof(buf));
 
-	r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen, ascii);
+	result = hex_dump_to_buffer(data_b, len, rowsize, groupsize, buf,
+				    buflen, ascii);
 
-	/*
-	 * Caller must provide the data length multiple of groupsize. The
-	 * calculations below are made with that assumption in mind.
-	 */
-	ae = rs * 2 /* hex */ + rs / gs /* spaces */ + 1 /* space */ + len /* ascii */;
-	he = (gs * 2 /* hex */ + 1 /* space */) * len / gs - 1 /* no trailing space */;
+	/* Test output is generated into a 'long enough' buffer */
+	expected = test_hexdump_prepare_test(len, rowsize, groupsize, test,
+					     sizeof(test), ascii);
 
-	if (ascii)
-		e = ae;
-	else
-		e = he;
-
-	f = min(e + 1, buflen);
-	if (buflen) {
-		test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
+	f = min(expected + 1, buflen);
+	if (f)
 		test[f - 1] = '\0';
-	}
 	memset(test + f, FILL_CHAR, sizeof(test) - f);
 
-	a = r == e && !memcmp(test, buf, TEST_HEXDUMP_BUF_SIZE);
+	ok = result == expected && !memcmp(test, buf, TEST_HEXDUMP_BUF_SIZE);
 
 	buf[sizeof(buf) - 1] = '\0';
 
-	if (!a) {
+	if (!ok) {
 		pr_err("Len: %zu buflen: %zu strlen: %zu\n",
 			len, buflen, strnlen(buf, sizeof(buf)));
-		pr_err("Result: %zu '%s'\n", r, buf);
-		pr_err("Expect: %zu '%s'\n", e, test);
+		pr_err("Result: %zu '%s'\n", result, buf);
+		pr_err("Expect: %zu '%s'\n", expected, test);
 		failed_tests++;
 	}
 }
@@ -158,13 +148,14 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
 {
 	unsigned int i = 0;
-	size_t rs = get_random_u32_inclusive(1, 2) * 16;
+	size_t rowsize = get_random_u32_inclusive(1, 2) * 16;
 
 	do {
-		size_t gs = 1 << i;
-		size_t len = get_random_u32_below(rs) + gs;
+		size_t groupsize = 1 << i;
+		size_t len = get_random_u32_below(rowsize) + groupsize;
 
-		test_hexdump_overflow(buflen, rounddown(len, gs), rs, gs, ascii);
+		test_hexdump_overflow(buflen, rounddown(len, groupsize),
+				      rowsize, groupsize, ascii);
 	} while (i++ < 3);
 }
 
-- 
2.39.5

[PATCH next 4/8] test_hexdump: Check for buffer overrun of sample output buffer

[David Laight]

While the output generated by test_hexdump_prepare_test() shouldn't
be longer than the size of the buffer passed, for safety verify that
the buffer is long enough.
If too short fill the buffer with an error message - output on
test failure.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 743ea5c78f9e..ed6f0b0a1bb3 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -39,6 +39,14 @@ static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 		groupsize = 1;
 	byteswap = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) ? 0 : groupsize - 1;
 
+	/* Check test passed a big enough output buffer */
+	if (ascii)
+		i = rowsize * 2 + rowsize / groupsize + 1 + len + 1;
+	else
+		i = len * 2 + len / groupsize - 1 + 1;
+	if (i > testlen)
+		return scnprintf(test, testlen, "buffer too short %zu < %zu", testlen, i);
+
 	/* hex dump */
 	p = test;
 	for (i = 0, j = 0; i < len; i++) {
-- 
2.39.5

Re: [PATCH next 4/8] test_hexdump: Check for buffer overrun of sample output buffer

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 09:34:48AM +0000, David Laight wrote:
> While the output generated by test_hexdump_prepare_test() shouldn't
> be longer than the size of the buffer passed, for safety verify that
> the buffer is long enough.
> If too short fill the buffer with an error message - output on
> test failure.

Isn't the function should behave snprintf() alike?
I think this patch is simply wrong because it's based on a wrong assumption.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next 4/8] test_hexdump: Check for buffer overrun of sample output buffer

[David Laight]

On Mon, 10 Mar 2025 11:01:29 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Mar 08, 2025 at 09:34:48AM +0000, David Laight wrote:
> > While the output generated by test_hexdump_prepare_test() shouldn't
> > be longer than the size of the buffer passed, for safety verify that
> > the buffer is long enough.
> > If too short fill the buffer with an error message - output on
> > test failure.  
> 
> Isn't the function should behave snprintf() alike?
> I think this patch is simply wrong because it's based on a wrong assumption.
> 

The normal hex_dump_to_buffer() is snprintf() like.

But, as the tests are written, test_hexdump_prepare_test() is always
passed a fixed size buffer that should be big enough.
The test control code then truncates the output to the required length
and fills the rest of the buffer with '#' before the compare.

The 'testlen' (output buffer length) parameter is actually ignored.
So if someone adds a test that would request output longer than the
buffer (actually requires the 'rowsize' limit be relaxed) then the
test code overruns the on-stack buffer.
While the control code could be rewritten that would be more changes.
So it seemed best just to force the test to fail and make it obvious why.

	David

[PATCH next 5/8] test_hexdump: Fix sample output if zero bytes requested

[David Laight]

If 'len' is zero the expected output is an empty string even if
'ascii' is requested.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index ed6f0b0a1bb3..07a8cc7e9088 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -29,6 +29,11 @@ static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 	char *p;
 	size_t byteswap, i, j;
 
+	if (!len) {
+		test[0] = 0;
+		return 0;
+	}
+
 	if (rowsize != 16 && rowsize != 32)
 		rowsize = 16;
 
@@ -58,8 +63,7 @@ static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 			*p++ = ' ';
 		}
 	}
-	if (i)
-		p--;
+	p--;
 
 	/* ASCII part */
 	if (ascii) {
-- 
2.39.5

Re: [PATCH next 5/8] test_hexdump: Fix sample output if zero bytes requested

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 09:34:49AM +0000, David Laight wrote:
> If 'len' is zero the expected output is an empty string even if
> 'ascii' is requested.

It's related to the previous one...
So, seems to me the series need a bit more (re-)thinking...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH next 5/8] test_hexdump: Fix sample output if zero bytes requested

[David Laight]

On Mon, 10 Mar 2025 11:03:07 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Mar 08, 2025 at 09:34:49AM +0000, David Laight wrote:
> > If 'len' is zero the expected output is an empty string even if
> > 'ascii' is requested.  
> 
> It's related to the previous one...
> So, seems to me the series need a bit more (re-)thinking...
> 

Nope, all its own bug.
This is a request to hexdump zero bytes into a valid buffer size.
hex_dump_to_buffer() gets it right unless the output buffer length
is also zero.

Found when I started doing a full range of tests.

	David

[PATCH next 6/8] test_hexdump: If a test fails print all the parameters

[David Laight]

Print the returned lengths as signed integers.
While they shouldn't be negative, -1 is better than 18446744073709551615.
The string outputs were also incorrectly truncated.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 07a8cc7e9088..56c0dfbd075b 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -146,13 +146,12 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 
 	ok = result == expected && !memcmp(test, buf, TEST_HEXDUMP_BUF_SIZE);
 
-	buf[sizeof(buf) - 1] = '\0';
-
 	if (!ok) {
-		pr_err("Len: %zu buflen: %zu strlen: %zu\n",
+		pr_err("Rowsize %zu, groupsize %zu, ascii %s, len %zu, buflen %zu, strlen %zu\n",
+			rowsize, groupsize, ascii ? "yes" : "no",
 			len, buflen, strnlen(buf, sizeof(buf)));
-		pr_err("Result: %zu '%s'\n", result, buf);
-		pr_err("Expect: %zu '%s'\n", expected, test);
+		pr_err("Result: %zd '%.*s'\n", result, TEST_HEXDUMP_BUF_SIZE, buf);
+		pr_err("Expect: %zd '%.*s'\n", expected, TEST_HEXDUMP_BUF_SIZE, test);
 		failed_tests++;
 	}
 }
-- 
2.39.5

[PATCH next 7/8] test_hexdump: Run fixed not random tests

[David Laight]

For reproducibilty instead of running 16 random length with two
random 'rowsize' picked from 16 and 32, run the tests with rowsize
16 and 32 and all the lengths from 1 to rowsize.
(This still includes all the cases where groupsize gets forced to 1.)

For the overflow tests increase 'len' if the data fits and 'buflen'
if it doesn't.

The is more tests than really needed but covers all the interesting
cases without generating a very large number of tests.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 64 +++++++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 56c0dfbd075b..877033a570d4 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -7,7 +7,6 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/random.h>
 #include <linux/string.h>
 
 static const unsigned char data_b[] = {
@@ -109,18 +108,19 @@ static void __init test_hexdump(size_t len, size_t rowsize, size_t groupsize,
 
 static void __init test_hexdump_set(size_t rowsize, bool ascii)
 {
-	size_t d = min(sizeof(data_b), rowsize);
-	size_t len = get_random_u32_inclusive(1, d);
+	size_t len;
 
-	test_hexdump(len, rowsize, 4, ascii);
-	test_hexdump(len, rowsize, 2, ascii);
-	test_hexdump(len, rowsize, 8, ascii);
-	test_hexdump(len, rowsize, 1, ascii);
+	for (len = 1; len <= rowsize; len++) {
+		test_hexdump(len, rowsize, 4, ascii);
+		test_hexdump(len, rowsize, 2, ascii);
+		test_hexdump(len, rowsize, 8, ascii);
+		test_hexdump(len, rowsize, 1, ascii);
+	}
 }
 
-static void __init test_hexdump_overflow(size_t buflen, size_t len,
-					 size_t rowsize, size_t groupsize,
-					 bool ascii)
+static bool __init test_hexdump_overflow(size_t buflen, size_t len,
+					   size_t rowsize, size_t groupsize,
+					   bool ascii)
 {
 	char test[TEST_HEXDUMP_BUF_SIZE];
 	char buf[TEST_HEXDUMP_BUF_SIZE];
@@ -154,40 +154,40 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
 		pr_err("Expect: %zd '%.*s'\n", expected, TEST_HEXDUMP_BUF_SIZE, test);
 		failed_tests++;
 	}
+
+	return expected < buflen;
 }
 
-static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
+static void __init test_hexdump_overflow_set(size_t rowsize, bool ascii)
 {
-	unsigned int i = 0;
-	size_t rowsize = get_random_u32_inclusive(1, 2) * 16;
-
-	do {
-		size_t groupsize = 1 << i;
-		size_t len = get_random_u32_below(rowsize) + groupsize;
-
-		test_hexdump_overflow(buflen, rounddown(len, groupsize),
-				      rowsize, groupsize, ascii);
-	} while (i++ < 3);
+	size_t groupsize, len, buflen;
+
+	for (groupsize = 1; groupsize <= 8; groupsize *= 2) {
+		len = 0;
+		buflen = 0;
+		for (; len <= rowsize && buflen <= TEST_HEXDUMP_BUF_SIZE;) {
+			if (test_hexdump_overflow(buflen, len, rowsize,
+						  groupsize, ascii))
+				len += groupsize;
+			else
+				buflen++;
+		}
+	}
 }
 
 static int __init test_hexdump_init(void)
 {
-	unsigned int i;
 	size_t rowsize;
 
-	rowsize = get_random_u32_inclusive(1, 2) * 16;
-	for (i = 0; i < 16; i++)
+	for (rowsize = 16; rowsize <= 32; rowsize += 16) {
 		test_hexdump_set(rowsize, false);
-
-	rowsize = get_random_u32_inclusive(1, 2) * 16;
-	for (i = 0; i < 16; i++)
 		test_hexdump_set(rowsize, true);
+	}
 
-	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
-		test_hexdump_overflow_set(i, false);
-
-	for (i = 0; i <= TEST_HEXDUMP_BUF_SIZE; i++)
-		test_hexdump_overflow_set(i, true);
+	test_hexdump_overflow_set(16, false);
+	test_hexdump_overflow_set(16, true);
+	test_hexdump_overflow_set(32, false);
+	test_hexdump_overflow_set(32, true);
 
 	if (failed_tests == 0)
 		pr_info("all %u tests passed\n", total_tests);
-- 
2.39.5

[PATCH next 8/8] test_hexdump: Test all 256 byte values

[David Laight]

Pass the hex data buffer as a parameter to test_hexdump_prepare_test()
and test_hexdump().
Add additional tests that pass all 256 byte values requesting 'ascii'.
Checks that unprintable characters are concerted to '.'.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 lib/test_hexdump.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 877033a570d4..d2ed1870f869 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -21,9 +21,9 @@ static const unsigned char data_b[] = {
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 
-static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
-					       size_t groupsize, char *test,
-					       size_t testlen, bool ascii)
+static size_t __init test_hexdump_prepare_test(const unsigned char *buf, size_t len,
+					       size_t rowsize, size_t groupsize,
+					       char *test, size_t testlen, bool ascii)
 {
 	char *p;
 	size_t byteswap, i, j;
@@ -54,7 +54,7 @@ static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 	/* hex dump */
 	p = test;
 	for (i = 0, j = 0; i < len; i++) {
-		unsigned char b = data_b[i ^ byteswap];
+		unsigned char b = buf[i ^ byteswap];
 		*p++ = "0123456789abcdef"[b >> 4];
 		*p++ = "0123456789abcdef"[b & 15];
 		if (++j == groupsize) {
@@ -71,7 +71,7 @@ static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 		} while (p < test + rowsize * 2 + rowsize / groupsize + 1);
 
 		for (i = 0; i < len; i++) {
-			unsigned char b = data_b[i];
+			unsigned char b = buf[i];
 			*p++ = (isascii(b) && isprint(b)) ? b : '.';
 		}
 	}
@@ -82,8 +82,8 @@ static size_t __init test_hexdump_prepare_test(size_t len, size_t rowsize,
 
 #define TEST_HEXDUMP_BUF_SIZE		(32 * 3 + 2 + 32 + 1)
 
-static void __init test_hexdump(size_t len, size_t rowsize, size_t groupsize,
-				bool ascii)
+static void __init test_hexdump(const void *buf, size_t len, size_t rowsize,
+				size_t groupsize, bool ascii)
 {
 	char test[TEST_HEXDUMP_BUF_SIZE];
 	char real[TEST_HEXDUMP_BUF_SIZE];
@@ -91,11 +91,11 @@ static void __init test_hexdump(size_t len, size_t rowsize, size_t groupsize,
 	total_tests++;
 
 	memset(real, FILL_CHAR, sizeof(real));
-	hex_dump_to_buffer(data_b, len, rowsize, groupsize, real, sizeof(real),
+	hex_dump_to_buffer(buf, len, rowsize, groupsize, real, sizeof(real),
 			   ascii);
 
 	memset(test, FILL_CHAR, sizeof(test));
-	test_hexdump_prepare_test(len, rowsize, groupsize, test, sizeof(test),
+	test_hexdump_prepare_test(buf, len, rowsize, groupsize, test, sizeof(test),
 				  ascii);
 
 	if (memcmp(test, real, TEST_HEXDUMP_BUF_SIZE)) {
@@ -111,10 +111,10 @@ static void __init test_hexdump_set(size_t rowsize, bool ascii)
 	size_t len;
 
 	for (len = 1; len <= rowsize; len++) {
-		test_hexdump(len, rowsize, 4, ascii);
-		test_hexdump(len, rowsize, 2, ascii);
-		test_hexdump(len, rowsize, 8, ascii);
-		test_hexdump(len, rowsize, 1, ascii);
+		test_hexdump(data_b, len, rowsize, 4, ascii);
+		test_hexdump(data_b, len, rowsize, 2, ascii);
+		test_hexdump(data_b, len, rowsize, 8, ascii);
+		test_hexdump(data_b, len, rowsize, 1, ascii);
 	}
 }
 
@@ -136,7 +136,7 @@ static bool __init test_hexdump_overflow(size_t buflen, size_t len,
 				    buflen, ascii);
 
 	/* Test output is generated into a 'long enough' buffer */
-	expected = test_hexdump_prepare_test(len, rowsize, groupsize, test,
+	expected = test_hexdump_prepare_test(data_b, len, rowsize, groupsize, test,
 					     sizeof(test), ascii);
 
 	f = min(expected + 1, buflen);
@@ -175,6 +175,22 @@ static void __init test_hexdump_overflow_set(size_t rowsize, bool ascii)
 	}
 }
 
+static void __init test_hexdump_bytes(void)
+{
+	unsigned char buf[16];
+	size_t i, j;
+
+	for (i = 0; i < 256; i += 16) {
+		for (j = 0; j < 16; j++)
+			buf[j] = i + j;
+		test_hexdump(buf, 16, 16, 1, true);
+		test_hexdump(buf, 16, 16, 2, true);
+		test_hexdump(buf, 16, 16, 4, true);
+		test_hexdump(buf, 16, 16, 8, true);
+	}
+}
+
+
 static int __init test_hexdump_init(void)
 {
 	size_t rowsize;
@@ -189,6 +205,8 @@ static int __init test_hexdump_init(void)
 	test_hexdump_overflow_set(32, false);
 	test_hexdump_overflow_set(32, true);
 
+	test_hexdump_bytes();
+
 	if (failed_tests == 0)
 		pr_info("all %u tests passed\n", total_tests);
 	else
-- 
2.39.5