| Message ID | 20230927165058.29484-1-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: debugfs: Handle errors from alloc_string_stream() | expand |
On Wed, Sep 27, 2023 at 12:51 PM 'Richard Fitzgerald' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > In kunit_debugfs_create_suite() give up and skip creating the debugfs > file if any of the alloc_string_stream() calls return an error or NULL. > Only put a value in the log pointer of kunit_suite and kunit_test if it > is a valid pointer to a log. > > This prevents the potential invalid dereference reported by smatch: > > lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' > dereferencing possible ERR_PTR() > lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' > dereferencing possible ERR_PTR() > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") Hi! I've tested this and this all looks good to me. Reviewed-by: Rae Moar <rmoar@google.com> Thanks! Rae > --- > lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..73075ca6e88c 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { > void kunit_debugfs_create_suite(struct kunit_suite *suite) > { > struct kunit_case *test_case; > + struct string_stream *stream; > > - /* Allocate logs before creating debugfs representation. */ > - suite->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(suite->log, true); > + /* > + * Allocate logs before creating debugfs representation. > + * The log pointer must be NULL if there isn't a log so only > + * set it if the log stream was created successfully. > + */ > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; > + > + string_stream_set_append_newlines(stream, true); > + suite->log = stream; > > kunit_suite_for_each_test_case(suite, test_case) { > - test_case->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(test_case->log, true); > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; > + > + string_stream_set_append_newlines(stream, true); > + test_case->log = stream; > } > > suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); > @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) > debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, > suite->debugfs, > suite, &debugfs_results_fops); > + return; > + > +err: > + string_stream_destroy(suite->log); > + kunit_suite_for_each_test_case(suite, test_case) > + string_stream_destroy(test_case->log); > } > > void kunit_debugfs_destroy_suite(struct kunit_suite *suite) > -- > 2.30.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/20230927165058.29484-1-rf%40opensource.cirrus.com.
On Wed, Sep 27, 2023 at 05:50:58PM +0100, Richard Fitzgerald wrote: > In kunit_debugfs_create_suite() give up and skip creating the debugfs > file if any of the alloc_string_stream() calls return an error or NULL. > Only put a value in the log pointer of kunit_suite and kunit_test if it > is a valid pointer to a log. > > This prevents the potential invalid dereference reported by smatch: > > lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' > dereferencing possible ERR_PTR() > lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' > dereferencing possible ERR_PTR() > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") > --- > lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..73075ca6e88c 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { > void kunit_debugfs_create_suite(struct kunit_suite *suite) > { > struct kunit_case *test_case; > + struct string_stream *stream; > > - /* Allocate logs before creating debugfs representation. */ > - suite->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(suite->log, true); > + /* > + * Allocate logs before creating debugfs representation. > + * The log pointer must be NULL if there isn't a log so only > + * set it if the log stream was created successfully. > + */ > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; So when you're programming, there is an aspect where you are telling the computer what to do, but there is also an aspect where you are communicating to other programmers. Checking for IS_ERR_OR_NULL() works in terms of computers, but in terms of communicating to humans it's misleading. And to be honest the comments are even more confusing because they suggest something complicated is happening so I had to review the code extra carefully. When a function returns both error pointers and NULL then it means it is an optional feature which can be disabled. Error pointers means errors. NULL means disabled. So typically the IS_ERR_OR_NULL() or NULL check looks like this: blinky_lights = get_blinky(); if (IS_ERR_OR_NULL(blinky_lights)) return PTR_ERR(blinky_lights); blinky_lights->blink(); return 0; This means that the function requires the blinky feature to continue. If blinky_lights is an error pointer then it returns an error. If blinky_lights is NULL then PTR_ERR(NULL) is zero which is success. We didn't blink the lights but only because the admin told us not to. It's a no-op but it's not an error. In this case, alloc_string_stream() is not optional. It only returns error pointers. It can never return NULL. I have written a blog about this in more detail but already this email is probably longer than my blog was. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ regards, dan carpenter
On 27/09/2023 17:50, Richard Fitzgerald wrote: > In kunit_debugfs_create_suite() give up and skip creating the debugfs > file if any of the alloc_string_stream() calls return an error or NULL. > Only put a value in the log pointer of kunit_suite and kunit_test if it > is a valid pointer to a log. > > This prevents the potential invalid dereference reported by smatch: > > lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' > dereferencing possible ERR_PTR() > lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' > dereferencing possible ERR_PTR() > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") > --- > lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 270d185737e6..73075ca6e88c 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { > void kunit_debugfs_create_suite(struct kunit_suite *suite) > { > struct kunit_case *test_case; > + struct string_stream *stream; > > - /* Allocate logs before creating debugfs representation. */ > - suite->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(suite->log, true); > + /* > + * Allocate logs before creating debugfs representation. > + * The log pointer must be NULL if there isn't a log so only > + * set it if the log stream was created successfully. > + */ > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; This can be a return. Nothing has been created at this point so there is nothing to clean up. I'll send a V2. > + > + string_stream_set_append_newlines(stream, true); > + suite->log = stream; > > kunit_suite_for_each_test_case(suite, test_case) { > - test_case->log = alloc_string_stream(GFP_KERNEL); > - string_stream_set_append_newlines(test_case->log, true); > + stream = alloc_string_stream(GFP_KERNEL); > + if (IS_ERR_OR_NULL(stream)) > + goto err; > + > + string_stream_set_append_newlines(stream, true); > + test_case->log = stream; > } > > suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); > @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) > debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, > suite->debugfs, > suite, &debugfs_results_fops); > + return; > + > +err: > + string_stream_destroy(suite->log); > + kunit_suite_for_each_test_case(suite, test_case) > + string_stream_destroy(test_case->log); > } > > void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 270d185737e6..73075ca6e88c 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -109,14 +109,27 @@ static const struct file_operations debugfs_results_fops = { void kunit_debugfs_create_suite(struct kunit_suite *suite) { struct kunit_case *test_case; + struct string_stream *stream; - /* Allocate logs before creating debugfs representation. */ - suite->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(suite->log, true); + /* + * Allocate logs before creating debugfs representation. + * The log pointer must be NULL if there isn't a log so only + * set it if the log stream was created successfully. + */ + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + suite->log = stream; kunit_suite_for_each_test_case(suite, test_case) { - test_case->log = alloc_string_stream(GFP_KERNEL); - string_stream_set_append_newlines(test_case->log, true); + stream = alloc_string_stream(GFP_KERNEL); + if (IS_ERR_OR_NULL(stream)) + goto err; + + string_stream_set_append_newlines(stream, true); + test_case->log = stream; } suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); @@ -124,6 +137,12 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444, suite->debugfs, suite, &debugfs_results_fops); + return; + +err: + string_stream_destroy(suite->log); + kunit_suite_for_each_test_case(suite, test_case) + string_stream_destroy(test_case->log); } void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
In kunit_debugfs_create_suite() give up and skip creating the debugfs file if any of the alloc_string_stream() calls return an error or NULL. Only put a value in the log pointer of kunit_suite and kunit_test if it is a valid pointer to a log. This prevents the potential invalid dereference reported by smatch: lib/kunit/debugfs.c:115 kunit_debugfs_create_suite() error: 'suite->log' dereferencing possible ERR_PTR() lib/kunit/debugfs.c:119 kunit_debugfs_create_suite() error: 'test_case->log' dereferencing possible ERR_PTR() Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Fixes: 05e2006ce493 ("kunit: Use string_stream for test log") --- lib/kunit/debugfs.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)