Date:	Thu, 19 Nov 2015 11:11:51 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

> After processing by hex_dump_to_buffer() check all the parts to be expected.
>
> Part 1. The actual expected hex dump with or without ASCII part.
> 	This is provided by plain strcmp() call including check for the
> 	terminating NUL.
>
> Part 2. Check if the buffer is dirty beyond needed.
> 	We fill the buffer by ' ' (space) characters, so, we expect to have the
> 	tail of buffer will be left untouched. Check all bytes in the tail of
> 	the buffer.

First of all, ' ' is one of the characters which hexdump is certainly supposed
to spit out, so I think it's better to use some other character for
prefilling. Otherwise we wouldn't be able to detect a stray write of a
space which wasn't properly guarded by a size check. I'd suggest
'\xff' or any other non-ascii character (and make it a #define so that
it's less magic).


> Part 3. Return code should be as expected.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.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 a3e3b01..9b95b67 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -128,10 +128,9 @@ static void __init test_hexdump_set(int rowsize, bool ascii)
>  
>  static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  {
> +	char test[TEST_HEXDUMP_BUF_SIZE];
>  	char buf[TEST_HEXDUMP_BUF_SIZE];
> -	const char *t = test_data_1_le[0];
>  	size_t len = 1;
> -	size_t l = buflen;
>  	int rs = 16, gs = 1;
>  	int ae, he, e, r;
>  	bool a;
> @@ -147,26 +146,27 @@ static void __init test_hexdump_overflow(size_t buflen, bool ascii)
>  		e = ae;
>  	else
>  		e = he;
> -	buf[e + 2] = '\0';
>  
>  	if (!buflen) {
> -		a = r == e && buf[0] == ' ';
> -	} else if (l < 3) {
> -		a = r == e && buf[0] == '\0';
> -	} else if (l < 4) {
> -		a = r == e && !strcmp(buf, t);
> -	} else if (ascii) {
> -		if (l < 51)
> -			a = r == e && buf[l - 1] == '\0' && buf[l - 2] == ' ';
> -		else
> -			a = r == e && buf[50] == '\0' && buf[49] == '.';
> +		memset(test, ' ', sizeof(test));
> +		test[sizeof(buf) - 1] = '\0';
> +
> +		a = r == e && !memchr_inv(buf, ' ', sizeof(buf));

test and buf happen to have the same size, but
"test[sizeof(buf) - 1] = '\0'" is rather odd. But you don't even seem
to use test in this branch?

>  	} else {
> -		a = r == e && buf[e] == '\0';
> +		int f = min_t(int, e + 1, buflen);
> +
> +		test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
> +		test[f - 1] = '\0';
> +
> +		a = r == e && !memchr_inv(buf + f, ' ', sizeof(buf) - f) && !strcmp(buf, test);
>  	}

There's also a bit of duplication in the !buflen and buflen
branches. Why not pull the computation of f (the number of expected
bytes written) outside and do

  f = min_t(int, e + 1, buflen);
  a = r == e && !memchr_inv(buf + f, ' ', sizeof(buf) - f);
  if (buflen) {
    test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
    test[f - 1] = '\0';
    a = a && !memcmp(buf, test, f);
  }

(I think it's better to use memcmp for "untrusted" buffers - if
hexdump didn't make buf into a proper C string, it's a little fragile
passing it to strcmp). This makes it obvious that the entire contents
of buf is being tested.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists