| Message ID | 20230831071655.2907683-4-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Fix some bugs in kunit_filter_suites() | expand |
On Thu, Aug 31, 2023 at 3:17 AM 'Jinjie Ruan' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > If both filter_glob and filters are not NULL, and kunit_parse_glob_filter() > succeed, but kcalloc parsed_filters fails, the suite_glob and test_glob of > parsed kzalloc in kunit_parse_glob_filter() will be leaked. > > Fixes: 1c9fd080dffe ("kunit: fix uninitialized variables bug in attributes filtering") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> Hello! Thanks for catching this! I just had two concerns listed below. -Rae > --- > lib/kunit/executor.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 25ce8d6e5fe7..7654c09c1ab1 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -176,10 +176,8 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > if (filters) { > filter_count = kunit_get_filter_count(filters); > parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL); > - if (!parsed_filters) { > - kfree(copy); > - return filtered; > - } > + if (!parsed_filters) > + goto err; When this goes to the "err" label this will likely enter the if (filter_count) statement and attempt to free parsed_filters. Since parsed_filters is NULL in this case, it could be a cleaner practice to set a new label, maybe "filter_err", to skip over that if statement. Although, I realize this may require the order of the patches to switch. Additionally, the value of *err should definitely be set here before "goto err". The kunit_run_all_tests() function checks this err value in order to produce the correct error message and to prevent any attempt to run tests. I would suggest: *err = -ENOMEM; > for (j = 0; j < filter_count; j++) > parsed_filters[j] = kunit_next_attr_filter(&filters, err); > if (*err) > -- > 2.34.1 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230831071655.2907683-4-ruanjinjie%40huawei.com.
On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > If both filter_glob and filters are not NULL, and kunit_parse_glob_filter() > succeed, but kcalloc parsed_filters fails, the suite_glob and test_glob of > parsed kzalloc in kunit_parse_glob_filter() will be leaked. > > Fixes: 1c9fd080dffe ("kunit: fix uninitialized variables bug in attributes filtering") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- As Rae points out, I don't think this is correct. If filter_count is non-zero, but parsed_filters is NULL due to an allocation failure, we'll still end up trying to kfree() a NULL pointer. That's why this didn't goto err; before: we explicitly _don't_ want to free parsed_filters here. We could set filter_count = 0 before goto err, but I think either wrapping the kfree() call in if (parsed_filters), or Indeed, this may be the case for most of the cleanup here: we're checking if we intended to allocate something (e.g., the filter_count is nonzero, the filter_glob is set), rather than whether we successfully allocated something, so can kfree(NULL) on failure. The issues with ordering (that you fix in the next patch) could help if we had several labels to jump to after each allocation, or just checking that what we're freeing was non-NULL (and initialising them to NULL if needed). Thanks, -- David > lib/kunit/executor.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 25ce8d6e5fe7..7654c09c1ab1 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -176,10 +176,8 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > if (filters) { > filter_count = kunit_get_filter_count(filters); > parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL); > - if (!parsed_filters) { > - kfree(copy); > - return filtered; > - } > + if (!parsed_filters) > + goto err; > for (j = 0; j < filter_count; j++) > parsed_filters[j] = kunit_next_attr_filter(&filters, err); > if (*err) > -- > 2.34.1 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230831071655.2907683-4-ruanjinjie%40huawei.com.
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 25ce8d6e5fe7..7654c09c1ab1 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -176,10 +176,8 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, if (filters) { filter_count = kunit_get_filter_count(filters); parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL); - if (!parsed_filters) { - kfree(copy); - return filtered; - } + if (!parsed_filters) + goto err; for (j = 0; j < filter_count; j++) parsed_filters[j] = kunit_next_attr_filter(&filters, err); if (*err)
If both filter_glob and filters are not NULL, and kunit_parse_glob_filter() succeed, but kcalloc parsed_filters fails, the suite_glob and test_glob of parsed kzalloc in kunit_parse_glob_filter() will be leaked. Fixes: 1c9fd080dffe ("kunit: fix uninitialized variables bug in attributes filtering") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- lib/kunit/executor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)