[RFC PATCH 1/1] cmd: nvedit: Forbid key to be empty.

[RFC PATCH 1/1] cmd: nvedit: Forbid key to be empty.

[Francis Laniel]

Before this patch, it was possible to do the following using setenv:
setenv '' foo
Then, on next reboot, U-Boot will not be able to parse environment due to it
having:
=foo

Now, if the above command is given, an error message is thrown and environment
is not modified.

Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
---
 cmd/nvedit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index d14ba10cef..64b7aef78d 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -262,6 +262,11 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 		return 1;
 	}
 
+	if (!strlen(name)) {
+		printf("## Error: variable name cannot be empty\n");
+		return 1;
+	}
+
 	env_id++;
 
 	/* Delete only ? */
-- 
2.25.1

Re: [RFC PATCH 1/1] cmd: nvedit: Forbid key to be empty.

[Wolfgang Denk]

Dear Francis,

In message <20210628193145.796999-2-francis.laniel@amarulasolutions.com> you wrote:
> Before this patch, it was possible to do the following using setenv:
> setenv '' foo
> Then, on next reboot, U-Boot will not be able to parse environment due to it
> having:
> =foo
>
> Now, if the above command is given, an error message is thrown and environment
> is not modified.
>
> Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
> ---
>  cmd/nvedit.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index d14ba10cef..64b7aef78d 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -262,6 +262,11 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
>  		return 1;
>  	}
>  
> +	if (!strlen(name)) {

Instead of calling a library function here you could use the same
shortcut as in the patch you referenced:

	if (*name == 0) {

> +		printf("## Error: variable name cannot be empty\n");

 s/cannot/must not/  ??

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"The fundamental principle of  science,  the  definition  almost,  is
this: the sole test of the validity of any idea is experiment."
- Richard P. Feynman

Re: [RFC PATCH 1/1] cmd: nvedit: Forbid key to be empty.

[Francis Laniel]

Hi.

On Tue, Jun 29, 2021 at 10:29 AM Wolfgang Denk <wd@denx.de> wrote:

> Dear Francis,
>
> In message <20210628193145.796999-2-francis.laniel@amarulasolutions.com>
> you wrote:
> > Before this patch, it was possible to do the following using setenv:
> > setenv '' foo
> > Then, on next reboot, U-Boot will not be able to parse environment due
> to it
> > having:
> > =foo
> >
> > Now, if the above command is given, an error message is thrown and
> environment
> > is not modified.
> >
> > Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
> > ---
> >  cmd/nvedit.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index d14ba10cef..64b7aef78d 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -262,6 +262,11 @@ static int _do_env_set(int flag, int argc, char
> *const argv[], int env_flag)
> >               return 1;
> >       }
> >
> > +     if (!strlen(name)) {
>
> Instead of calling a library function here you could use the same
> shortcut as in the patch you referenced:
>
>         if (*name == 0) {
>
> > +             printf("## Error: variable name cannot be empty\n");
>
>  s/cannot/must not/  ??
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "The fundamental principle of  science,  the  definition  almost,  is
> this: the sole test of the validity of any idea is experiment."
> - Richard P. Feynman
>

Thank you for the review, I updated my patch and will push soon the v2.


Best regards.

[RFC PATCH 0/1] cmd: nvedit: Forbid environment key to be empty.

[Francis Laniel]

Hi.


First of all, I hope you are fine and the same for your relatives.

In this patch, I modified the setenv command to decline empty variable
name.
Indeed, it was strangely possible to give the following to this command:
setenv '' foo
Which results in the following belonging to environment:
=foo
And which in turns leads to problem while rebooting:
Loading Environment from Flash... Cannot import environment: errno = 22
*** Warning - import failed, using default environment
This error message is due to varname being empty and was added in [1].

With this patch, executing the above command will lead to the following being
printed:
## Error: variable name cannot be empty
And the environment will not be changed.

Here is the diffstat for this patch:
Francis Laniel (1):
  cmd: nvedit: Forbid key to be empty.

 cmd/nvedit.c | 5 +++++
 1 file changed, 5 insertions(+)

If you see any way to improve the patch, feel free to share it.


Best regards.

---
[1] https://u-boot.denx.narkive.com/P4aKxVFu/patch-env-don-t-add-an-empty-key-to-the-env-hashtable

--
2.25.1

[RFC PATCH 1/1] cmd: nvedit: Forbid key to be empty.

[Francis Laniel]

Before this patch, it was possible to do the following using setenv:
setenv '' foo
Then, on next reboot, U-Boot will not be able to parse environment due to it
having:
=foo

Now, if the above command is given, an error message is thrown and environment
is not modified.

Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
---
 cmd/nvedit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index d14ba10cef..64b7aef78d 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -262,6 +262,11 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 		return 1;
 	}
 
+	if (!strlen(name)) {
+		printf("## Error: variable name cannot be empty\n");
+		return 1;
+	}
+
 	env_id++;
 
 	/* Delete only ? */
-- 
2.25.1