Re: [PATCH v1 1/3] kunit: fix bug in debugfs logs of parameterized tests

[Rae Moar]

On Thu, Feb 9, 2023 at 12:06 AM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Wed, 1 Feb 2023 at 06:04, Rae Moar <rmoar@xxxxxxxxxx> wrote:
> >
> > Fix bug in debugfs logs that causes parameterized results to not appear
> > in the log because the log is reintialized (cleared) when each parameter is
>

Hi David!

> Nit: s/reintialized/reinitialized
>

Oops. Thanks for pointing this out. Will fix in v2.

> > run.
> >
> > Ensure these results appear in the debugfs logs and increase log size to
> > allow for the size of parameterized results.
> >
> > Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> > ---
>
> This looks pretty good to me, but we may need to restrict the size of
> a single log line separately from that of the whole log.
>
> (It'd also be nice to include a before/after in the commit description.)

Yes, as mentioned in the other patches, I will add an individual
"before and after" comparison to each of the patches in v2. This is
much clearer than just the overall comparison in the cover letter.

>
> With the stack size issue fixed, though, this looks good to me:
> Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
>
> Cheers,
> -- David
>
> >  include/kunit/test.h | 2 +-
> >  lib/kunit/test.c     | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 87ea90576b50..0a077a4c067c 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -34,7 +34,7 @@ DECLARE_STATIC_KEY_FALSE(kunit_running);
> >  struct kunit;
> >
> >  /* Size of log associated with test. */
> > -#define KUNIT_LOG_SIZE 512
> > +#define KUNIT_LOG_SIZE 1500
>
> This is used both as the overall log size, and the size of a single
> line in kunit_log_append.
>
> As the latter involves a local 'line' array, it can bloat out stack usage.
>
> Could we either restrict the maximum line length separately, or rework
> kunit_log_append() to not use a local variable?
> (I imagine we could just vsnprintf() directly into the log buffer. We
> could make two sprintf calls to calculate the length required to
> maintain some atomicity as well (this could open us up to
> time-of-check/time-of-use vulnerabilities, but I think the
> vulnerability ship has sailed if you're passing untrusted pointers
> into the kunit_log macro anyway))
>

Thanks for your help here. I will play around with these two options.
Although, I think I am leaning toward restricting the maximum line
length separately.

Thanks!

-Rae

> >
> >  /* Maximum size of parameter description string. */
> >  #define KUNIT_PARAM_DESC_SIZE 128
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index 51cae59d8aae..66ba93b8222c 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -437,7 +437,6 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
> >         struct kunit_try_catch_context context;
> >         struct kunit_try_catch *try_catch;
> >
> > -       kunit_init_test(test, test_case->name, test_case->log);
> >         try_catch = &test->try_catch;
> >
> >         kunit_try_catch_init(try_catch,
> > @@ -533,6 +532,8 @@ int kunit_run_tests(struct kunit_suite *suite)
> >                 struct kunit_result_stats param_stats = { 0 };
> >                 test_case->status = KUNIT_SKIPPED;
> >
> > +               kunit_init_test(&test, test_case->name, test_case->log);
> > +
> >                 if (!test_case->generate_params) {
> >                         /* Non-parameterised test. */
> >                         kunit_run_case_catch_errors(suite, test_case, &test);
> > --
> > 2.39.1.456.gfc5497dd1b-goog
> >