Re: [PATCH -next] kunit: fix uninitialized variables bug in attributes filtering

[David Gow]

On Thu, 3 Aug 2023 at 05:28, Rae Moar <rmoar@xxxxxxxxxx> wrote:
>
> Fix smatch warnings regarding uninitialized variables in the filtering
> patch of the new KUnit Attributes feature.
>
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Closes: https://lore.kernel.org/r/202307270610.s0w4NKEn-lkp@xxxxxxxxx/
>
> Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> ---

These fixes look good, especially the ones in attributes.c.

There's still a possibility of returning uninitialised or freed
pointers in executor.c. If we can keep 'filtered' valid at all times,
this should be easier to deal with, e.g.:

- Initialise 'filtered' to {NULL, NULL}, which is a valid "empty" value.
- Only ever set start and end at the same time, so don't set 'start'
immediately after allocation.
- Wait until the filtering is complete and successful (i.e., where
'end' is set now), and set 'start' there as well.
- Then return filtered will definitely either return the completely
filtered value, or a valid empty suite_set.

Otherwise, this looks good.

-- David

>
> Note that this is rebased on top of the recent fix:
> ("kunit: fix possible memory leak in kunit_filter_suites()").
>
>  lib/kunit/attributes.c | 40 +++++++++++++++++-----------------------
>  lib/kunit/executor.c   | 10 +++++++---
>  2 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/lib/kunit/attributes.c b/lib/kunit/attributes.c
> index d37c40c0ce4f..5e3034b6be99 100644
> --- a/lib/kunit/attributes.c
> +++ b/lib/kunit/attributes.c
> @@ -102,7 +102,7 @@ static int int_filter(long val, const char *op, int input, int *err)
>  static int attr_enum_filter(void *attr, const char *input, int *err,
>                 const char * const str_list[], int max)
>  {
> -       int i, j, input_int;
> +       int i, j, input_int = -1;
>         long test_val = (long)attr;
>         const char *input_val = NULL;
>
> @@ -124,7 +124,7 @@ static int attr_enum_filter(void *attr, const char *input, int *err,
>                         input_int = j;
>         }
>
> -       if (!input_int) {
> +       if (input_int < 0) {
>                 *err = -EINVAL;
>                 pr_err("kunit executor: invalid filter input: %s\n", input);
>                 return false;
> @@ -186,8 +186,10 @@ static void *attr_module_get(void *test_or_suite, bool is_test)
>         // Suites get their module attribute from their first test_case
>         if (test)
>                 return ((void *) test->module_name);
> -       else
> +       else if (kunit_suite_num_test_cases(suite) > 0)
>                 return ((void *) suite->test_cases[0].module_name);
> +       else
> +               return (void *) "";
>  }
>
>  /* List of all Test Attributes */
> @@ -221,7 +223,7 @@ const char *kunit_attr_filter_name(struct kunit_attr_filter filter)
>  void kunit_print_attr(void *test_or_suite, bool is_test, unsigned int test_level)
>  {
>         int i;
> -       bool to_free;
> +       bool to_free = false;
>         void *attr;
>         const char *attr_name, *attr_str;
>         struct kunit_suite *suite = is_test ? NULL : test_or_suite;
> @@ -255,7 +257,7 @@ void kunit_print_attr(void *test_or_suite, bool is_test, unsigned int test_level
>
>  int kunit_get_filter_count(char *input)
>  {
> -       int i, comma_index, count = 0;
> +       int i, comma_index = 0, count = 0;
>
>         for (i = 0; input[i]; i++) {
>                 if (input[i] == ',') {
> @@ -272,7 +274,7 @@ int kunit_get_filter_count(char *input)
>  struct kunit_attr_filter kunit_next_attr_filter(char **filters, int *err)
>  {
>         struct kunit_attr_filter filter = {};
> -       int i, j, comma_index, new_start_index;
> +       int i, j, comma_index = 0, new_start_index = 0;
>         int op_index = -1, attr_index = -1;
>         char op;
>         char *input = *filters;
> @@ -316,7 +318,7 @@ struct kunit_attr_filter kunit_next_attr_filter(char **filters, int *err)
>                 filter.attr = &kunit_attr_list[attr_index];
>         }
>
> -       if (comma_index) {
> +       if (comma_index > 0) {
>                 input[comma_index] = '\0';
>                 filter.input = input + op_index;
>                 input = input + new_start_index;
> @@ -356,31 +358,22 @@ struct kunit_suite *kunit_filter_attr_tests(const struct kunit_suite *const suit
>
>         /* Save filtering result on default value */
>         default_result = filter.attr->filter(filter.attr->attr_default, filter.input, err);
> -       if (*err) {
> -               kfree(copy);
> -               kfree(filtered);
> -               return NULL;
> -       }
> +       if (*err)
> +               goto err;
>
>         /* Save suite attribute value and filtering result on that value */
>         suite_val = filter.attr->get_attr((void *)suite, false);
>         suite_result = filter.attr->filter(suite_val, filter.input, err);
> -       if (*err) {
> -               kfree(copy);
> -               kfree(filtered);
> -               return NULL;
> -       }
> +       if (*err)
> +               goto err;
>
>         /* For each test case, save test case if passes filtering. */
>         kunit_suite_for_each_test_case(suite, test_case) {
>                 test_val = filter.attr->get_attr((void *) test_case, true);
>                 test_result = filter.attr->filter(filter.attr->get_attr(test_case, true),
>                                 filter.input, err);
> -               if (*err) {
> -                       kfree(copy);
> -                       kfree(filtered);
> -                       return NULL;
> -               }
> +               if (*err)
> +                       goto err;
>
>                 /*
>                  * If attribute value of test case is set, filter on that value.
> @@ -406,7 +399,8 @@ struct kunit_suite *kunit_filter_attr_tests(const struct kunit_suite *const suit
>                 }
>         }
>
> -       if (n == 0) {
> +err:
> +       if (n == 0 || *err) {
>                 kfree(copy);
>                 kfree(filtered);
>                 return NULL;
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 481901d245d0..b6e07de2876a 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -130,7 +130,7 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
>         struct kunit_suite **copy, *filtered_suite, *new_filtered_suite;
>         struct suite_set filtered;
>         struct kunit_glob_filter parsed_glob;
> -       struct kunit_attr_filter *parsed_filters;
> +       struct kunit_attr_filter *parsed_filters = NULL;
>
>         const size_t max = suite_set->end - suite_set->start;
>
> @@ -147,7 +147,11 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
>         /* Parse attribute filters */
>         if (filters) {
>                 filter_count = kunit_get_filter_count(filters);
> -               parsed_filters = kcalloc(filter_count + 1, sizeof(*parsed_filters), GFP_KERNEL);
> +               parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL);
> +               if (!parsed_filters) {
> +                       kfree(copy);
> +                       return filtered;

Is 'filtered' properly initialised here?
filtered.start is already set to 'copy' by this point (so, having
freed 'copy', this would now be an invalid pointer).
filtered.end is uninitialised.

Can we instead initialise filtered to {NULL, NULL} at the start, and
only set start and end after the filtering has succeeded?

> +               }
>                 for (j = 0; j < filter_count; j++)
>                         parsed_filters[j] = kunit_next_attr_filter(&filters, err);
>                 if (*err)
> @@ -166,7 +170,7 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
>                                 goto err;
>                         }
>                 }
> -               if (filter_count) {
> +               if (filter_count > 0 && parsed_filters != NULL) {
>                         for (k = 0; k < filter_count; k++) {
>                                 new_filtered_suite = kunit_filter_attr_tests(filtered_suite,
>                                                 parsed_filters[k], filter_action, err);
>
> base-commit: 3bffe185ad11e408903d2782727877388d08d94e
> --
> 2.41.0.585.gd2178a4bd4-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature