[PATCH 0/3] cmd: setexpr: add fmt format string operation

[PATCH 0/3] cmd: setexpr: add fmt format string operation

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>


This patch series is the revised version of the previos proposal
[PATCH v1 0/1] cmd: setexpr: add dec operation for converting variable
to decimal

In contrast to version 1, the "dec" operator has been replaced by
a more flexible "fmt" operator to handle format strings.

U-Boot uses almost everywhere hexadecimal numbers. But some bootargs
passed to Linux are expecting decimal numbers. As long as the values
are in the range 0 to 9 it is sufficient to just strip 0x from the
number. But for greater values a method for converting numbers to
decimal is needed.

This patch adds C like format string capabilities to the setexpr
command. Here are some examples:

  => setexpr foo fmt %d 0x100
  => echo $foo
  256
  =>

  => setexpr foo fmt 0x%08x 63
  => echo $foo
  0x00000063
  =>

  => setexpr foo fmt %%%o 8
  => echo $foo
  %10
  =>

In contrast to the original C format strings the number of parameters
is limited to one. As the get_arg() function returns unsigned long
type, the format string commands are limited to those which are
operating on unsigned long type.

Patman insisted adding a maintainer to for the doc/usage/setexpr.rst,
but I am not sure if chose the correct section and person.



Roland Gaudig (3):
  cmd: setexpr: add fmt format string operation
  doc: usage: add description for setexpr command
  test: cmd: setexpr: add tests for format string operations

 MAINTAINERS           |   6 ++
 cmd/setexpr.c         | 102 ++++++++++++++++++++++++++++++++-
 doc/usage/index.rst   |   1 +
 doc/usage/setexpr.rst | 130 ++++++++++++++++++++++++++++++++++++++++++
 test/cmd/setexpr.c    |  33 +++++++++++
 5 files changed, 271 insertions(+), 1 deletion(-)
 create mode 100644 doc/usage/setexpr.rst

-- 
2.25.1

[PATCH 1/3] cmd: setexpr: add fmt format string operation

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Add format string "fmt" operation to the setexpr command
which converts the input value according the format string
specification and stores the result into the variable named name.

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

 cmd/setexpr.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index e828be3970..b69cbab3dd 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -11,12 +11,15 @@
 #include <common.h>
 #include <config.h>
 #include <command.h>
+#include <ctype.h>
 #include <env.h>
 #include <log.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <linux/sizes.h>
 
