lkml.org 
[lkml]   [2015]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v1 5/7] test_hexdump: check all bytes in real buffer
From
Date
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



\
 
 \ /
  Last update: 2015-11-20 18:21    [W:0.082 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site