| Message ID | 3ff9d019-75b6-45ee-9b03-9d6ec7c5364f@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Check for kunit_parse_glob_filter() failure | expand |
On Fri, Sep 15, 2023 at 8:58 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > Smatch complains that the missing error checks would lead to a crash: > > lib/kunit/executor_test.c:40 parse_filter_test() > error: double free of 'filter.test_glob' > > We may as well do it right... > > Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Hello! We definitely should add in checks for these errors. I have a couple comments below. Thanks! -Rae > --- > lib/kunit/executor_test.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > index b4f6f96b2844..176c9c9dfcfc 100644 > --- a/lib/kunit/executor_test.c > +++ b/lib/kunit/executor_test.c > @@ -27,13 +27,15 @@ static void parse_filter_test(struct kunit *test) > { > struct kunit_glob_filter filter = {NULL, NULL}; > > - kunit_parse_glob_filter(&filter, "suite"); > + if (!kunit_parse_glob_filter(&filter, "suite")) > + return; First, this is returning every time this test is run because the kunit_parse_glob_filter returns 0 when there is no error. So this should instead be checking for a result of not equal to 0. Secondly, this should fail the test if the parsing returns an error. So instead of returning I would recommend using a KUNIT_ASSERT statement to check the result of this method is equal to 0. > KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite"); > KUNIT_EXPECT_FALSE(test, filter.test_glob); > kfree(filter.suite_glob); > kfree(filter.test_glob); > > - kunit_parse_glob_filter(&filter, "suite.test"); > + if (!kunit_parse_glob_filter(&filter, "suite.test")) > + return; Similar to above I think this should be changed to a KUNIT_ASSERT statement to ensure the result is equal to 0. > KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite"); > KUNIT_EXPECT_STREQ(test, filter.test_glob, "test"); > kfree(filter.suite_glob); > -- > 2.39.2 > > -- > 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/3ff9d019-75b6-45ee-9b03-9d6ec7c5364f%40moroto.mountain.
On Tue, Sep 19, 2023 at 02:49:43PM -0400, Rae Moar wrote: > On Fri, Sep 15, 2023 at 8:58 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > Smatch complains that the missing error checks would lead to a crash: > > > > lib/kunit/executor_test.c:40 parse_filter_test() > > error: double free of 'filter.test_glob' > > > > We may as well do it right... > > > > Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > Hello! > > We definitely should add in checks for these errors. I have a couple > comments below. > > Thanks! > -Rae > > > --- > > lib/kunit/executor_test.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c > > index b4f6f96b2844..176c9c9dfcfc 100644 > > --- a/lib/kunit/executor_test.c > > +++ b/lib/kunit/executor_test.c > > @@ -27,13 +27,15 @@ static void parse_filter_test(struct kunit *test) > > { > > struct kunit_glob_filter filter = {NULL, NULL}; > > > > - kunit_parse_glob_filter(&filter, "suite"); > > + if (!kunit_parse_glob_filter(&filter, "suite")) > > + return; > > First, this is returning every time this test is run because the > kunit_parse_glob_filter returns 0 when there is no error. So this > should instead be checking for a result of not equal to 0. Oh... Duh. Sorry. That's embarrassing. > > Secondly, this should fail the test if the parsing returns an error. > So instead of returning I would recommend using a KUNIT_ASSERT > statement to check the result of this method is equal to 0. Will do. Thanks! regards, dan carpenter
diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c index b4f6f96b2844..176c9c9dfcfc 100644 --- a/lib/kunit/executor_test.c +++ b/lib/kunit/executor_test.c @@ -27,13 +27,15 @@ static void parse_filter_test(struct kunit *test) { struct kunit_glob_filter filter = {NULL, NULL}; - kunit_parse_glob_filter(&filter, "suite"); + if (!kunit_parse_glob_filter(&filter, "suite")) + return; KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite"); KUNIT_EXPECT_FALSE(test, filter.test_glob); kfree(filter.suite_glob); kfree(filter.test_glob); - kunit_parse_glob_filter(&filter, "suite.test"); + if (!kunit_parse_glob_filter(&filter, "suite.test")) + return; KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite"); KUNIT_EXPECT_STREQ(test, filter.test_glob, "test"); kfree(filter.suite_glob);
Smatch complains that the missing error checks would lead to a crash: lib/kunit/executor_test.c:40 parse_filter_test() error: double free of 'filter.test_glob' We may as well do it right... Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- lib/kunit/executor_test.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)