+#define MAX_STR_LEN 128
+
 /**
  * struct expr_arg: Holds an argument to an expression
  *
@@ -361,6 +364,95 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
 }
 #endif
 
+/**
+ * setexpr_fmt_spec_search() - Search for format specifier
+ *
+ * This function searches the intput string for the first occurrence of a
+ * format specifier which starts with a %. Double % are ignored.
+ *
+ * @format: C like format string to search
+ * @return: Pointer to found format specifier, NULL in case none is found
+ */
+static char *setexpr_fmt_spec_search(char *format)
+{
+	while (*format != '\0') {
+		if (*format == '%') {
+			switch (*(format + 1)) {
+			case '%':
+				/* found '%%', not a format specifier, skip. */
+				format++;
+				break;
+			case '\0':
+				/* found '%' at end of string,
+				 * incomplete format specifier.
+				 */
+				return NULL;
+			default:
+				/* looks like a format specifier */
+				return format;
+			}
+		}
+		format++;
+	}
+
+	return NULL;
+}
+
+/**
+ * setexpr_fmt() - Implements the setexpr <name> fmt command
+ *
+ * This function implements the setexpr <name> fmt <format> <value> command.
+ *
+ * @name: Name of the environment variable to save the evaluated expression in
+ * @format: C like format string
+ * @value: Input value to be converted
+ * @return: 0 if OK, 1 on error
+ */
+static int setexpr_fmt(char *name, char *format, char *value)
+{
+	struct expr_arg aval;
+	int data_size;
+	char str[MAX_STR_LEN];
+	int fmt_len = strlen(format);
+
+	if (fmt_len < 2) {
+		printf("Error: missing format string");
+		return CMD_RET_FAILURE;
+	}
+
+	/* Search format specifier */
+	char *first = setexpr_fmt_spec_search(format);
+
+	/* Exactly one format specifier is required */
+	if (!first || setexpr_fmt_spec_search(first + 1)) {
+		printf("Error: exactly one format specifier is required\n");
+		return CMD_RET_FAILURE;
+	}
+
+	/* Search type field of format specifier */
+	while (*first && !isalpha(*first))
+		first++;
+
+	/* Test if type is supported */
+	if (!strchr("diouxX", *first)) {
+		printf("Error: format type not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	data_size = cmd_get_data_size(value, 4);
+
+	if (data_size == CMD_DATA_SIZE_STR) {
+		free(aval.sval);
+		return CMD_RET_FAILURE;
+	}
+
+	if (get_arg(value, data_size, &aval))
+		return CMD_RET_FAILURE;
+
+	snprintf(str, sizeof(str), format, aval.ival);
+	return env_set(name, str);
+}
+
 static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[])
 {
@@ -374,6 +466,7 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	 * 3 : setexpr name value
 	 * 5 : setexpr name val1 op val2
 	 *     setexpr name [g]sub r s
+	 *     setexpr name fmt format value
 	 * 6 : setexpr name [g]sub r s t
 	 */
 
@@ -398,6 +491,10 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 		return ret;
 	}
 
+	/* format string assignment: "setexpr name fmt %d value" */
+	if (strcmp(argv[2], "fmt") == 0)
+		return setexpr_fmt(argv[1], argv[3], argv[4]);
+
 	/* 5 or 6 args (6 args only with [g]sub) */
 #ifdef CONFIG_REGEX
 	/*
@@ -504,7 +601,10 @@ U_BOOT_CMD(
 	"      size argument is only meaningful if value1 and/or value2 are\n"
 	"      memory addresses (*)\n"
 	"setexpr[.b, .w, .l] name [*]value\n"
-	"    - load a value into a variable"
+	"    - load a value into a variable\n"
+	"setexpr name fmt <format> [*]value\n"
+	"    - set environment variable 'name' to the result of the C like\n"
+	"      format string evaluation of [*]value.\n"
 #ifdef CONFIG_REGEX
 	"\n"
 	"setexpr name gsub r s [t]\n"
-- 
2.25.1

Re: [PATCH 1/3] cmd: setexpr: add fmt format string operation

[Rasmus Villemoes]

On 28/06/2021 17.17, Roland Gaudig wrote:
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
> 
> Add format string "fmt" operation to the setexpr command
> which converts the input value according the format string
> specification and stores the result into the variable named name.
> 
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
> 
>  cmd/setexpr.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
> index e828be3970..b69cbab3dd 100644
> --- a/cmd/setexpr.c
> +++ b/cmd/setexpr.c
> @@ -11,12 +11,15 @@
>  #include <common.h>
>  #include <config.h>
>  #include <command.h>
> +#include <ctype.h>
>  #include <env.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <linux/sizes.h>
>  
> +#define MAX_STR_LEN 128
> +
>  /**
>   * struct expr_arg: Holds an argument to an expression
>   *
> @@ -361,6 +364,95 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
>  }
>  #endif
>  
> +/**
> + * setexpr_fmt_spec_search() - Search for format specifier
> + *
> + * This function searches the intput string for the first occurrence of a
> + * format specifier which starts with a %. Double % are ignored.
> + *
> + * @format: C like format string to search
> + * @return: Pointer to found format specifier, NULL in case none is found
> + */
> +static char *setexpr_fmt_spec_search(char *format)
> +{
> +	while (*format != '\0') {
> +		if (*format == '%') {
> +			switch (*(format + 1)) {
> +			case '%':
> +				/* found '%%', not a format specifier, skip. */
> +				format++;
> +				break;
> +			case '\0':
> +				/* found '%' at end of string,
> +				 * incomplete format specifier.
> +				 */
> +				return NULL;
> +			default:
> +				/* looks like a format specifier */
> +				return format;
> +			}
> +		}
> +		format++;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * setexpr_fmt() - Implements the setexpr <name> fmt command
> + *
> + * This function implements the setexpr <name> fmt <format> <value> command.
> + *
> + * @name: Name of the environment variable to save the evaluated expression in
> + * @format: C like format string
> + * @value: Input value to be converted
> + * @return: 0 if OK, 1 on error
> + */
> +static int setexpr_fmt(char *name, char *format, char *value)
> +{
> +	struct expr_arg aval;
> +	int data_size;
> +	char str[MAX_STR_LEN];
> +	int fmt_len = strlen(format);
> +
> +	if (fmt_len < 2) {
> +		printf("Error: missing format string");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	/* Search format specifier */
> +	char *first = setexpr_fmt_spec_search(format);
> +
> +	/* Exactly one format specifier is required */
> +	if (!first || setexpr_fmt_spec_search(first + 1)) {
> +		printf("Error: exactly one format specifier is required\n");
> +		return CMD_RET_FAILURE;
> +	}

I think this allows a lone % to appear at the end of the string after
the actual %d, i.e. it seems "%d bla %" would be accepted.

> +
> +	/* Search type field of format specifier */
> +	while (*first && !isalpha(*first))
> +		first++;

This will allow stuff like %*d or %30.*x both of which are format
strings expecting two integer arguments, the first specifying field
width or precision, respectively.


> +	/* Test if type is supported */
> +	if (!strchr("diouxX", *first)) {
> +		printf("Error: format type not supported\n");
> +		return CMD_RET_FAILURE;
> +	}

So this insists that the format specifier is one that expects a
corresponding "int" or "unsigned int" argument (no l or ll modifier) [*]

For something like "%123", the loop above would have made first point at
the nul at the end of the string, and strchr(whatever, '\0') is required
to consider the nul at the end of whatever part of the string, i.e. that
is equivalent to whatever+strlen(whatever), so that format string would
falsely be accepted. So you either need to explicitly test *first for
being non-zero, or write it in terms of switch(*first).

> +	data_size = cmd_get_data_size(value, 4);
> +
> +	if (data_size == CMD_DATA_SIZE_STR) {
> +		free(aval.sval);

Is aval or aval.sval even initialized at this point? I'd expect the
compiler to complain about this.

> +		return CMD_RET_FAILURE;
> +	}
> +
> +	if (get_arg(value, data_size, &aval))
> +		return CMD_RET_FAILURE;
> +
> +	snprintf(str, sizeof(str), format, aval.ival);

[*] but you pass an "unsigned long".

Also, I think you should check for overflow and make it fail instead of
silently assigning a truncated string to the target.

I think allowing "arbitrary" format strings, restricted to those
expected exactly one integer argument, is too fragile and error-prone.
If somebody wants the result prepended by "100% max ", it's just as easy
to do

env set foo "100% max $foo"

right after the setexpr.

It's not that the above are impossible to fix; you can insist on the
specifier ending in "l[diouxX]" (i.e., require the l modifier), but that
makes it somewhat less ergonomic. Casting aval.ival to (int) or
(unsigned int) should not be an option; on 64 bit machines that would
make a lot of values impossible to handle.

Rasmus

Re: [PATCH 1/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk]

Dear Rasmus,

In message <4c69688f-c187-ad14-5cfe-cdcddc71f5c8@prevas.dk> you wrote:
>
> I think this allows a lone % to appear at the end of the string after
> the actual %d, i.e. it seems "%d bla %" would be accepted.

there are a lot of highly unintuitive restrictions in that code,
indeed.

> I think allowing "arbitrary" format strings, restricted to those
> expected exactly one integer argument, is too fragile and error-prone.

Indeed.  See for example the "printf" (bash) shell builtin - we
should have a similar flexibility here.

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
Humanity has the  stars  in  its  future,  and  that  future  is  too
important  to be lost under the burden of juvenile folly and ignorant
superstition.                                          - Isaac Asimov

Re: [PATCH 1/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk]

Dear Roland,

In message <20210628151750.572837-2-roland.gaudig-oss@weidmueller.com> you wrote:
>
> +		if (*format == '%') {
> +			switch (*(format + 1)) {
> +			case '%':
> +				/* found '%%', not a format specifier, skip. */
> +				format++;
> +				break;
> +			case '\0':
> +				/* found '%' at end of string,
> +				 * incomplete format specifier.
> +				 */
> +				return NULL;
> +			default:
> +				/* looks like a format specifier */
> +				return format;
> +			}
> +		}

Why do you do that here?  *printf() should be clever enough to parst
the format string.

> +static int setexpr_fmt(char *name, char *format, char *value)
> +{
> +	struct expr_arg aval;
> +	int data_size;
> +	char str[MAX_STR_LEN];
> +	int fmt_len = strlen(format);
> +
> +	if (fmt_len < 2) {
> +		printf("Error: missing format string");
> +		return CMD_RET_FAILURE;
> +	}

This is an arbitrary restriction that just limits the potential use.
Please don't do this.  Maybe I want to use:

	=> setexpr foo fmt X

This should not cause problems.

> +	/* Exactly one format specifier is required */
> +	if (!first || setexpr_fmt_spec_search(first + 1)) {
> +		printf("Error: exactly one format specifier is required\n");
> +		return CMD_RET_FAILURE;
> +	}

Please get rid of this restriction.

> +	/* Search type field of format specifier */
> +	while (*first && !isalpha(*first))
> +		first++;
> +
> +	/* Test if type is supported */
> +	if (!strchr("diouxX", *first)) {
> +		printf("Error: format type not supported\n");
> +		return CMD_RET_FAILURE;
> +	}

Get rid of all these, please!


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
Man did not weave the web of life; he  is  merely  a  strand  in  it.
Whatever he does to the web, he does to himself.     - Seattle [1854]

[PATCH 2/3] doc: usage: add description for setexpr command

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Add usage for the setexpr command. It has been added to describe
mainly the new setexpr format string operation.

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

 MAINTAINERS           |   6 ++
 doc/usage/index.rst   |   1 +
 doc/usage/setexpr.rst | 130 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 doc/usage/setexpr.rst

diff --git a/MAINTAINERS b/MAINTAINERS
index 11e11d51a7..15df95567f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1048,6 +1048,12 @@ F:	arch/sandbox/
 F:	doc/arch/sandbox.rst
 F:	include/dt-bindings/*/sandbox*.h
 
+SETEXPR
+M:	Simon Glass <sjg@chromium.org>
+S:	Maintained
+F:	cmd/setexpr.c
+F:	doc/usage/setexpr.rst
+
 SH
 M:	Marek Vasut <marek.vasut+renesas@gmail.com>
 M:	Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 843b4371f1..6e94ab0313 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -42,6 +42,7 @@ Shell commands
    reset
    sbi
    scp03
+   setexpr
    size
    true
    ums
diff --git a/doc/usage/setexpr.rst b/doc/usage/setexpr.rst
new file mode 100644
index 0000000000..ed2fd284fa
--- /dev/null
+++ b/doc/usage/setexpr.rst
@@ -0,0 +1,130 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+setexpr command
+===============
+
+Synopsis
+--------
+
+::
+
+    setexpr[.b, .w, .l .s] <name> [*]<value> <op> [*]<value2>
+    setexpr[.b, .w, .l] <name> [*]<value>
+    setexpr <name> fmt <format> [*]<value>
+    setexpr <name> gsub r s [t]
+    setexpr <name> sub r s [t]
+
+Description
+-----------
+
+The setexpr command is used to set an environment variable to the result
+of an evaluation.
+
+setexpr[.b, .w, .l .s] <name> [*]<value> <op> [*]<value2>
+     Set environment variable <name> to the result of the evaluated
+     expression specified by <op>.
+
+setexpr[.b, .w, .l] name [*]value
+     Load <value> into environment variable <name>
+
+setexpr name fmt <format> [*]value
+     Set environment variable <name> to the result of the C like
+     format string <format> evaluation of [*]<value>.
+
+setexpr name gsub <r> <s> [<t>]
+     For each substring matching the regular expression <r> in the
+     string <t>, substitute the string <s>.
+     The result is assigned to <name>.
+     If <t> is not supplied, use the old value of <name>.
+
+setexpr name sub <r> <s> [<t>]
+     Just like gsub(), but replace only the first matching substring
+
+The setexpr command takes the following arguments:
+
+format
+    This parameter contains a C like format string. In contrast to the C
+    language some simplificatinons were made. Only one argument is supported.
+    Only the following format types are supported:
+
+    d, i
+        decimal value
+    o
+        octal value
+    x, X
+        hexadecimal value
+    u
+        unsigned decimal value
+    '%'
+        no conversion, instead a % character will be written
+
+name
+    The name of the environment variable to be set
+
+op
+    '|'
+        name = value | value2
+    '&'
+        name = value & value2
+    '+'
+        name = value + value2
+        (This is the only operator supported for strings.
+	It acts as concatenation operator on strings)
+    '^'
+        name = value ^ value2
+    '-'
+        name = value - value2
+    '*'
+        name = value * value2
+    '/'
+        name = value / value2
+    '%'
+        name = value % value2
+
+r
+    Regular expression
+
+s
+    Substitution string
+
+t
+    string
+
+value
+    Can either be an integer value, a string.
+    If the pointer prefix '*' is given value is treated as memory address.
+
+value2
+    See value
+
+Example
+-------
+
+::
+
+    => setexpr foo fmt %d 0x100
+    => echo $foo
+    256
+    =>
+
+    => setexpr foo fmt 0x%08x 63
+    => echo $foo
+    0x00000063
+    =>
+
+    => setexpr foo fmt %%%o 8
+    => echo $foo
+    %10
+    =>
+
+Configuration
+-------------
+
+The setexpr gsub and sub operations are only available if CONFIG_REGEX=y.
+
+Return value
+------------
+
+The return value $? is set to 0 (true) if the operation was successful.
+
+If an error occurs, the return value $? is set to 1 (false).
-- 
2.25.1

Re: [PATCH 2/3] doc: usage: add description for setexpr command

[Simon Glass]

On Mon, 28 Jun 2021 at 09:18, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Add usage for the setexpr command. It has been added to describe
> mainly the new setexpr format string operation.
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
>  MAINTAINERS           |   6 ++
>  doc/usage/index.rst   |   1 +
>  doc/usage/setexpr.rst | 130 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 137 insertions(+)
>  create mode 100644 doc/usage/setexpr.rst

Nice!

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/3] test: cmd: setexpr: add tests for format string operations

[Roland Gaudig]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

Series-version 2

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

 test/cmd/setexpr.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index c537e89353..f8b5eef62e 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -388,6 +388,39 @@ static int setexpr_test_str_long(struct unit_test_state *uts)
 }
 SETEXPR_TEST(setexpr_test_str_long, UT_TESTF_CONSOLE_REC);
 
+/* Test 'setexpr' command with simply setting integers */
+static int setexpr_test_fmt(struct unit_test_state *uts)
+{
+		u8 *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+
+	/* Test decimal conversion */
+	ut_assertok(run_command("setexpr fred fmt %d 0xff", 0));
+	ut_asserteq_str("255", env_get("fred"));
+	/* Test hexadecimal conversion with 0x prefix and 4 digits */
+	ut_assertok(run_command("setexpr fred fmt 0x%04x 257", 0));
+	ut_asserteq_str("0x0257", env_get("fred"));
+	/* Test octal conversion with % prefix */
+	ut_assertok(run_command("setexpr fred fmt %%%o 8", 0));
+	ut_asserteq_str("%10", env_get("fred"));
+	/* Error test with missing format specifier */
+	ut_asserteq(1, run_command("setexpr fred fmd hello 0xff", 0));
+	/* Error test with invalid format type */
+	ut_asserteq(1, run_command("setexpr fred fmt %a 0xff", 0));
+	/* Error test with incomplete format specifier */
+	ut_asserteq(1, run_command("setexpr fred fmt hello% bf", 0));
+	/* Error test with more than one format specifier */
+	ut_asserteq(1, run_command("setexpr fred fmt %d_%x 100", 0));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+
+SETEXPR_TEST(setexpr_test_fmt, UT_TESTF_CONSOLE_REC);
+
 int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct unit_test *tests = UNIT_TEST_SUITE_START(setexpr_test);
-- 
2.25.1

Re: [PATCH 3/3] test: cmd: setexpr: add tests for format string operations

[Simon Glass]

On Mon, 28 Jun 2021 at 09:18, Roland Gaudig
<roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> Series-version 2

missing :

>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
>  test/cmd/setexpr.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

(please resend as v3 to fix the above)

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk]

Dear Roland,

In message <20210628151750.572837-1-roland.gaudig-oss@weidmueller.com> you wrote:
>
>
> U-Boot uses almost everywhere hexadecimal numbers. But some bootargs
> passed to Linux are expecting decimal numbers. As long as the values
> are in the range 0 to 9 it is sufficient to just strip 0x from the
> number. But for greater values a method for converting numbers to
> decimal is needed.
>
> This patch adds C like format string capabilities to the setexpr
> command. Here are some examples:

Thanks!

> In contrast to the original C format strings the number of parameters
> is limited to one. As the get_arg() function returns unsigned long
> type, the format string commands are limited to those which are
> operating on unsigned long type.

These are two pretty unfortunate restrictions.  I guess it should
not be too hard to avoid both of these.  Can you please give it a
try?

I think it is reasonable to assume (and specify) that, when the
"fmt" option is used, _all_ following arguments will be passed
(unchanged) to the sprintf() function.

This was actually one of my intentions when making this suggestion -
to be able to construct any kind of data from pieces; say, for
example:

=> setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d

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
Never ascribe to malice that which can  adequately  be  explained  by
stupidity.

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Roland Gaudig (OSS)]

Hello Wolfgang

On 29.06.21 08:37, Wolfgang Denk wrote:
>
> In message <20210628151750.572837-1-roland.gaudig-oss@weidmueller.com> you wrote:
>>
>>
>> U-Boot uses almost everywhere hexadecimal numbers. But some bootargs
>> passed to Linux are expecting decimal numbers. As long as the values
>> are in the range 0 to 9 it is sufficient to just strip 0x from the
>> number. But for greater values a method for converting numbers to
>> decimal is needed.
>>
>> This patch adds C like format string capabilities to the setexpr
>> command. Here are some examples:
> 
> Thanks!
> 
>> In contrast to the original C format strings the number of parameters
>> is limited to one. As the get_arg() function returns unsigned long
>> type, the format string commands are limited to those which are
>> operating on unsigned long type.
> 
> These are two pretty unfortunate restrictions.  I guess it should
> not be too hard to avoid both of these.  Can you please give it a
> try?

I think it is possible to allow more than one format parameter or more
types. But it would make checking much more difficult.

> I think it is reasonable to assume (and specify) that, when the
> "fmt" option is used, _all_ following arguments will be passed
> (unchanged) to the sprintf() function.

I think just passing the format string directly to sprintf should be
avoided because it is unsafe. For example

=> setexpr foo fmt %s 0xffffffff

would surely lead to access on memory location outside the variable
where 0xffffffff is stored.

> This was actually one of my intentions when making this suggestion -
> to be able to construct any kind of data from pieces; say, for
> example:
> 
> => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d

I think the only way to support such expressions in a save way would
be implementing an own format string parser for setexpr with
corresponding checks if access is possible, instead of just directly
passing all values unchecked to sprintf.
 
Best regards,
Roland Gaudig

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Marek Behun]

On Tue, 29 Jun 2021 09:41:25 +0000
"Roland Gaudig (OSS)" <roland.gaudig-oss@weidmueller.com> wrote:

> I think just passing the format string directly to sprintf should be
> avoided because it is unsafe. For example
> 
> => setexpr foo fmt %s 0xffffffff  
> 
> would surely lead to access on memory location outside the variable
> where 0xffffffff is stored.

+1. I guess Wolfgang's rationale was that in U-Boot we already have
pretty serious means to break the system, so allowing the user to
directly pass wrong parameters to sprintf is not that much of a problem
since we can say that the user should know what they are doing.

But implementing a dedicated format parser for this that is also safe
is a simple exercise, imho, so I think we should do this properly, if
at all.

> > This was actually one of my intentions when making this suggestion -
> > to be able to construct any kind of data from pieces; say, for
> > example:
> >   
> > => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d  
> 
> I think the only way to support such expressions in a save way would
> be implementing an own format string parser for setexpr with
> corresponding checks if access is possible, instead of just directly
> passing all values unchecked to sprintf.

We can properly implement
 %s with field width, justification

 %c

 integral types (everything 64-bits, no reason for length modifiers,
 imho) with field width, precision, zero padding, sign forcing,
 etc...

We don't need floating points nor out of order arguments.

Marek

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk]

Dear Roland,

In message <a463f32f-8ef0-6973-f1c3-a881ee6e5d26@weidmueller.com> you wrote:
>
> > These are two pretty unfortunate restrictions.  I guess it should
> > not be too hard to avoid both of these.  Can you please give it a
> > try?
>
> I think it is possible to allow more than one format parameter or more
> types. But it would make checking much more difficult.

Maybe we need _less_ checking, not more - and maybe the needed
checking is already done in the *printf() code?

> I think just passing the format string directly to sprintf should be
> avoided because it is unsafe. For example
>
> => setexpr foo fmt %s 0xffffffff
>
> would surely lead to access on memory location outside the variable
> where 0xffffffff is stored.

Only if you make the wrong assumptions.  I would expect this to
result in

	foo=0xffffffff

in the same way as the bash builting gives

	$ printf '%s\n' 0xffffffff
	0xffffffff

> > => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d
>
> I think the only way to support such expressions in a save way would
> be implementing an own format string parser for setexpr with

Maybe it makes sense to have a look at the bash code?

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
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is  a  nanocentury.
                                               -- Tom Duff, Bell Labs

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Roland Gaudig (OSS)]

Hello Wolfgang

On 29.06.21 10:40, Wolfgang Denk wrote:
> 
> Dear Roland,
> 
> In message <a463f32f-8ef0-6973-f1c3-a881ee6e5d26@weidmueller.com> you wrote:
>>
>>> These are two pretty unfortunate restrictions.  I guess it should
>>> not be too hard to avoid both of these.  Can you please give it a
>>> try?
>>
>> I think it is possible to allow more than one format parameter or more
>> types. But it would make checking much more difficult.
> 
> Maybe we need _less_ checking, not more - and maybe the needed
> checking is already done in the *printf() code?

The problem printf does not do much checking. For example in case the
format string does not match the number of arguments or argument types
in best case it just delivers a wrong result, but the program also can
just crash. That is why I added the checks.

In contrast Bash and Busybox are reporting error messages in the above
cases.

>> I think just passing the format string directly to sprintf should be
>> avoided because it is unsafe. For example
>>
>> => setexpr foo fmt %s 0xffffffff
>>
>> would surely lead to access on memory location outside the variable
>> where 0xffffffff is stored.
> 
> Only if you make the wrong assumptions.  I would expect this to
> result in
> 
>         foo=0xffffffff
> 
> in the same way as the bash builting gives
> 
>         $ printf '%s\n' 0xffffffff
>         0xffffffff

Yes, but that requires further checks and interpretation. To maintain
the possibility to use pointers as arguments, the get_arg() function is
necessary, but in the above example it would return a ulong which needs
to be converted to a string before passing to printf, to get the above
result.

>>> => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d
>>
>> I think the only way to support such expressions in a save way would
>> be implementing an own format string parser for setexpr with
> 
> Maybe it makes sense to have a look at the bash code?

I looked at Bash code but it is quite confusing as they implement at
least three format string parsers. I think the one relevant for us is
the function printf_builtin() inside builtins/printf.dev. It has a
length of about 450 lines.

I also had a look at the busybox shell. They implemented their own
format string parser too, which is also about 450 lines long.

I don't see a leaner way for implementing a Bash like printf
functionality with multiple arguments and all kinds of supported format
types. When adding that format string capabilities in my opinion it
should be a configuration option to keep code size low on systems not
needing that functionality. Also I would tend to make the specific
format string features configurable. For example in my application only
decimal conversion is needed, also enabling floating point support
would just increase code without bringing any benefits.

Best regards,
Roland Gaudig

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Sean Anderson]

On 6/29/21 4:37 AM, Wolfgang Denk wrote:
> Dear Roland,
> 
> In message <20210628151750.572837-1-roland.gaudig-oss@weidmueller.com> you wrote:
>>
>>
>> U-Boot uses almost everywhere hexadecimal numbers. But some bootargs
>> passed to Linux are expecting decimal numbers. As long as the values
>> are in the range 0 to 9 it is sufficient to just strip 0x from the
>> number. But for greater values a method for converting numbers to
>> decimal is needed.
>>
>> This patch adds C like format string capabilities to the setexpr
>> command. Here are some examples:
> 
> Thanks!
> 
>> In contrast to the original C format strings the number of parameters
>> is limited to one. As the get_arg() function returns unsigned long
>> type, the format string commands are limited to those which are
>> operating on unsigned long type.
> 
> These are two pretty unfortunate restrictions.  I guess it should
> not be too hard to avoid both of these.  Can you please give it a
> try?
> 
> I think it is reasonable to assume (and specify) that, when the
> "fmt" option is used, _all_ following arguments will be passed
> (unchanged) to the sprintf() function.
> 
> This was actually one of my intentions when making this suggestion -
> to be able to construct any kind of data from pieces; say, for
> example:
> 
> => setexpr foo fmt "%0x08x-%s-%d-%s" $a $b $c $d
> 
> Best regards,
> 
> Wolfgang Denk
> 

The issue with this is twofold. First, there is no portable way to
construct a va_list from C code. So the likely way to do this would be
to set an arbitrary limit, and then just pass the arguments in. E.g.
something like

	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc */);

