Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, 2015-11-19 at 11:11 +0100, Rasmus Villemoes wrote:
> On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.c
> om> 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

Okay, I may change the ' ' to something, but somehow printable. See
also below.

>  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@linux.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?

Here I feel the test buffer just to print below if any error happens
when buflen == 0.

That's why I would like to have a somehow printable character.

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

See above. buflen == 0 is a special case where buffer shouldn't be
touched at all.

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

Can do that.

> 
> Rasmus

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy