Re: [PATCH v1] kunit: fix bug in KUNIT_EXPECT_MEMEQ

[David Gow]

On Wed, 25 Jan 2023 at 14:19, Rae Moar <rmoar@xxxxxxxxxx> wrote:
>
> In KUNIT_EXPECT_MEMEQ and KUNIT_EXPECT_MEMNEQ, add check if one of the
> inputs is NULL and fail if this is the case.
>
> Currently, the kernel crashes if one of the inputs is NULL. Instead,
> fail the test and add an appropriate error message.
>
> This was found by the kernel test robot:
> https://lore.kernel.org/all/202212191448.D6EDPdOh-lkp@xxxxxxxxx/
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>
> Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> ---

This looks good to me, modulo a small formatting issue with the
continuation backslashes (see below).

It might be worth considering this as a fix to the patch which first
introduced the MEMEQ macros with a Fixes tag, e.g.:
Fixes: b8a926bea8b1 ("kunit: Introduce KUNIT_EXPECT_MEMEQ and
KUNIT_EXPECT_MEMNEQ macros")

Otherwise, this is:
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

>  include/kunit/test.h |  7 ++++---
>  lib/kunit/assert.c   | 40 +++++++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 87ea90576b50..3c7045e22512 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -683,9 +683,10 @@ do {                                                                              \
>                 .right_text = #right,                                          \
>         };                                                                     \
>                                                                                \
> -       if (likely(memcmp(__left, __right, __size) op 0))                      \
> -               break;                                                         \
> -                                                                              \
> +       if (likely(__left && __right))                     \
> +               if (likely(memcmp(__left, __right, __size) op 0))            \
> +                       break;                                                         \
> +                                                                                              \

The backslashes no longer line up here.

>         _KUNIT_FAILED(test,                                                    \
>                       assert_type,                                             \
>                       kunit_mem_assert,                                        \
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index f5b50babe38d..05a09652f5a1 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -241,24 +241,34 @@ void kunit_mem_assert_format(const struct kunit_assert *assert,
>         mem_assert = container_of(assert, struct kunit_mem_assert,
>                                   assert);
>
> -       string_stream_add(stream,
> -                         KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> -                         mem_assert->text->left_text,
> -                         mem_assert->text->operation,
> -                         mem_assert->text->right_text);
> +       if (!mem_assert->left_value) {
> +               string_stream_add(stream,
> +                                 KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
> +                                 mem_assert->text->left_text);
> +       } else if (!mem_assert->right_value) {
> +               string_stream_add(stream,
> +                                 KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
> +                                 mem_assert->text->right_text);
> +       } else {
> +               string_stream_add(stream,
> +                               KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
> +                               mem_assert->text->left_text,
> +                               mem_assert->text->operation,
> +                               mem_assert->text->right_text);
>
> -       string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s ==\n",
> -                         mem_assert->text->left_text);
> -       kunit_assert_hexdump(stream, mem_assert->left_value,
> -                            mem_assert->right_value, mem_assert->size);
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s ==\n",
> +                               mem_assert->text->left_text);
> +               kunit_assert_hexdump(stream, mem_assert->left_value,
> +                                       mem_assert->right_value, mem_assert->size);
>
> -       string_stream_add(stream, "\n");
> +               string_stream_add(stream, "\n");
>
> -       string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s ==\n",
> -                         mem_assert->text->right_text);
> -       kunit_assert_hexdump(stream, mem_assert->right_value,
> -                            mem_assert->left_value, mem_assert->size);
> +               string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s ==\n",
> +                               mem_assert->text->right_text);
> +               kunit_assert_hexdump(stream, mem_assert->right_value,
> +                                       mem_assert->left_value, mem_assert->size);
>
> -       kunit_assert_print_msg(message, stream);
> +               kunit_assert_print_msg(message, stream);
> +       }
>  }
>  EXPORT_SYMBOL_GPL(kunit_mem_assert_format);
>
> base-commit: 5cb26c298ffde271d9bd1dd1b87ad028218f77fe
> --
> 2.39.1.405.gd4c25cc71f-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature