[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75VdhY6-OmNYtbpnb+dv1Du=LTnignsTRSU9+8yqpbFqPsQ@mail.gmail.com>
Date: Fri, 1 Mar 2024 13:09:27 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Andy Shevchenko <andy@...nel.org>, linux-hardening@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] string: Convert selftest to KUnit
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@...omium.org> wrote:
>
> Convert test_string.c to KUnit so it can be easily run with everything
> else.
Have you run it?
..
> if (i < 256)
> - return (i << 24) | (j << 16) | k | 0x8000;
> - return 0;
> + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
First of all, this special value encodes the problematic patterns, so
you missed proper messaging.
Second, the returned value has a constant, how do you expect 0 to be
equal to something (guaranteed not to be 0)?
This needs a good rethink of what you should do in the KUnit approach.
..
> + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
Ditto.
..
> + KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
Ditto.
..
> for (i = 0; i < strlen(test_string) + 1; i++) {
> result = strchr(test_string, test_string[i]);
> - if (result - test_string != i)
> - return i + 'a';
> + KUNIT_ASSERT_EQ(test, result - test_string, i);
In a similar way, all returned values are *special*, you really need
to think about them before converting to a simple (and sometimes
wrong) checks)
..
I dunno if KUnit has a fault ejection simulation. It should, in order
to be sure that test cases are fine when they fail.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists