[PATCH v2 1/1] input: avoid NULL dereference

[Heinrich Schuchardt]

On 10/3/23 01:40, Tom Rini wrote:
> On Tue, Oct 03, 2023 at 01:37:14AM +0200, Rasmus Villemoes wrote:
>> On 03/10/2023 00.46, Tom Rini wrote:
>>> On Tue, Oct 03, 2023 at 12:27:25AM +0200, Heinrich Schuchardt wrote:
>>>>   
>>>>   	error = stdio_register(dev);
>>>> -#if !defined(CONFIG_SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)
>>>> -	/* check if this is the standard input device */
>>>> -	if (!error && strcmp(env_get("stdin"), dev->name) == 0) {
>>>> -		/* reassign the console */
>>>> -		if (OVERWRITE_CONSOLE ||
>>>> -				console_assign(stdin, dev->name))
>>>> -			return -1;
>>>> +	if ((IS_ENABLED(SPL_BUILD) || CONFIG_IS_ENABLED(ENV_SUPPORT)) &&
>>>> +	    !error) {
>>>> +		const char *cstdin;
>>>> +
>>>> +		/* check if this is the standard input device */
>>>> +		cstdin = env_get("stdin");
>>>> +		if (cstdin && !strcmp(cstdin, dev->name)) {
>>>> +			/* reassign the console */
>>>> +			if (OVERWRITE_CONSOLE ||
>>>> +			    console_assign(stdin, dev->name))
>>>> +				return -1;
>>>> +		}
>>>>   	}
>>>> -#else
>>>> -	error = error;
>>>> -#endif
>>>>   
>>>>   	return 0;
>>>>   }
>>>
>>> This is an example I think of where #if is more readable.
>>>
>>
>> I kind of agree, but at the same time, this could just be rewritten to
>> avoid that extra indentation. Turn the condition around and make it an
>> early return, then the rest of the function need not be indented.
> 
> Yes, both the indentation and having to use "CONFIG_SPL_BUILD" to start
> with (which is a special case!).

The configuration check looks incorrect:

Function get_env() only exists if CONFIG_$(SPL_TPL)_ENV_SUPPORT=y.
There is no good reason to check for CONFIG_SPL_BUILD.

Why do we have symbols CONFIG_SPL_INPUT and CONFIG_TPL_INPUT if there is 
no board using these?

Best regards

Heinrich

> 
>> I also think the conversion above is buggy (loses a ! on the SPL part),
>> but also, the condition is needlessly convoluted to begin with.
>> !defined(CONFIG_SPL_BUILD) implies ENV_SUPPORT, so the condition is
>> equivalent to just CONFIG_IS_ENABLED(ENV_SUPPORT). So I'd add a
>>
>>    if (!CONFIG_IS_ENABLED(ENV_SUPPORT)
>>      return 0;
> 
> Changes like that would make things overall better, agreed.
>