but of course there is no way to check that the format string matches
the correct number of arguments. This is a pretty big footgun.

The other problem is that things like `%d` expect a number and not a
string. So you would have to reimplement snprintf anyway so that it
expects all of its arguments to be strings, and calls strtoul as
appropriate.  And considering that the *printf functions take 5k
already, this reimplementation may add a significant amount of code.
For this reason, I'd much prefer to just have `hex` and `dec` functions
which do the appropriate conversions.

--Sean

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk]

Dear Sean,

In message <19b6eeea-2aad-972b-aeeb-8959aab17d7a@gmail.com> you wrote:
>
> The issue with this is twofold. First, there is no portable way to
> construct a va_list from C code. So the likely way to do this would be
> to set an arbitrary limit, and then just pass the arguments in. E.g.
> something like

We already have an argument list: it's what's being passed to the
"setexpr" command, minus the initial arguments.

> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc */);

Why this test on argc?  If it's less than 4, argv[4] should be NULL
anyway.

> but of course there is no way to check that the format string matches
> the correct number of arguments. This is a pretty big footgun.

You have this problem always when you have user provided format
strings and arguments.  We don't have to re-invent the wheel here.
I repeat myself: maybe we should have a look at bash's
implementation of the printf builtin command?  there I get for
example this:

	$ printf "%d %d %d\n" 3
	3 0 0
	$ printf "%d %d %d\n" foo bar
	-bash: printf: foo: invalid number
	-bash: printf: bar: invalid number
	0 0 0

