[PATCH v2 1/1] input: avoid NULL dereference
Before using the result of env_get("stdin") we must check if it is NULL.
Avoid #if. This resolves the -Wunused-but-set-variable issue and we don't need a dummy assignment in the else branch. Anyway this warning is disabled in the Makefile.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: Avoid #if. --- drivers/input/input.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c index a4341e8c7c..de62189782 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -669,17 +669,19 @@ int input_stdio_register(struct stdio_dev *dev) int error;
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; }
On Tue, Oct 03, 2023 at 12:27:25AM +0200, Heinrich Schuchardt wrote:
Before using the result of env_get("stdin") we must check if it is NULL.
Avoid #if. This resolves the -Wunused-but-set-variable issue and we don't need a dummy assignment in the else branch. Anyway this warning is disabled in the Makefile.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: Avoid #if.
drivers/input/input.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c index a4341e8c7c..de62189782 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -669,17 +669,19 @@ int input_stdio_register(struct stdio_dev *dev) int error;
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.
On 10/3/23 00:46, Tom Rini wrote:
On Tue, Oct 03, 2023 at 12:27:25AM +0200, Heinrich Schuchardt wrote:
Before using the result of env_get("stdin") we must check if it is NULL.
Avoid #if. This resolves the -Wunused-but-set-variable issue and we don't need a dummy assignment in the else branch. Anyway this warning is disabled in the Makefile.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: Avoid #if.
drivers/input/input.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c index a4341e8c7c..de62189782 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -669,17 +669,19 @@ int input_stdio_register(struct stdio_dev *dev) int error;
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.
See v1 review. Simon asked to replace #if.
I don't mind which version you merge. Just mark the other one as superseded.
Best regards
Heinrich
Hi Tom,
On Mon, 2 Oct 2023 at 16:46, Tom Rini trini@konsulko.com wrote:
On Tue, Oct 03, 2023 at 12:27:25AM +0200, Heinrich Schuchardt wrote:
Before using the result of env_get("stdin") we must check if it is NULL.
Avoid #if. This resolves the -Wunused-but-set-variable issue and we don't need a dummy assignment in the else branch. Anyway this warning is disabled in the Makefile.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: Avoid #if.
drivers/input/input.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c index a4341e8c7c..de62189782 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -669,17 +669,19 @@ int input_stdio_register(struct stdio_dev *dev) int error;
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 have seen you make this comment a bit, lately. I would have thought that dropping a build path would be a win...is it the extra indentation you don't like?
Regards, Simon
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.
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;
Rasmus
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!).
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.
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.
On Mon, 2 Oct 2023 at 16:27, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:
Before using the result of env_get("stdin") we must check if it is NULL.
Avoid #if. This resolves the -Wunused-but-set-variable issue and we don't need a dummy assignment in the else branch. Anyway this warning is disabled in the Makefile.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: Avoid #if.
drivers/input/input.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
participants (4)
-
Heinrich Schuchardt -
Rasmus Villemoes -
Simon Glass -
Tom Rini