Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1272998 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-19 11:20 +0100
    Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-11-20 18:00 +0100
      Re: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 10:30 +0100

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

On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevchenko@linux.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@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?

>  	} 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1274268

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

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

[toc] | [prev] | [next] | [standalone]

#1275152

On Fri, Nov 20 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, 2015-11-19 at 11:11 +0100, Rasmus Villemoes wrote:
>> 
>> 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.
>
>> > -			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.

Oh, but that's wrong! That is, printing the filled test buffer does not
actually show what was "expected", whether you were filling with spaces,
$ or \xff. I really can't think of a situation where you'd want to print
the supposedly unused part of the buffer, so the character used for
filling shouldn't matter.

In any case, the most important thing is to stay away from [ a-f0-9]
which hexdump is certainly expected to produce - the reason I suggested
a non-ascii character was because hexdump might produce any printable
ascii character in the ascii part (but I suppose one could use e.g. '#'
which isn't among the characters we feed it).

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

No, buflen==0 is not that special. We expect hexdump to touch exactly
the first buflen bytes (or only e+1, whichever is smaller), and the
remaining bytes should be untouched. A memcmp handles the first part, a
memchr_inv the second.

Rasmus



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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web