* [PATCH 01/10] env: Drop env_get_char_spec() and old, unused .get_char() implementations
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:04 ` [PATCH 02/10] examples: api: glue: Remove comment that does not apply anymore Marek Behún
` (8 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Commit b2cdef4861be ("env: restore old env_get_char() behaviour")
dropped the .get_char() method from struct env_driver, but left the two
existing implementations (eeprom and nvram) in case someone would use
them by overwriting weak function env_get_char_spec().
Since this was never done in the 3.5 years, let's drop these methods and
simplify the code.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
env/eeprom.c | 18 ------------------
env/env.c | 7 +------
env/nvram.c | 14 --------------
3 files changed, 1 insertion(+), 38 deletions(-)
diff --git a/env/eeprom.c b/env/eeprom.c
index 253bdf1428..f8556a4721 100644
--- a/env/eeprom.c
+++ b/env/eeprom.c
@@ -64,24 +64,6 @@ static int eeprom_bus_write(unsigned dev_addr, unsigned offset,
return rcode;
}
-/** Call this function from overridden env_get_char_spec() if you need
- * this functionality.
- */
-int env_eeprom_get_char(int index)
-{
- uchar c;
- unsigned int off = CONFIG_ENV_OFFSET;
-
-#ifdef CONFIG_ENV_OFFSET_REDUND
- if (gd->env_valid == ENV_REDUND)
- off = CONFIG_ENV_OFFSET_REDUND;
-#endif
- eeprom_bus_read(CONFIG_SYS_I2C_EEPROM_ADDR,
- off + index + offsetof(env_t, data), &c, 1);
-
- return c;
-}
-
static int env_eeprom_load(void)
{
char buf_env[CONFIG_ENV_SIZE];
diff --git a/env/env.c b/env/env.c
index e534008006..91d220c3dd 100644
--- a/env/env.c
+++ b/env/env.c
@@ -166,17 +166,12 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
return drv;
}
-__weak int env_get_char_spec(int index)
-{
- return *(uchar *)(gd->env_addr + index);
-}
-
int env_get_char(int index)
{
if (gd->env_valid == ENV_INVALID)
return default_environment[index];
else
- return env_get_char_spec(index);
+ return *(uchar *)(gd->env_addr + index);
}
int env_load(void)
diff --git a/env/nvram.c b/env/nvram.c
index f4126858b5..261b31edfb 100644
--- a/env/nvram.c
+++ b/env/nvram.c
@@ -42,20 +42,6 @@ extern void nvram_write(long dest, const void *src, size_t count);
static env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
#endif
-#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE
-/** Call this function from overridden env_get_char_spec() if you need
- * this functionality.
- */
-int env_nvram_get_char(int index)
-{
- uchar c;
-
- nvram_read(&c, CONFIG_ENV_ADDR + index, 1);
-
- return c;
-}
-#endif
-
static int env_nvram_load(void)
{
char buf[CONFIG_ENV_SIZE];
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 02/10] examples: api: glue: Remove comment that does not apply anymore
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
2021-10-12 11:04 ` [PATCH 01/10] env: Drop env_get_char_spec() and old, unused .get_char() implementations Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:04 ` [PATCH 03/10] env: Change env_match() to static and remove from header Marek Behún
` (7 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This comment is not true since commit 6215bd4c1fd6 ("api: Use hashtable
function for API_env_enum").
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
examples/api/glue.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/examples/api/glue.c b/examples/api/glue.c
index 91d13157a1..075d307ae2 100644
--- a/examples/api/glue.c
+++ b/examples/api/glue.c
@@ -365,11 +365,6 @@ const char * ub_env_enum(const char *last)
env = NULL;
- /*
- * It's OK to pass only the name piece as last (and not the whole
- * 'name=val' string), since the API_ENUM_ENV call uses env_match()
- * internally, which handles such case
- */
if (!syscall(API_ENV_ENUM, NULL, last, &env))
return NULL;
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 03/10] env: Change env_match() to static and remove from header
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
2021-10-12 11:04 ` [PATCH 01/10] env: Drop env_get_char_spec() and old, unused .get_char() implementations Marek Behún
2021-10-12 11:04 ` [PATCH 02/10] examples: api: glue: Remove comment that does not apply anymore Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:04 ` [PATCH 04/10] env: Don't match empty variable name in env_match() Marek Behún
` (6 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This function was used by other parts of U-Boot in the past when
environment was read from underlying device one character at a time.
This is not the case anymore.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 30 +++++++++++++++---------------
include/env.h | 11 -----------
2 files changed, 15 insertions(+), 26 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index ddc715b4f9..742e0924af 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -706,6 +706,21 @@ char *from_env(const char *envvar)
return ret;
}
+static int env_match(uchar *s1, int i2)
+{
+ if (s1 == NULL)
+ return -1;
+
+ while (*s1 == env_get_char(i2++))
+ if (*s1++ == '=')
+ return i2;
+
+ if (*s1 == '\0' && env_get_char(i2-1) == '=')
+ return i2;
+
+ return -1;
+}
+
/*
* Look up variable from environment for restricted C runtime env.
*/
@@ -816,21 +831,6 @@ static int do_env_select(struct cmd_tbl *cmdtp, int flag, int argc,
#endif /* CONFIG_SPL_BUILD */
-int env_match(uchar *s1, int i2)
-{
- if (s1 == NULL)
- return -1;
-
- while (*s1 == env_get_char(i2++))
- if (*s1++ == '=')
- return i2;
-
- if (*s1 == '\0' && env_get_char(i2-1) == '=')
- return i2;
-
- return -1;
-}
-
#ifndef CONFIG_SPL_BUILD
static int do_env_default(struct cmd_tbl *cmdtp, int flag,
int argc, char *const argv[])
diff --git a/include/env.h b/include/env.h
index d5e2bcb530..6b774ae078 100644
--- a/include/env.h
+++ b/include/env.h
@@ -90,17 +90,6 @@ int env_init(void);
*/
void env_relocate(void);
-/**
- * env_match() - Match a name / name=value pair
- *
- * This is used prior to relocation for finding envrionment variables
- *
- * @name: A simple 'name', or a 'name=value' pair.
- * @index: The environment index for a 'name2=value2' pair.
- * @return index for the value if the names match, else -1.
- */
-int env_match(unsigned char *name, int index);
-
/**
* env_get() - Look up the value of an environment variable
*
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 04/10] env: Don't match empty variable name in env_match()
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (2 preceding siblings ...)
2021-10-12 11:04 ` [PATCH 03/10] env: Change env_match() to static and remove from header Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:04 ` [PATCH 05/10] env: Check for terminating null-byte " Marek Behún
` (5 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Do we really allow zero-length variable name? I guess not.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 742e0924af..e2e8a38b5d 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -708,7 +708,7 @@ char *from_env(const char *envvar)
static int env_match(uchar *s1, int i2)
{
- if (s1 == NULL)
+ if (s1 == NULL || *s1 == '\0')
return -1;
while (*s1 == env_get_char(i2++))
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 05/10] env: Check for terminating null-byte in env_match()
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (3 preceding siblings ...)
2021-10-12 11:04 ` [PATCH 04/10] env: Don't match empty variable name in env_match() Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-13 15:35 ` Marek Behún
2021-10-12 11:04 ` [PATCH 06/10] env: Inline env_get_char() into it's only user Marek Behún
` (4 subsequent siblings)
9 siblings, 1 reply; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
There is a possible overflow in env_match(): if environment contains
a terminating null-byte before '=' character (i.e. environment is
broken), the env_match() function can access data after the terminating
null-byte from parameter pointer.
Example: if env_get_char() returns characters from string array
"abc\0def\0" and env_match("abc", 0) is called, the function will access
at least one byte after the end of the "abc" literal.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index e2e8a38b5d..a516491832 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -711,7 +711,7 @@ static int env_match(uchar *s1, int i2)
if (s1 == NULL || *s1 == '\0')
return -1;
- while (*s1 == env_get_char(i2++))
+ while (*s1 != '\0' && *s1 == env_get_char(i2++))
if (*s1++ == '=')
return i2;
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 06/10] env: Inline env_get_char() into it's only user
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (4 preceding siblings ...)
2021-10-12 11:04 ` [PATCH 05/10] env: Check for terminating null-byte " Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:04 ` [PATCH 07/10] env: Early return from env_get_f() on NULL name Marek Behún
` (3 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This function is a relic from the past when environment was read from
underlying device one character at a time.
It is used only in the case when getting an environemnt variable prior
relocation, and the function is simple enough to be inlined there.
Since env_get_char() is being changed to simple access to an array, we
can drop the failing cases and simplify the code (this could have been
done before, since env_get_char() did not fail even before).
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 28 ++++++++++++++--------------
env/env.c | 8 --------
env/nowhere.c | 5 ++---
include/env.h | 10 ----------
4 files changed, 16 insertions(+), 35 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index a516491832..4664b7b872 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -706,16 +706,16 @@ char *from_env(const char *envvar)
return ret;
}
-static int env_match(uchar *s1, int i2)
+static int env_match(const char *env, const char *s1, int i2)
{
if (s1 == NULL || *s1 == '\0')
return -1;
- while (*s1 != '\0' && *s1 == env_get_char(i2++))
+ while (*s1 != '\0' && *s1 == env[i2++])
if (*s1++ == '=')
return i2;
- if (*s1 == '\0' && env_get_char(i2-1) == '=')
+ if (*s1 == '\0' && env[i2-1] == '=')
return i2;
return -1;
@@ -726,28 +726,28 @@ static int env_match(uchar *s1, int i2)
*/
int env_get_f(const char *name, char *buf, unsigned len)
{
- int i, nxt, c;
+ const char *env;
+ int i, nxt;
- for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) {
+ if (gd->env_valid == ENV_INVALID)
+ env = (const char *)default_environment;
+ else
+ env = (const char *)gd->env_addr;
+
+ for (i = 0; env[i] != '\0'; i = nxt + 1) {
int val, n;
- for (nxt = i; (c = env_get_char(nxt)) != '\0'; ++nxt) {
- if (c < 0)
- return c;
+ for (nxt = i; env[nxt] != '\0'; ++nxt)
if (nxt >= CONFIG_ENV_SIZE)
return -1;
- }
- val = env_match((uchar *)name, i);
+ val = env_match(env, name, i);
if (val < 0)
continue;
/* found; copy out */
for (n = 0; n < len; ++n, ++buf) {
- c = env_get_char(val++);
- if (c < 0)
- return c;
- *buf = c;
+ *buf = env[val++];
if (*buf == '\0')
return n;
}
diff --git a/env/env.c b/env/env.c
index 91d220c3dd..e4dfb92e15 100644
--- a/env/env.c
+++ b/env/env.c
@@ -166,14 +166,6 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
return drv;
}
-int env_get_char(int index)
-{
- if (gd->env_valid == ENV_INVALID)
- return default_environment[index];
- else
- return *(uchar *)(gd->env_addr + index);
-}
-
int env_load(void)
{
struct env_driver *drv;
diff --git a/env/nowhere.c b/env/nowhere.c
index 41557f5ce4..1fcf503453 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -31,9 +31,8 @@ static int env_nowhere_init(void)
static int env_nowhere_load(void)
{
/*
- * for SPL, set env_valid = ENV_INVALID is enough as env_get_char()
- * return the default env if env_get is used
- * and SPL don't used env_import to reduce its size
+ * For SPL, setting env_valid = ENV_INVALID is enough, as env_get()
+ * searches default_environment array in that case.
* For U-Boot proper, import the default environment to allow reload.
*/
if (!IS_ENABLED(CONFIG_SPL_BUILD))
diff --git a/include/env.h b/include/env.h
index 6b774ae078..412b2bf21b 100644
--- a/include/env.h
+++ b/include/env.h
@@ -348,16 +348,6 @@ char *env_get_default(const char *name);
/* [re]set to the default environment */
void env_set_default(const char *s, int flags);
-/**
- * env_get_char() - Get a character from the early environment
- *
- * This reads from the pre-relocation environment
- *
- * @index: Index of character to read (0 = first)
- * @return character read, or -ve on error
- */
-int env_get_char(int index);
-
/**
* env_reloc() - Relocate the 'env' sub-commands
*
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 07/10] env: Early return from env_get_f() on NULL name
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (5 preceding siblings ...)
2021-10-12 11:04 ` [PATCH 06/10] env: Inline env_get_char() into it's only user Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:04 ` [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value Marek Behún
` (2 subsequent siblings)
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Test non-NULL name immediately, not in env_match().
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 4664b7b872..412918a000 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -708,9 +708,6 @@ char *from_env(const char *envvar)
static int env_match(const char *env, const char *s1, int i2)
{
- if (s1 == NULL || *s1 == '\0')
- return -1;
-
while (*s1 != '\0' && *s1 == env[i2++])
if (*s1++ == '=')
return i2;
@@ -729,6 +726,9 @@ int env_get_f(const char *name, char *buf, unsigned len)
const char *env;
int i, nxt;
+ if (name == NULL || *name == '\0')
+ return -1;
+
if (gd->env_valid == ENV_INVALID)
env = (const char *)default_environment;
else
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (6 preceding siblings ...)
2021-10-12 11:04 ` [PATCH 07/10] env: Early return from env_get_f() on NULL name Marek Behún
@ 2021-10-12 11:04 ` Marek Behún
2021-10-12 11:41 ` Rasmus Villemoes
2021-10-12 11:05 ` [PATCH 09/10] env: Use string pointer instead of indexes in env_get_f() Marek Behún
2021-10-12 11:05 ` [PATCH 10/10] env: Move non-cmd specific env functions to env/common.c Marek Behún
9 siblings, 1 reply; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:04 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Copy the value of the found variable into given buffer with strncpy().
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 412918a000..51b9e4ffa4 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -746,18 +746,13 @@ int env_get_f(const char *name, char *buf, unsigned len)
continue;
/* found; copy out */
- for (n = 0; n < len; ++n, ++buf) {
- *buf = env[val++];
- if (*buf == '\0')
- return n;
+ n = strncpy(buf, &env[val], len) - buf;
+ if (len && n == len) {
+ buf[len - 1] = '\0';
+ printf("env_buf [%u bytes] too small for value of \"%s\"\n",
+ len, name);
}
- if (n)
- *--buf = '\0';
-
- printf("env_buf [%u bytes] too small for value of \"%s\"\n",
- len, name);
-
return n;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value
2021-10-12 11:04 ` [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value Marek Behún
@ 2021-10-12 11:41 ` Rasmus Villemoes
2021-10-12 12:00 ` Marek Behún
0 siblings, 1 reply; 15+ messages in thread
From: Rasmus Villemoes @ 2021-10-12 11:41 UTC (permalink / raw)
To: Marek Behún, Simon Glass, Tom Rini
Cc: U-Boot Mailing List, Marek Behún
On 12/10/2021 13.04, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
>
> Copy the value of the found variable into given buffer with strncpy().
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 412918a000..51b9e4ffa4 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -746,18 +746,13 @@ int env_get_f(const char *name, char *buf, unsigned len)
> continue;
>
> /* found; copy out */
> - for (n = 0; n < len; ++n, ++buf) {
> - *buf = env[val++];
> - if (*buf == '\0')
> - return n;
> + n = strncpy(buf, &env[val], len) - buf;
strncpy by definition returns the dst argument, so this is, apart from
the side effects done by strncpy, a complicated way of saying "n = 0;".
> + if (len && n == len) {
which then makes this automatically false always.
I understand why you want to avoid an open-coded copying, but strncpy is
almost certainly the wrong tool for the job. Apart from not revealing
anything about the actual length of the source string, it also has the
well-known defect of zeroing the tail of the buffer in the (usual) case
where the source is shorter than the buffer.
n = strlcpy(buf, &env[val], len);
if (n >= len) ...
is probably closer to what you want. But only if you can trust &env[val]
to actually have a nul byte before going into lala land.
Rasmus
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value
2021-10-12 11:41 ` Rasmus Villemoes
@ 2021-10-12 12:00 ` Marek Behún
2021-10-12 12:45 ` Rasmus Villemoes
0 siblings, 1 reply; 15+ messages in thread
From: Marek Behún @ 2021-10-12 12:00 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Simon Glass, Tom Rini, U-Boot Mailing List, Marek Behún
On Tue, 12 Oct 2021 13:41:43 +0200
Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> On 12/10/2021 13.04, Marek Behún wrote:
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > Copy the value of the found variable into given buffer with
> > strncpy().
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> > cmd/nvedit.c | 15 +++++----------
> > 1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 412918a000..51b9e4ffa4 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -746,18 +746,13 @@ int env_get_f(const char *name, char *buf,
> > unsigned len) continue;
> >
> > /* found; copy out */
> > - for (n = 0; n < len; ++n, ++buf) {
> > - *buf = env[val++];
> > - if (*buf == '\0')
> > - return n;
> > + n = strncpy(buf, &env[val], len) - buf;
>
> strncpy by definition returns the dst argument, so this is, apart from
> the side effects done by strncpy, a complicated way of saying "n =
> 0;".
You are right, this is a mistake.
> > + if (len && n == len) {
>
> which then makes this automatically false always.
>
> I understand why you want to avoid an open-coded copying, but strncpy
> is almost certainly the wrong tool for the job. Apart from not
> revealing anything about the actual length of the source string, it
> also has the well-known defect of zeroing the tail of the buffer in
> the (usual) case where the source is shorter than the buffer.
U-Boot's strncpy does not do that:
* Note that unlike userspace strncpy, this does not %NUL-pad the
buffer.
* However, the result is not %NUL-terminated if the source exceeds
* @count bytes.
But you are right that I should use strlcpy here.
Thanks.
> n = strlcpy(buf, &env[val], len);
> if (n >= len) ...
>
> is probably closer to what you want. But only if you can trust
> &env[val] to actually have a nul byte before going into lala land.
>
> Rasmus
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value
2021-10-12 12:00 ` Marek Behún
@ 2021-10-12 12:45 ` Rasmus Villemoes
0 siblings, 0 replies; 15+ messages in thread
From: Rasmus Villemoes @ 2021-10-12 12:45 UTC (permalink / raw)
To: Marek Behún
Cc: Simon Glass, Tom Rini, U-Boot Mailing List, Marek Behún
On 12/10/2021 14.00, Marek Behún wrote:
> On Tue, 12 Oct 2021 13:41:43 +0200
> Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
>
>> On 12/10/2021 13.04, Marek Behún wrote:
>> I understand why you want to avoid an open-coded copying, but strncpy
>> is almost certainly the wrong tool for the job. Apart from not
>> revealing anything about the actual length of the source string, it
>> also has the well-known defect of zeroing the tail of the buffer in
>> the (usual) case where the source is shorter than the buffer.
>
> U-Boot's strncpy does not do that:
> * Note that unlike userspace strncpy, this does not %NUL-pad the
> buffer.
> * However, the result is not %NUL-terminated if the source exceeds
> * @count bytes.
Yeah, I just saw that. That's extremely dangerous, and can cause silent
wrong code generation. The compiler knows the semantics of various
standard functions, including strncpy, and can optimize accordingly. For
example, look at
https://godbolt.org/z/855s7hsEE
In g1, we see that gcc has recognized strncpy() and open-codes it by two
immediate stores (6384738 == 0x616c62 == ascii "bla"), including the
trailing zero padding. Then in g2, gcc further realizes that the
conditional is always false, hence folds the whole thing to "return -1"
(though it does not manage to elide the then-dead stores to buf).
So, I wouldn't bet that in cases where the compiler _does_ end up
emitting a call to strncpy() that it doesn't somehow also rely on the
implementation actually honouring the semantics required by the C standard.
For another example, see
https://godbolt.org/z/W6W3od
The only way it is ok for gcc to compile copy_and_do to the exact same
machine code as copy_and_do2 is because it knows memcpy returns its dst
argument, hence (in the x86-64 case) it can prepare the first argument
to do_stuff via a "mov rdi, rax", instead of spilling and reloading p.
Well, U-Boot seems to pessimize the build with -fno-builtin making much
of the above moot. But:
On pa-risc, until very recently, "prctl(PR_SET_NAME, "x"); ....;
prctl(PR_GET_NAME)" was a simple way to read 14 bytes of kernel stack
from userspace because pa-risc's strncpy didn't do that zeroing. Of
course U-Boot doesn't really have problems with that kind of info-leak,
but given the amount of code U-Boot imports from other projects, not
least linux, there's certainly a fair chance that some of that code
relies on strncpy's semantics for correctness.
Rasmus
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 09/10] env: Use string pointer instead of indexes in env_get_f()
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (7 preceding siblings ...)
2021-10-12 11:04 ` [PATCH 08/10] env: Use strncpy() instead of ad-hoc code to copy variable value Marek Behún
@ 2021-10-12 11:05 ` Marek Behún
2021-10-12 11:05 ` [PATCH 10/10] env: Move non-cmd specific env functions to env/common.c Marek Behún
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:05 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Since we no longer use env_get_char() to access n-th character of
linearized environment data, but rather access the arrays themselves, we
can convert the iteration to use string pointers instead of position
indexes.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 37 ++++++++++++++++++-------------------
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 51b9e4ffa4..455783dccc 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -706,16 +706,16 @@ char *from_env(const char *envvar)
return ret;
}
-static int env_match(const char *env, const char *s1, int i2)
+static const char *matching_name_get_value(const char *p, const char *name)
{
- while (*s1 != '\0' && *s1 == env[i2++])
- if (*s1++ == '=')
- return i2;
+ while (*name != '\0' && *name == *p++)
+ if (*name++ == '=')
+ return p;
- if (*s1 == '\0' && env[i2-1] == '=')
- return i2;
+ if (*name == '\0' && p[-1] == '=')
+ return p;
- return -1;
+ return NULL;
}
/*
@@ -723,8 +723,7 @@ static int env_match(const char *env, const char *s1, int i2)
*/
int env_get_f(const char *name, char *buf, unsigned len)
{
- const char *env;
- int i, nxt;
+ const char *env, *p, *nxt;
if (name == NULL || *name == '\0')
return -1;
@@ -734,26 +733,26 @@ int env_get_f(const char *name, char *buf, unsigned len)
else
env = (const char *)gd->env_addr;
- for (i = 0; env[i] != '\0'; i = nxt + 1) {
- int val, n;
+ for (p = env; *p != '\0'; p = nxt + 1) {
+ const char *value;
+ unsigned copied;
- for (nxt = i; env[nxt] != '\0'; ++nxt)
- if (nxt >= CONFIG_ENV_SIZE)
+ for (nxt = p; *nxt != '\0'; ++nxt)
+ if (p - env >= CONFIG_ENV_SIZE)
return -1;
- val = env_match(env, name, i);
- if (val < 0)
+ value = matching_name_get_value(p, name);
+ if (value == NULL)
continue;
- /* found; copy out */
- n = strncpy(buf, &env[val], len) - buf;
- if (len && n == len) {
+ copied = strncpy(buf, value, len) - buf;
+ if (len && copied == len) {
buf[len - 1] = '\0';
printf("env_buf [%u bytes] too small for value of \"%s\"\n",
len, name);
}
- return n;
+ return copied;
}
return -1;
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 10/10] env: Move non-cmd specific env functions to env/common.c
2021-10-12 11:04 [PATCH 00/10] env_get_char() removal and env_get_f() refactor Marek Behún
` (8 preceding siblings ...)
2021-10-12 11:05 ` [PATCH 09/10] env: Use string pointer instead of indexes in env_get_f() Marek Behún
@ 2021-10-12 11:05 ` Marek Behún
9 siblings, 0 replies; 15+ messages in thread
From: Marek Behún @ 2021-10-12 11:05 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Move the following functions from cmd/nvedit.c to env/common.c:
env_set_ulong()
env_set_hex()
env_get_hex()
eth_env_get_enetaddr()
eth_env_set_enetaddr()
env_get()
from_env()
env_get_f()
env_get_ulong()
since these functions are not specific for U-Boot's CLI.
We leave env_set() in cmd/nvedit.c, since it calls _do_env_set(), which
is a static function in this file.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 182 ---------------------------------------------------
env/common.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 180 insertions(+), 182 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 455783dccc..3bb6e764c0 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -30,7 +30,6 @@
#include <env.h>
#include <env_internal.h>
#include <log.h>
-#include <net.h>
#include <search.h>
#include <errno.h>
#include <malloc.h>
@@ -38,7 +37,6 @@
#include <asm/global_data.h>
#include <linux/bitops.h>
#include <u-boot/crc.h>
-#include <watchdog.h>
#include <linux/stddef.h>
#include <asm/byteorder.h>
#include <asm/io.h>
@@ -320,69 +318,6 @@ int env_set(const char *varname, const char *varvalue)
return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
}
-/**
- * Set an environment variable to an integer value
- *
- * @param varname Environment variable to set
- * @param value Value to set it to
- * @return 0 if ok, 1 on error
- */
-int env_set_ulong(const char *varname, ulong value)
-{
- /* TODO: this should be unsigned */
- char *str = simple_itoa(value);
-
- return env_set(varname, str);
-}
-
-/**
- * Set an environment variable to an value in hex
- *
- * @param varname Environment variable to set
- * @param value Value to set it to
- * @return 0 if ok, 1 on error
- */
-int env_set_hex(const char *varname, ulong value)
-{
- char str[17];
-
- sprintf(str, "%lx", value);
- return env_set(varname, str);
-}
-
-ulong env_get_hex(const char *varname, ulong default_val)
-{
- const char *s;
- ulong value;
- char *endp;
-
- s = env_get(varname);
- if (s)
- value = hextoul(s, &endp);
- if (!s || endp == s)
- return default_val;
-
- return value;
-}
-
-int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
-{
- string_to_enetaddr(env_get(name), enetaddr);
- return is_valid_ethaddr(enetaddr);
-}
-
-int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr)
-{
- char buf[ARP_HLEN_ASCII + 1];
-
- if (eth_env_get_enetaddr(name, (uint8_t *)buf))
- return -EEXIST;
-
- sprintf(buf, "%pM", enetaddr);
-
- return env_set(name, buf);
-}
-
#ifndef CONFIG_SPL_BUILD
static int do_env_set(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
@@ -661,124 +596,7 @@ static int do_env_edit(struct cmd_tbl *cmdtp, int flag, int argc,
}
}
#endif /* CONFIG_CMD_EDITENV */
-#endif /* CONFIG_SPL_BUILD */
-
-/*
- * Look up variable from environment,
- * return address of storage for that variable,
- * or NULL if not found
- */
-char *env_get(const char *name)
-{
- if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
- struct env_entry e, *ep;
-
- WATCHDOG_RESET();
-
- e.key = name;
- e.data = NULL;
- hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
-
- return ep ? ep->data : NULL;
- }
-
- /* restricted capabilities before import */
- if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
- return (char *)(gd->env_buf);
-
- return NULL;
-}
-/*
- * Like env_get, but prints an error if envvar isn't defined in the
- * environment. It always returns what env_get does, so it can be used in
- * place of env_get without changing error handling otherwise.
- */
-char *from_env(const char *envvar)
-{
- char *ret;
-
- ret = env_get(envvar);
-
- if (!ret)
- printf("missing environment variable: %s\n", envvar);
-
- return ret;
-}
-
-static const char *matching_name_get_value(const char *p, const char *name)
-{
- while (*name != '\0' && *name == *p++)
- if (*name++ == '=')
- return p;
-
- if (*name == '\0' && p[-1] == '=')
- return p;
-
- return NULL;
-}
-
-/*
- * Look up variable from environment for restricted C runtime env.
- */
-int env_get_f(const char *name, char *buf, unsigned len)
-{
- const char *env, *p, *nxt;
-
- if (name == NULL || *name == '\0')
- return -1;
-
- if (gd->env_valid == ENV_INVALID)
- env = (const char *)default_environment;
- else
- env = (const char *)gd->env_addr;
-
- for (p = env; *p != '\0'; p = nxt + 1) {
- const char *value;
- unsigned copied;
-
- for (nxt = p; *nxt != '\0'; ++nxt)
- if (p - env >= CONFIG_ENV_SIZE)
- return -1;
-
- value = matching_name_get_value(p, name);
- if (value == NULL)
- continue;
-
- copied = strncpy(buf, value, len) - buf;
- if (len && copied == len) {
- buf[len - 1] = '\0';
- printf("env_buf [%u bytes] too small for value of \"%s\"\n",
- len, name);
- }
-
- return copied;
- }
-
- return -1;
-}
-
-/**
- * Decode the integer value of an environment variable and return it.
- *
- * @param name Name of environment variable
- * @param base Number base to use (normally 10, or 16 for hex)
- * @param default_val Default value to return if the variable is not
- * found
- * @return the decoded value, or default_val if not found
- */
-ulong env_get_ulong(const char *name, int base, ulong default_val)
-{
- /*
- * We can use env_get() here, even before relocation, since the
- * environment variable value is an integer and thus short.
- */
- const char *str = env_get(name);
-
- return str ? simple_strtoul(str, NULL, base) : default_val;
-}
-
-#ifndef CONFIG_SPL_BUILD
#if defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE)
static int do_env_save(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
diff --git a/env/common.c b/env/common.c
index 81e9e0b2aa..b1b2989bc9 100644
--- a/env/common.c
+++ b/env/common.c
@@ -21,6 +21,8 @@
#include <malloc.h>
#include <u-boot/crc.h>
#include <dm/ofnode.h>
+#include <net.h>
+#include <watchdog.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -33,6 +35,184 @@ struct hsearch_data env_htab = {
.change_ok = env_flags_validate,
};
+/**
+ * Set an environment variable to an integer value
+ *
+ * @param varname Environment variable to set
+ * @param value Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int env_set_ulong(const char *varname, ulong value)
+{
+ /* TODO: this should be unsigned */
+ char *str = simple_itoa(value);
+
+ return env_set(varname, str);
+}
+
+/**
+ * Set an environment variable to an value in hex
+ *
+ * @param varname Environment variable to set
+ * @param value Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int env_set_hex(const char *varname, ulong value)
+{
+ char str[17];
+
+ sprintf(str, "%lx", value);
+ return env_set(varname, str);
+}
+
+ulong env_get_hex(const char *varname, ulong default_val)
+{
+ const char *s;
+ ulong value;
+ char *endp;
+
+ s = env_get(varname);
+ if (s)
+ value = hextoul(s, &endp);
+ if (!s || endp == s)
+ return default_val;
+
+ return value;
+}
+
+int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
+{
+ string_to_enetaddr(env_get(name), enetaddr);
+ return is_valid_ethaddr(enetaddr);
+}
+
+int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr)
+{
+ char buf[ARP_HLEN_ASCII + 1];
+
+ if (eth_env_get_enetaddr(name, (uint8_t *)buf))
+ return -EEXIST;
+
+ sprintf(buf, "%pM", enetaddr);
+
+ return env_set(name, buf);
+}
+
+/*
+ * Look up variable from environment,
+ * return address of storage for that variable,
+ * or NULL if not found
+ */
+char *env_get(const char *name)
+{
+ if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
+ struct env_entry e, *ep;
+
+ WATCHDOG_RESET();
+
+ e.key = name;
+ e.data = NULL;
+ hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
+
+ return ep ? ep->data : NULL;
+ }
+
+ /* restricted capabilities before import */
+ if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
+ return (char *)(gd->env_buf);
+
+ return NULL;
+}
+
+/*
+ * Like env_get, but prints an error if envvar isn't defined in the
+ * environment. It always returns what env_get does, so it can be used in
+ * place of env_get without changing error handling otherwise.
+ */
+char *from_env(const char *envvar)
+{
+ char *ret;
+
+ ret = env_get(envvar);
+
+ if (!ret)
+ printf("missing environment variable: %s\n", envvar);
+
+ return ret;
+}
+
+static const char *matching_name_get_value(const char *p, const char *name)
+{
+ while (*name != '\0' && *name == *p++)
+ if (*name++ == '=')
+ return p;
+
+ if (*name == '\0' && p[-1] == '=')
+ return p;
+
+ return NULL;
+}
+
+/*
+ * Look up variable from environment for restricted C runtime env.
+ */
+int env_get_f(const char *name, char *buf, unsigned len)
+{
+ const char *env, *p, *nxt;
+
+ if (name == NULL || *name == '\0')
+ return -1;
+
+ if (gd->env_valid == ENV_INVALID)
+ env = (const char *)default_environment;
+ else
+ env = (const char *)gd->env_addr;
+
+ for (p = env; *p != '\0'; p = nxt + 1) {
+ const char *value;
+ unsigned copied;
+
+ for (nxt = p; *nxt != '\0'; ++nxt)
+ if (p - env >= CONFIG_ENV_SIZE)
+ return -1;
+
+ value = matching_name_get_value(p, name);
+ if (value == NULL)
+ continue;
+
+ copied = strncpy(buf, value, len) - buf;
+ if (len && copied == len) {
+ buf[len - 1] = '\0';
+ printf("env_buf [%u bytes] too small for value of \"%s\"\n",
+ len, name);
+ }
+
+ return copied;
+ }
+
+ return -1;
+}
+
+/**
+ * Decode the integer value of an environment variable and return it.
+ *
+ * @param name Name of environment variable
+ * @param base Number base to use (normally 10, or 16 for hex)
+ * @param default_val Default value to return if the variable is not
+ * found
+ * @return the decoded value, or default_val if not found
+ */
+ulong env_get_ulong(const char *name, int base, ulong default_val)
+{
+ /*
+ * We can use env_get() here, even before relocation, since the
+ * environment variable value is an integer and thus short.
+ */
+ const char *str = env_get(name);
+
+ return str ? simple_strtoul(str, NULL, base) : default_val;
+}
+
/*
* Read an environment variable as a boolean
* Return -1 if variable does not exist (default to true)
--
2.32.0
^ permalink raw reply related [flat|nested] 15+ messages in thread