[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202403010921.65C845411@keescook>
Date: Fri, 1 Mar 2024 09:25:07 -0800
From: Kees Cook <keescook@...omium.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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 01, 2024 at 01:09:27PM +0200, Andy Shevchenko wrote:
> 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?
Yes:
$ ./tools/testing/kunit/kunit.py run string
[09:21:32] Starting KUnit Kernel (1/1)...
[09:21:32] ============================================================
[09:21:32] =================== string (6 subtests) ====================
[09:21:32] [PASSED] test_memset16
[09:21:32] [PASSED] test_memset32
[09:21:32] [PASSED] test_memset64
[09:21:32] [PASSED] test_strchr
[09:21:32] [PASSED] test_strnchr
[09:21:32] [PASSED] test_strspn
[09:21:32] ===================== [PASSED] string ======================
[09:21:32] ============================================================
[09:21:32] Testing complete. Ran 6 tests: passed: 6
[09:21:32] Elapsed time: 11.545s total, 0.001s configuring, 11.327s building, 0.183s running
> ...
>
> > 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.
Yeah, I see now this isn't a test but rather an encoded report. Since
the failures are caught earlier, I can improve those messages instead of
doing an encoded version.
> 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)
This encoding is trying to report "i", so I've adjusted the error
reporting in v3.
> I dunno if KUnit has a fault ejection simulation. It should, in order
> to be sure that test cases are fine when they fail.
Yeah, bumping offsets and such produce expected failures.
--
Kees Cook
Powered by blists - more mailing lists