Re: [PATCH v2 1/2] string: Convert selftest to KUnit

[Kees Cook]

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@xxxxxxxxxxxx> 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