[PATCH] kunit: debugfs: Handle errors from alloc_string_stream()

[Richard Fitzgerald]

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 <r...@opensource.cirrus.com>
Reported-by: Dan Carpenter <dan.ca...@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;
+
+ 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

[Rae Moar]

On Wed, Sep 27, 2023 at 12:51 PM 'Richard Fitzgerald' via KUnit
Development <kuni...@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 <r...@opensource.cirrus.com>
> Reported-by: Dan Carpenter <dan.ca...@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 <rm...@google.com>

Thanks!
Rae

> --
> 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+...@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.

[Dan Carpenter]

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

[Richard Fitzgerald]

This can be a return. Nothing has been created at this point so
there is nothing to clean up.
I'll send a V2.

[Richard Fitzgerald]

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 <r...@opensource.cirrus.com>
Reported-by: Dan Carpenter <dan.ca...@linaro.org>
Fixes: 05e2006ce493 ("kunit: Use string_stream for test log")

Reviewed-by: Rae Moar <rm...@google.com>
---
Changes from V1:
- If the alloc_string_stream() for the suite->log fails
just return. Nothing has been created at this point so
there's nothing to clean up.
- Re-word the explanation of why the log pointers are only
set if they point to a valid log.

As these changes are trivial I've carried Rae Moar's
Reviewed-by from V1.
---
lib/kunit/debugfs.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
index 270d185737e6..9d167adfa746 100644
--- a/lib/kunit/debugfs.c
+++ b/lib/kunit/debugfs.c
@@ -109,14 +109,28 @@ 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 suite->log and test_case->log pointer are expected to 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))

+ return;

+
+ 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 +138,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