On Thu, Aug 3, 2023 at 4:15 AM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> 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.
>
Hi!
Great point. I will definitely change this. This is definitely a flaw
in the code. I have sent out a v2 of this patch with your suggested
changes above. Let me know what you think.
Thanks!
-Rae
> 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
> >
