[U-Boot] [PATCH] env: don't add an empty key to the env hashtable

[Lucian Cojocar]

If the environment contains an entry like "=value" "\0" we should skip
this key/value. Otherwise, U-Boot will enter in an infinite loop.

Signed-off-by: Lucian Cojocar <cojocar at gmail.com>
---
lib/hashtable.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/lib/hashtable.c b/lib/hashtable.c
index 07ebfb2..8f5a6f8 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -870,6 +870,10 @@ int himport_r(struct hsearch_data *htab,
*sp++ = '\0'; /* terminate value */
++dp;

+ /* skip this entry if the name is empty */
+ if (*name == 0)
+ continue;
+
/* Skip variables which are not supposed to be processed */
if (!drop_var_from_set(name, nvars, localvars))
continue;

[Wolfgang Denk]

Dear Lucian Cojocar,

Post by Lucian Cojocar
If the environment contains an entry like "=value" "\0" we should skip
this key/value. Otherwise, U-Boot will enter in an infinite loop.
Signed-off-by: Lucian Cojocar <cojocar at gmail.com>
---
lib/hashtable.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 07ebfb2..8f5a6f8 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -870,6 +870,10 @@ int himport_r(struct hsearch_data *htab,
*sp++ = '\0'; /* terminate value */
++dp;
+ /* skip this entry if the name is empty */
+ if (*name == 0)
+ continue;

NAK. This would be a serious error, and silently ignoring this is
downright wrong.

If such a situation should ever happen, it must cause a fatal error.

Could you please explain which exact problem you are trying to fix?
I have to admit that I cannot think of a usage szenario that would
lead to such an error.

Best regards,

Wolfgang Denk

[Lucian Cojocar]

Post by Wolfgang Denk
Dear Lucian Cojocar,

Post by Lucian Cojocar
If the environment contains an entry like "=value" "\0" we should skip
this key/value. Otherwise, U-Boot will enter in an infinite loop.
Signed-off-by: Lucian Cojocar <cojocar at gmail.com>
---
lib/hashtable.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 07ebfb2..8f5a6f8 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -870,6 +870,10 @@ int himport_r(struct hsearch_data *htab,
*sp++ = '\0'; /* terminate value */
++dp;
+ /* skip this entry if the name is empty */
+ if (*name == 0)
+ continue;

NAK. This would be a serious error, and silently ignoring this is
downright wrong.
If such a situation should ever happen, it must cause a fatal error.

I agree.

Post by Wolfgang Denk
Could you please explain which exact problem you are trying to fix?
I have to admit that I cannot think of a usage szenario that would
lead to such an error.

I had an error (typo) in my predefined environment. Basically I had this:

#define CONFIG_EXTRA_ENV_SETTINGS \
"key" "\0" "=value" "\0"

It would be nice if U-Boot told me that there was something wrong with
my environment rather than just hang.

Thanks,
Lucian

[Wolfgang Denk]

Dear Lucian Cojocar,

Post by Lucian Cojocar
#define CONFIG_EXTRA_ENV_SETTINGS \
"key" "\0" "=value" "\0"
It would be nice if U-Boot told me that there was something wrong with
my environment rather than just hang.

Agreed - it should throw an error.

Best regards,

Wolfgang Denk

[Lucian Cojocar]

If the environment contains an entry like "=value" "\0" we should throw
an error when parsing the environment. Otherwise, U-Boot will enter in
an infinite loop.

Signed-off-by: Lucian Cojocar <cojocar at gmail.com>
---
Changes for v2:
- Throw an error instead of silently skip the empty key

lib/hashtable.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/lib/hashtable.c b/lib/hashtable.c
index 07ebfb2..779580b 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -870,6 +870,12 @@ int himport_r(struct hsearch_data *htab,
*sp++ = '\0'; /* terminate value */
++dp;

+ if (*name == 0) {
+ debug("INSERT: unable to use an empty key\n");
+ __set_errno(EINVAL);
+ return 0;
+ }
+
/* Skip variables which are not supposed to be processed */
if (!drop_var_from_set(name, nvars, localvars))
continue;

[Tom Rini]

Post by Lucian Cojocar
If the environment contains an entry like "=value" "\0" we should throw
an error when parsing the environment. Otherwise, U-Boot will enter in
an infinite loop.
Signed-off-by: Lucian Cojocar <cojocar at gmail.com>

Applied to u-boot/master, thanks!