On Wed, Nov 11 2015, Andy Shevchenko <andriy.shevche...@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.shevche...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
