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

[Dan Carpenter]

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@xxxxxxxxxxxxxxxxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 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