> The other problem is that things like `%d` expect a number and not a
> string. So you would have to reimplement snprintf anyway so that it
> expects all of its arguments to be strings, and calls strtoul as
> appropriate.  And considering that the *printf functions take 5k
> already, this reimplementation may add a significant amount of code.
> For this reason, I'd much prefer to just have `hex` and `dec` functions
> which do the appropriate conversions.

Eventually the format checking can be kept out of the generic
*printf() code; it could then be optional/configurable with the
"fmt" option in the setexpr command.

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
Every program has at least one bug and can be shortened by  at  least
one  instruction  --  from  which,  by induction, one can deduce that
every program can be reduced to one instruction which doesn't work.

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Sean Anderson]

On 6/29/21 11:13 AM, Wolfgang Denk wrote:
> Dear Sean,
> 
> In message <19b6eeea-2aad-972b-aeeb-8959aab17d7a@gmail.com> you wrote:
>>
>> The issue with this is twofold. First, there is no portable way to
>> construct a va_list from C code. So the likely way to do this would be
>> to set an arbitrary limit, and then just pass the arguments in. E.g.
>> something like
> 
> We already have an argument list: it's what's being passed to the
> "setexpr" command, minus the initial arguments.
> 
>> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc */);
> 
> Why this test on argc?  If it's less than 4, argv[4] should be NULL
> anyway.

	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL,
		 argc >= 5 ? argv[5] : NULL, argc >= 6 ? argv[6] : NULL, /* etc */);

