linux-kernel - Re: [PATCH v2 3/4] kunit: Don't crash if no parameters are generated

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Oct 2021 11:59:55 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Rae Moar <rmr167@...il.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] kunit: Don't crash if no parameters are generated

On Tue, Oct 26, 2021 at 6:37 PM David Gow <davidgow@...gle.com> wrote:
>
> It's possible that a parameterised test could end up with zero
> parameters. At the moment, the test function will nevertheless be called
> with NULL as the parameter. Instead, don't try to run the test code, and
> just mark the test as SKIPped.
>
> Reported-by: Daniel Latypov <dlatypov@...gle.com>
> Signed-off-by: David Gow <davidgow@...gle.com>
> ---
>  lib/kunit/test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 3bd741e50a2d..e028d98e4f5b 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -500,7 +500,10 @@ int kunit_run_tests(struct kunit_suite *suite)
>         kunit_print_subtest_start(suite);
>
>         kunit_suite_for_each_test_case(suite, test_case) {
> -               struct kunit test = { .param_value = NULL, .param_index = 0 };
> +               /* The initial param value is nonzero, as we want
> +                * non-parametrised tests to run once.
> +                */
> +               struct kunit test = { .param_value = (void *)-1, .param_index = 0 };

(Not a strong preference)

Hmm, I'd slightly prefer we don't set a dummy value of -1 for this.
I personally think something like this is a bit less subtle:

/* Run non-parameterised tests once */
while (!test_case->generate_param || test.param_value) {

  if (!test_case->generate_param) break;
}

Alternatively, we don't need to share the loop

if (!test_case->generate_param) {
  kunit_run_case_catch_errors(suite, test_case, &test);
  kunit_update_stats(&param_stats, test.status);
} else while (test_param.value) {
   kunit_run_case_catch_errors(suite, test_case, &test);
   kunit_update_stats(&param_stats, test.status);
   /* print subtest and advance next param */
}

}


>                 struct kunit_result_stats param_stats = { 0 };
>                 test_case->status = KUNIT_SKIPPED;
>
> @@ -510,7 +513,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>                         test.param_value = test_case->generate_params(NULL, param_desc);
>                 }
>
> -               do {
> +               while (test.param_value) {
>                         kunit_run_case_catch_errors(suite, test_case, &test);
>
>                         if (test_case->generate_params) {
> @@ -530,11 +533,12 @@ int kunit_run_tests(struct kunit_suite *suite)
>                                 param_desc[0] = '\0';
>                                 test.param_value = test_case->generate_params(test.param_value, param_desc);
>                                 test.param_index++;
> -                       }
> +                       } else
> +                               test.param_value = NULL;
>
>                         kunit_update_stats(&param_stats, test.status);
>
> -               } while (test.param_value);
> +               }
>
>                 kunit_print_test_stats(&test, param_stats);
>
> --
> 2.33.0.1079.g6e70778dc9-goog
>