From: David Gow <davidgow@google.com>
To: Rae Moar <rmoar@google.com>
Cc: shuah@kernel.org, brendan.higgins@linux.dev,
ruanjinjie@huawei.com, linux-kselftest@vger.kernel.org,
kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org,
kernel test robot <lkp@intel.com>,
Dan Carpenter <dan.carpenter@linaro.org>
Subject: Re: [PATCH -next] kunit: fix uninitialized variables bug in attributes filtering
Date: Thu, 3 Aug 2023 16:15:34 +0800 [thread overview]
Message-ID: <CABVgOSkGL2QhcQ7067NddajXGOJvjN88O7OjdTs3YY2xgMSZPQ@mail.gmail.com> (raw)
In-Reply-To: <20230802212834.761327-1-rmoar@google.com>
[-- Attachment #1: Type: text/plain, Size: 8446 bytes --]
On Thu, 3 Aug 2023 at 05:28, Rae Moar <rmoar@google.com> 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@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202307270610.s0w4NKEn-lkp@intel.com/
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---
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 #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
next prev parent reply other threads:[~2023-08-03 8:21 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-02 21:28 [PATCH -next] kunit: fix uninitialized variables bug in attributes filtering Rae Moar
2023-08-03 8:15 ` David Gow [this message]
2023-08-03 19:39 ` Rae Moar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABVgOSkGL2QhcQ7067NddajXGOJvjN88O7OjdTs3YY2xgMSZPQ@mail.gmail.com \
--to=davidgow@google.com \
--cc=brendan.higgins@linux.dev \
--cc=dan.carpenter@linaro.org \
--cc=kunit-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=lkp@intel.com \
--cc=rmoar@google.com \
--cc=ruanjinjie@huawei.com \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.