and you keep doing this until you get to whatever number of arguments
you'd like.

> 
>> but of course there is no way to check that the format string matches
>> the correct number of arguments. This is a pretty big footgun.
> 
> You have this problem always when you have user provided format
> strings and arguments.  We don't have to re-invent the wheel here.
> I repeat myself: maybe we should have a look at bash's
> implementation of the printf builtin command?  there I get for
> example this:
> 
> 	$ printf "%d %d %d\n" 3
> 	3 0 0
> 	$ printf "%d %d %d\n" foo bar
> 	-bash: printf: foo: invalid number
> 	-bash: printf: bar: invalid number
> 	0 0 0
>> The other problem is that things like `%d` expect a number and not a
>> string. So you would have to reimplement snprintf anyway so that it
>> expects all of its arguments to be strings, and calls strtoul as
>> appropriate.  And considering that the *printf functions take 5k
>> already, this reimplementation may add a significant amount of code.
>> For this reason, I'd much prefer to just have `hex` and `dec` functions
>> which do the appropriate conversions.
> 
> Eventually the format checking can be kept out of the generic
> *printf() code; it could then be optional/configurable with the
> "fmt" option in the setexpr command.

It's not a "checking" problem. The issue is that "123" cannot
be passed directly to %d. So you have dig into the guts of snprintf
anyway.

--Sean

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Marek Behún]

On Wed, 30 Jun 2021 12:17:47 -0400
Sean Anderson <seanga2@gmail.com> wrote:

> On 6/29/21 11:13 AM, Wolfgang Denk wrote:
> > Dear Sean,
> > 
> > In message <19b6eeea-2aad-972b-aeeb-8959aab17d7a@gmail.com> you
> > wrote:  
> >>
> >> The issue with this is twofold. First, there is no portable way to
> >> construct a va_list from C code. So the likely way to do this
> >> would be to set an arbitrary limit, and then just pass the
> >> arguments in. E.g. something like  
> > 
> > We already have an argument list: it's what's being passed to the
> > "setexpr" command, minus the initial arguments.
> >   
> >> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] :
> >> NULL, /* etc */);  
> > 
> > Why this test on argc?  If it's less than 4, argv[4] should be NULL
> > anyway.  
> 
> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] :
> NULL, argc >= 5 ? argv[5] : NULL, argc >= 6 ? argv[6] : NULL, /* etc
> */);

This is insane. The argv[]s are strings. What if I use "%08x" as format,
and pass "123" as argument?? It would print pointer to the string.

Clearly this needs its own implementation...

Marek

Re: [PATCH 0/3] cmd: setexpr: add fmt format string operation

[Wolfgang Denk]

Dear Sean,

In message <b85f202f-8527-b149-58d5-7ee09081d645@gmail.com> you wrote:
>
> >> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL, /* etc */);
> > 
> > Why this test on argc?  If it's less than 4, argv[4] should be NULL
> > anyway.
>
> 	snprintf(buf, sizeof(buf), argv[3], argc >= 4 ? argv[4] : NULL,
> 		 argc >= 5 ? argv[5] : NULL, argc >= 6 ? argv[6] : NULL, /* etc */);
>
> and you keep doing this until you get to whatever number of arguments
> you'd like.

I'm sorry, but this is not an acceptable way to implement variadic
functions in C.  Are you aware how va_arg works?

> > Eventually the format checking can be kept out of the generic
> > *printf() code; it could then be optional/configurable with the
> > "fmt" option in the setexpr command.
>
> It's not a "checking" problem. The issue is that "123" cannot
> be passed directly to %d. So you have dig into the guts of snprintf
> anyway.

Did you read my recommendation to have a look for example at the
implementation of the printf bash builting?

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 moral of the story is: "Don't stop to  tighten  your  shoe  laces
during the Olympics 100m finals".
                             - Kevin Jones in <DEJo68.K1t@bri.hp.com>