[PATCH 00/10] setexpr: Correct various bugs and add tests plus string support

[PATCH 00/10] setexpr: Correct various bugs and add tests plus string support

[Simon Glass]

This command has several bugs that were discovered in trying to get zboot
to make use of regular expression substitution:

- reads the wrong values with 'setexpr.l' or 'setexpr' on 64-bit machines
- buffer overflow of main string buffer
- buffer overflow of 'new' string buffer
- produces the wrong string in some cases when using back references

This series corrects these bugs and adds regression tests for them. It
also adds several other tests for the command and the core logic of
setexpr.

Finally, with Chrome OS boot it is necessary to read a command-line string
from memory, process it with a regex and then provide it to the zboot
command. So this series adds support for these features, as well as string
concatenation.

This series is available at u-boot-dm/sete-working


Simon Glass (10):
  test: Add some tests for setexpr
  command: Add constants for cmd_get_data_size string / error
  setexpr: Add explicit support for 32- and 64-bit ints
  test: Add some setexpr regex tests
  setexpr: Split the core logic into its own function
  setexpr: Add some tests for buffer overflow and backref
  setexpr: Correct dropping of final unmatched string
  setexpr: Correct buffer overflow bug and enable tests
  setexpr: Convert to use a struct for values
  setexpr: Add support for strings

 cmd/itest.c           |   4 +-
 cmd/mem.c             |   2 +-
 cmd/setexpr.c         | 336 ++++++++++++++++++++++++------------
 common/command.c      |   4 +-
 include/command.h     |  43 ++++-
 include/test/suites.h |   2 +
 test/cmd/Makefile     |   1 +
 test/cmd/setexpr.c    | 384 ++++++++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c         |   3 +
 9 files changed, 664 insertions(+), 115 deletions(-)
 create mode 100644 test/cmd/setexpr.c

-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 01/10] test: Add some tests for setexpr

[Simon Glass]

This command currently has no tests. Add some for basic assignment and the
integer operations.

Note that the default size for setexpr is ulong, which varies depending on
the build machine. So for sandbox on a 64-bit host, this means that the
default size is 64 bits.

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

 include/test/suites.h |   2 +
 test/cmd/Makefile     |   1 +
 test/cmd/setexpr.c    | 162 ++++++++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c         |   3 +
 4 files changed, 168 insertions(+)
 create mode 100644 test/cmd/setexpr.c

diff --git a/include/test/suites.h b/include/test/suites.h
index ab7b3bd9cad..5c97846e7f5 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -38,6 +38,8 @@ int do_ut_mem(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_optee(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_overlay(struct cmd_tbl *cmdtp, int flag, int argc,
 		  char *const argv[]);
+int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
+		  char *const argv[]);
 int do_ut_str(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_time(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_unicode(struct cmd_tbl *cmdtp, int flag, int argc,
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 859dcda2393..b2f71af8473 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -4,3 +4,4 @@
 
 obj-y += mem.o
 obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
+obj-y += setexpr.o
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
new file mode 100644
index 00000000000..cab6fdf4b83
--- /dev/null
+++ b/test/cmd/setexpr.c
@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Tests for setexpr command
+ *
+ * Copyright 2020 Google LLC
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#include <common.h>
+#include <console.h>
+#include <mapmem.h>
+#include <dm/test.h>
+#include <test/suites.h>
+#include <test/ut.h>
+
+#define BUF_SIZE	0x100
+
+/* Declare a new mem test */
+#define SETEXPR_TEST(_name, _flags)	UNIT_TEST(_name, _flags, setexpr_test)
+
+/* Test 'setexpr' command with simply setting integers */
+static int setexpr_test_int(struct unit_test_state *uts)
+{
+	u8 *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+
+	/* byte */
+	buf[0x0] = 0x12;
+	ut_assertok(run_command("setexpr.b fred 0", 0));
+	ut_asserteq_str("0", env_get("fred"));
+	ut_assertok(run_command("setexpr.b fred *0", 0));
+	ut_asserteq_str("12", env_get("fred"));
+
+	/* 16-bit */
+	*(short *)buf = 0x2345;
+	ut_assertok(run_command("setexpr.w fred 0", 0));
+	ut_asserteq_str("0", env_get("fred"));
+	ut_assertok(run_command("setexpr.w fred *0", 0));
+	ut_asserteq_str("2345", env_get("fred"));
+
+	/* 32-bit */
+	*(u32 *)buf = 0x3456789a;
+	ut_assertok(run_command("setexpr.l fred 0", 0));
+	ut_asserteq_str("0", env_get("fred"));
+	ut_assertok(run_command("setexpr.l fred *0", 0));
+	ut_asserteq_str("ffffffff3456789a", env_get("fred"));
+
+	/* 64-bit */
+	*(u64 *)buf = 0x456789abcdef0123;
+	ut_assertok(run_command("setexpr.q fred 0", 0));
+	ut_asserteq_str("0", env_get("fred"));
+	ut_assertok(run_command("setexpr.q fred *0", 0));
+	ut_asserteq_str("456789abcdef0123", env_get("fred"));
+
+	/* default */
+	ut_assertok(run_command("setexpr fred 0", 0));
+	ut_asserteq_str("0", env_get("fred"));
+	ut_assertok(run_command("setexpr fred *0", 0));
+	ut_asserteq_str("456789abcdef0123", env_get("fred"));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_int, UT_TESTF_CONSOLE_REC);
+
+/* Test 'setexpr' command with + operator */
+static int setexpr_test_plus(struct unit_test_state *uts)
+{
+	char *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+
+	/* byte */
+	buf[0x0] = 0x12;
+	buf[0x10] = 0x34;
+	ut_assertok(run_command("setexpr.b fred *0 + *10", 0));
+	ut_asserteq_str("46", env_get("fred"));
+
+	/* 16-bit */
+	*(short *)buf = 0x2345;
+	*(short *)(buf + 0x10) = 0xf012;
+	ut_assertok(run_command("setexpr.w fred *0 + *10", 0));
+	ut_asserteq_str("11357", env_get("fred"));
+
+	/* 32-bit */
+	*(u32 *)buf = 0x3456789a;
+	*(u32 *)(buf + 0x10) = 0xc3384235;
+	ut_assertok(run_command("setexpr.l fred *0 + *10", 0));
+	ut_asserteq_str("fffffffef78ebacf", env_get("fred"));
+
+	/* 64-bit */
+	*(u64 *)buf = 0x456789abcdef0123;
+	*(u64 *)(buf + 0x10) = 0x4987328372849283;
+	ut_assertok(run_command("setexpr.q fred *0 + *10", 0));
+	ut_asserteq_str("8eeebc2f407393a6", env_get("fred"));
+
+	/* default */
+	ut_assertok(run_command("setexpr fred *0 + *10", 0));
+	ut_asserteq_str("8eeebc2f407393a6", env_get("fred"));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_plus, UT_TESTF_CONSOLE_REC);
+
+/* Test 'setexpr' command with other operators */
+static int setexpr_test_oper(struct unit_test_state *uts)
+{
+	char *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+
+	*(u32 *)buf = 0x1234;
+	*(u32 *)(buf + 0x10) = 0x560000;
+
+	/* Quote | to avoid confusing hush */
+	ut_assertok(run_command("setexpr fred *0 \"|\" *10", 0));
+	ut_asserteq_str("ffffffff00561234", env_get("fred"));
+
+	*(u32 *)buf = 0x561200;
+	*(u32 *)(buf + 0x10) = 0x1234;
+
+	/* Quote & to avoid confusing hush */
+	ut_assertok(run_command("setexpr.l fred *0 \"&\" *10", 0));
+	ut_asserteq_str("ffffffff00001200", env_get("fred"));
+
+	ut_assertok(run_command("setexpr.l fred *0 ^ *10", 0));
+	ut_asserteq_str("560034", env_get("fred"));
+
+	ut_assertok(run_command("setexpr.l fred *0 - *10", 0));
+	ut_asserteq_str("55ffcc", env_get("fred"));
+
+	ut_assertok(run_command("setexpr.l fred *0 * *10", 0));
+	ut_asserteq_str("ffa9dbd21ebfa800", env_get("fred"));
+
+	ut_assertok(run_command("setexpr.l fred *0 / *10", 0));
+	ut_asserteq_str("1", env_get("fred"));
+
+	ut_assertok(run_command("setexpr.l fred *0 % *10", 0));
+	ut_asserteq_str("55ffcc", env_get("fred"));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_oper, UT_TESTF_CONSOLE_REC);
+
+int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+{
+	struct unit_test *tests = ll_entry_start(struct unit_test,
+						 setexpr_test);
+	const int n_ents = ll_entry_count(struct unit_test, setexpr_test);
+
+	return cmd_ut_category("cmd_setexpr", "cmd_mem_", tests, n_ents, argc,
+			       argv);
+}
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index 8f0bc688a22..f79109e6f8e 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -75,6 +75,8 @@ static struct cmd_tbl cmd_ut_sub[] = {
 	U_BOOT_CMD_MKENT(log, CONFIG_SYS_MAXARGS, 1, do_ut_log, "", ""),
 #endif
 	U_BOOT_CMD_MKENT(mem, CONFIG_SYS_MAXARGS, 1, do_ut_mem, "", ""),
+	U_BOOT_CMD_MKENT(setexpr, CONFIG_SYS_MAXARGS, 1, do_ut_setexpr, "",
+			 ""),
 #ifdef CONFIG_UT_TIME
 	U_BOOT_CMD_MKENT(time, CONFIG_SYS_MAXARGS, 1, do_ut_time, "", ""),
 #endif
@@ -153,6 +155,7 @@ static char ut_help_text[] =
 #ifdef CONFIG_UT_OVERLAY
 	"ut overlay [test-name]\n"
 #endif
+	"ut setexpr [test-name] - test setexpr command\n"
 #ifdef CONFIG_SANDBOX
 	"ut str - Basic test of string functions\n"
 #endif
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 02/10] command: Add constants for cmd_get_data_size string / error

[Simon Glass]

At present these values are open-coded in a few places. Add constants so
the meaning is clear.

Also add a comment to cmd_get_data_size()

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

 cmd/itest.c       |  4 ++--
 cmd/mem.c         |  2 +-
 common/command.c  |  4 ++--
 include/command.h | 26 +++++++++++++++++++++++++-
 4 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/cmd/itest.c b/cmd/itest.c
index a0cf4bee041..9a441ce9b8a 100644
--- a/cmd/itest.c
+++ b/cmd/itest.c
@@ -197,10 +197,10 @@ static int do_itest(struct cmd_tbl *cmdtp, int flag, int argc,
 #endif
 		value = binary_test (argv[2], argv[1], argv[3], w);
 		break;
-	case -2:
+	case CMD_DATA_SIZE_STR:
 		value = binary_test (argv[2], argv[1], argv[3], 0);
 		break;
-	case -1:
+	case CMD_DATA_SIZE_ERR:
 	default:
 		puts("Invalid data width specifier\n");
 		value = 0;
diff --git a/cmd/mem.c b/cmd/mem.c
index 56e1d0755b6..1d4f2bab2f9 100644
--- a/cmd/mem.c
+++ b/cmd/mem.c
@@ -393,7 +393,7 @@ static int do_mem_search(struct cmd_tbl *cmdtp, int flag, int argc,
 		 * Defaults to long if no or incorrect specification.
 		 */
 		size = cmd_get_data_size(argv[0], 4);
-		if (size < 0 && size != -2 /* string */)
+		if (size < 0 && size != CMD_DATA_SIZE_STR)
 			return 1;
 
 		argc--;
diff --git a/common/command.c b/common/command.c
index 2c491e20a74..068cb55b4cd 100644
--- a/common/command.c
+++ b/common/command.c
@@ -475,13 +475,13 @@ int cmd_get_data_size(char* arg, int default_size)
 		case 'l':
 			return 4;
 		case 's':
-			return -2;
+			return CMD_DATA_SIZE_STR;
 		case 'q':
 			if (MEM_SUPPORT_64BIT_DATA)
 				return 8;
 			/* no break */
 		default:
-			return -1;
+			return CMD_DATA_SIZE_ERR;
 		}
 	}
 	return default_size;
diff --git a/include/command.h b/include/command.h
index b9b5ec1afa0..e900f97df33 100644
--- a/include/command.h
+++ b/include/command.h
@@ -117,7 +117,31 @@ int cmd_process_error(struct cmd_tbl *cmdtp, int err);
 	defined(CONFIG_CMD_PCI) || \
 	defined(CONFIG_CMD_SETEXPR)
 #define CMD_DATA_SIZE
-extern int cmd_get_data_size(char* arg, int default_size);
+#define CMD_DATA_SIZE_ERR	(-1)
+#define CMD_DATA_SIZE_STR	(-2)
+
+/**
+ * cmd_get_data_size() - Get the data-size specifier from a command
+ *
+ * This reads a '.x' size specifier appended to a command. For example 'md.b'
+ * is the 'md' command with a '.b' specifier, meaning that the command should
+ * use bytes.
+ *
+ * Valid characters are:
+ *
+ *	b - byte
+ *	w - word (16 bits)
+ *	l - long (32 bits)
+ *	q - quad (64 bits)
+ *	s - string
+ *
+ * @arg: Pointers to the command to check. If a valid specifier is present it
+ *	will be the last character of the string, following a '.'
+ * @default_size: Default size to return if there is no specifier
+ * @return data size in bytes (1, 2, 4, 8) or CMD_DATA_SIZE_ERR for an invalid
+ *	character, or CMD_DATA_SIZE_STR for a string
+ */
+int cmd_get_data_size(char *arg, int default_size);
 #endif
 
 #ifdef CONFIG_CMD_BOOTD
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 03/10] setexpr: Add explicit support for 32- and 64-bit ints

[Simon Glass]

At present this function assumes that a size of 4 refers to a ulong. This
is true on 32-bit machines but not commonly on 64-bit machines.

This means that the 'l' specify does not work correctly with setexpr.

Add an explicit case for 32-bit values so that 64-bit machines can still
use the 'l' specifier. On 32-bit machines, 64-bit is still not supported.

This corrects the operation of the default size (which is 4 for setexpr),
so update the tests accordingly.

The original code for reading from memory was included in 47ab5ad1457
("cmd_setexpr: allow memory addresses in expressions") but I am not adding
a Fixes: tag since that code was not written with 64-bit machines in mind.

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

 cmd/setexpr.c      |  4 ++++
 test/cmd/setexpr.c | 18 +++++++++---------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index 770dc24d2b7..dd9c2574fdc 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -39,6 +39,10 @@ static ulong get_arg(char *s, int w)
 			unmap_sysmem(p);
 			return val;
 		case 4:
+			p = map_sysmem(addr, sizeof(u32));
+			val = *(u32 *)p;
+			unmap_sysmem(p);
+			return val;
 		default:
 			p = map_sysmem(addr, sizeof(ulong));
 			val = *p;
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index cab6fdf4b83..e950c380ce0 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -45,7 +45,7 @@ static int setexpr_test_int(struct unit_test_state *uts)
 	ut_assertok(run_command("setexpr.l fred 0", 0));
 	ut_asserteq_str("0", env_get("fred"));
 	ut_assertok(run_command("setexpr.l fred *0", 0));
-	ut_asserteq_str("ffffffff3456789a", env_get("fred"));
+	ut_asserteq_str("3456789a", env_get("fred"));
 
 	/* 64-bit */
 	*(u64 *)buf = 0x456789abcdef0123;
@@ -58,7 +58,7 @@ static int setexpr_test_int(struct unit_test_state *uts)
 	ut_assertok(run_command("setexpr fred 0", 0));
 	ut_asserteq_str("0", env_get("fred"));
 	ut_assertok(run_command("setexpr fred *0", 0));
-	ut_asserteq_str("456789abcdef0123", env_get("fred"));
+	ut_asserteq_str("cdef0123", env_get("fred"));
 
 	unmap_sysmem(buf);
 
@@ -90,7 +90,7 @@ static int setexpr_test_plus(struct unit_test_state *uts)
 	*(u32 *)buf = 0x3456789a;
 	*(u32 *)(buf + 0x10) = 0xc3384235;
 	ut_assertok(run_command("setexpr.l fred *0 + *10", 0));
-	ut_asserteq_str("fffffffef78ebacf", env_get("fred"));
+	ut_asserteq_str("f78ebacf", env_get("fred"));
 
 	/* 64-bit */
 	*(u64 *)buf = 0x456789abcdef0123;
@@ -100,7 +100,7 @@ static int setexpr_test_plus(struct unit_test_state *uts)
 
 	/* default */
 	ut_assertok(run_command("setexpr fred *0 + *10", 0));
-	ut_asserteq_str("8eeebc2f407393a6", env_get("fred"));
+	ut_asserteq_str("1407393a6", env_get("fred"));
 
 	unmap_sysmem(buf);
 
@@ -121,14 +121,14 @@ static int setexpr_test_oper(struct unit_test_state *uts)
 
 	/* Quote | to avoid confusing hush */
 	ut_assertok(run_command("setexpr fred *0 \"|\" *10", 0));
-	ut_asserteq_str("ffffffff00561234", env_get("fred"));
+	ut_asserteq_str("561234", env_get("fred"));
 
 	*(u32 *)buf = 0x561200;
 	*(u32 *)(buf + 0x10) = 0x1234;
 
 	/* Quote & to avoid confusing hush */
 	ut_assertok(run_command("setexpr.l fred *0 \"&\" *10", 0));
-	ut_asserteq_str("ffffffff00001200", env_get("fred"));
+	ut_asserteq_str("1200", env_get("fred"));
 
 	ut_assertok(run_command("setexpr.l fred *0 ^ *10", 0));
 	ut_asserteq_str("560034", env_get("fred"));
@@ -137,13 +137,13 @@ static int setexpr_test_oper(struct unit_test_state *uts)
 	ut_asserteq_str("55ffcc", env_get("fred"));
 
 	ut_assertok(run_command("setexpr.l fred *0 * *10", 0));
-	ut_asserteq_str("ffa9dbd21ebfa800", env_get("fred"));
+	ut_asserteq_str("61ebfa800", env_get("fred"));
 
 	ut_assertok(run_command("setexpr.l fred *0 / *10", 0));
-	ut_asserteq_str("1", env_get("fred"));
+	ut_asserteq_str("4ba", env_get("fred"));
 
 	ut_assertok(run_command("setexpr.l fred *0 % *10", 0));
-	ut_asserteq_str("55ffcc", env_get("fred"));
+	ut_asserteq_str("838", env_get("fred"));
 
 	unmap_sysmem(buf);
 
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 04/10] test: Add some setexpr regex tests

[Simon Glass]

Add tests for the setexpr regex commands.

Note that these tests currently crash on sandbox due to an existing bug in
the setexpr implementation, so two of the tests are commented out.

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

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

diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index e950c380ce0..de54561917c 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -151,6 +151,64 @@ static int setexpr_test_oper(struct unit_test_state *uts)
 }
 SETEXPR_TEST(setexpr_test_oper, UT_TESTF_CONSOLE_REC);
 
+/* Test 'setexpr' command with regex */
+static int setexpr_test_regex(struct unit_test_state *uts)
+{
+	char *buf, *val;
+
+	buf = map_sysmem(0, BUF_SIZE);
+
+	/* Single substitution */
+	ut_assertok(run_command("setenv fred 'this is a test'", 0));
+	ut_assertok(run_command("setexpr fred sub is us", 0));
+	val = env_get("fred");
+	ut_asserteq_str("thus is a test", val);
+
+	/* Global substitution */
+	ut_assertok(run_command("setenv fred 'this is a test'", 0));
+	if (0) {
+		/* Causes a crash at present due to a bug in setexpr */
+		ut_assertok(run_command("setexpr fred gsub is us", 0));
+		val = env_get("fred");
+		ut_asserteq_str("thus us a test", val);
+	}
+	/* Global substitution */
+	ut_assertok(run_command("setenv fred 'this is a test'", 0));
+	ut_assertok(run_command("setenv mary 'this is a test'", 0));
+	ut_assertok(run_command("setexpr fred gsub is us \"${mary}\"", 0));
+	val = env_get("fred");
+	ut_asserteq_str("thus us a test", val);
+	val = env_get("mary");
+	ut_asserteq_str("this is a test", val);
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_regex, UT_TESTF_CONSOLE_REC);
+
+/* Test 'setexpr' command with regex replacement that expands the string */
+static int setexpr_test_regex_inc(struct unit_test_state *uts)
+{
+	char *buf, *val;
+
+	buf = map_sysmem(0, BUF_SIZE);
+
+	ut_assertok(run_command("setenv fred 'this is a test'", 0));
+	if (0) {
+		/* Causes a crash at present due to a bug in setexpr */
+		ut_assertok(run_command("setexpr fred gsub is much_longer_string",
+					0));
+		val = env_get("fred");
+		ut_asserteq_str("thmuch_longer_string much_longer_string a test",
+				val);
+	}
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_regex_inc, UT_TESTF_CONSOLE_REC);
+
 int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct unit_test *tests = ll_entry_start(struct unit_test,
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 05/10] setexpr: Split the core logic into its own function

[Simon Glass]

At present this function always allocates a large amount of stack, and
selects its own size for buffers. This makes it hard to test the code
for buffer overflow.

Separate out the inner logic of the substitution so that tests can call
this directly. This will allow checking that the algorithm does not
overflow the buffer.

Fix up one of the error lines at the same time, since it should be
printing nbuf_size, not data_size.

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

 cmd/setexpr.c | 156 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 98 insertions(+), 58 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index dd9c2574fdc..fe3435b4d99 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -58,9 +58,6 @@ static ulong get_arg(char *s, int w)
 
 #include <slre.h>
 
-#define SLRE_BUFSZ	16384
-#define SLRE_PATSZ	4096
-
 /*
  * memstr - Find the first substring in memory
  * @s1: The string to be searched
@@ -83,13 +80,24 @@ static char *memstr(const char *s1, int l1, const char *s2, int l2)
 	return NULL;
 }
 
-static char *substitute(char *string,	/* string buffer */
-			int *slen,	/* current string length */
-			int ssize,	/* string bufer size */
-			const char *old,/* old (replaced) string */
-			int olen,	/* length of old string */
-			const char *new,/* new (replacement) string */
-			int nlen)	/* length of new string */
+/**
+ * substitute() - Substitute part of one string with another
+ *
+ * This updates @string so that the first occurrence of @old is replaced with
+ * @new
+ *
+ * @string: String buffer containing string to update at the start
+ * @slen: Pointer to current string length, updated on success
+ * @ssize: Size of string buffer
+ * @old: Old string to find in the buffer (no terminator needed)
+ * @olen: Length of @old excluding terminator
+ * @new: New string to replace @old with
+ * @nlen: Length of @new excluding terminator
+ * @return pointer to immediately after the copied @new in @string, or NULL if
+ *	no replacement took place
+ */
+static char *substitute(char *string, int *slen, int ssize,
+			const char *old, int olen, const char *new, int nlen)
 {
 	char *p = memstr(string, *slen, old, olen);
 
@@ -118,7 +126,7 @@ static char *substitute(char *string,	/* string buffer */
 		memmove(p + nlen, p + olen, tail);
 	}
 
-	/* insert substitue */
+	/* insert substitute */
 	memcpy(p, new, nlen);
 
 	*slen += nlen - olen;
@@ -126,52 +134,33 @@ static char *substitute(char *string,	/* string buffer */
 	return p + nlen;
 }
 
-/*
- * Perform regex operations on a environment variable
+/**
+ * regex_sub() - Replace a regex pattern with a string
  *
- * Returns 0 if OK, 1 in case of errors.
+ * @data: Buffer containing the string to update
+ * @data_size: Size of buffer (must be large enough for the new string)
+ * @nbuf: Back-reference buffer
+ * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
+ *	all back-reference expansions)
+ * @r: Regular expression to find
+ * @s: String to replace with
+ * @global: true to replace all matches in @data, false to replace just the
+ *	first
+ * @return 0 if OK, 1 on error
  */
-static int regex_sub(const char *name,
-	const char *r, const char *s, const char *t,
-	int global)
+static int regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
+		     const char *r, const char *s, bool global)
 {
 	struct slre slre;
-	char data[SLRE_BUFSZ];
 	char *datap = data;
-	const char *value;
 	int res, len, nlen, loop;
 
-	if (name == NULL)
-		return 1;
-
 	if (slre_compile(&slre, r) == 0) {
 		printf("Error compiling regex: %s\n", slre.err_str);
 		return 1;
 	}
 
-	if (t == NULL) {
-		value = env_get(name);
-
-		if (value == NULL) {
-			printf("## Error: variable \"%s\" not defined\n", name);
-			return 1;
-		}
-		t = value;
-	}
-
-	debug("REGEX on %s=%s\n", name, t);
-	debug("REGEX=\"%s\", SUBST=\"%s\", GLOBAL=%d\n",
-		r, s ? s : "<NULL>", global);
-
-	len = strlen(t);
-	if (len + 1 > SLRE_BUFSZ) {
-		printf("## error: subst buffer overflow: have %d, need %d\n",
-			SLRE_BUFSZ, len + 1);
-		return 1;
-	}
-
-	strcpy(data, t);
-
+	len = strlen(data);
 	if (s == NULL)
 		nlen = 0;
 	else
@@ -179,7 +168,6 @@ static int regex_sub(const char *name,
 
 	for (loop = 0;; loop++) {
 		struct cap caps[slre.num_caps + 2];
-		char nbuf[SLRE_PATSZ];
 		const char *old;
 		char *np;
 		int i, olen;
@@ -199,7 +187,7 @@ static int regex_sub(const char *name,
 
 		if (res == 0) {
 			if (loop == 0) {
-				printf("%s: No match\n", t);
+				printf("%s: No match\n", data);
 				return 1;
 			} else {
 				break;
@@ -208,17 +196,15 @@ static int regex_sub(const char *name,
 
 		debug("## MATCH ## %s\n", data);
 
-		if (s == NULL) {
-			printf("%s=%s\n", name, t);
+		if (!s)
 			return 1;
-		}
 
 		old = caps[0].ptr;
 		olen = caps[0].len;
 
-		if (nlen + 1 >= SLRE_PATSZ) {
+		if (nlen + 1 >= nbuf_size) {
 			printf("## error: pattern buffer overflow: have %d, need %d\n",
-				SLRE_BUFSZ, nlen + 1);
+			       nbuf_size, nlen + 1);
 			return 1;
 		}
 		strcpy(nbuf, s);
@@ -263,7 +249,7 @@ static int regex_sub(const char *name,
 					break;
 
 				np = substitute(np, &nlen,
-					SLRE_PATSZ,
+					nbuf_size,
 					backref, 2,
 					caps[i].ptr, caps[i].len);
 
@@ -273,9 +259,8 @@ static int regex_sub(const char *name,
 		}
 		debug("## SUBST(2) ## %s\n", nbuf);
 
-		datap = substitute(datap, &len, SLRE_BUFSZ,
-				old, olen,
-				nbuf, nlen);
+		datap = substitute(datap, &len, data_size, old, olen,
+				   nbuf, nlen);
 
 		if (datap == NULL)
 			return 1;
@@ -289,6 +274,61 @@ static int regex_sub(const char *name,
 	}
 	debug("## FINAL (now env_set()) :  %s\n", data);
 
+	return 0;
+}
+
+#define SLRE_BUFSZ	16384
+#define SLRE_PATSZ	4096
+
+/*
+ * Perform regex operations on a environment variable
+ *
+ * Returns 0 if OK, 1 in case of errors.
+ */
+static int regex_sub_var(const char *name, const char *r, const char *s,
+			 const char *t, int global)
+{
+	struct slre slre;
+	char data[SLRE_BUFSZ];
+	char nbuf[SLRE_PATSZ];
+	const char *value;
+	int len;
+	int ret;
+
+	if (!name)
+		return 1;
+
+	if (slre_compile(&slre, r) == 0) {
+		printf("Error compiling regex: %s\n", slre.err_str);
+		return 1;
+	}
+
+	if (!t) {
+		value = env_get(name);
+		if (!value) {
+			printf("## Error: variable \"%s\" not defined\n", name);
+			return 1;
+		}
+		t = value;
+	}
+
+	debug("REGEX on %s=%s\n", name, t);
+	debug("REGEX=\"%s\", SUBST=\"%s\", GLOBAL=%d\n", r, s ? s : "<NULL>",
+	      global);
+
+	len = strlen(t);
+	if (len + 1 > SLRE_BUFSZ) {
+		printf("## error: subst buffer overflow: have %d, need %d\n",
+		       SLRE_BUFSZ, len + 1);
+		return 1;
+	}
+
+	strcpy(data, t);
+
+	ret = regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s, global);
+	if (ret)
+		return 1;
+
 	printf("%s=%s\n", name, data);
 
 	return env_set(name, data);
@@ -331,10 +371,10 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	 * with 5 args, "t" will be NULL
 	 */
 	if (strcmp(argv[2], "gsub") == 0)
-		return regex_sub(argv[1], argv[3], argv[4], argv[5], 1);
+		return regex_sub_var(argv[1], argv[3], argv[4], argv[5], 1);
 
 	if (strcmp(argv[2], "sub") == 0)
-		return regex_sub(argv[1], argv[3], argv[4], argv[5], 0);
+		return regex_sub_var(argv[1], argv[3], argv[4], argv[5], 0);
 #endif
 
 	/* standard operators: "setexpr name val1 op val2" */
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Simon Glass]

Add tests to check for buffer overflow using simple replacement as well
as back references. At present these don't fully pass.

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

 cmd/setexpr.c      | 21 +++--------
 include/command.h  | 17 +++++++++
 test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+), 17 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index fe3435b4d99..dbb43b3be2f 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -134,22 +134,8 @@ static char *substitute(char *string, int *slen, int ssize,
 	return p + nlen;
 }
 
-/**
- * regex_sub() - Replace a regex pattern with a string
- *
- * @data: Buffer containing the string to update
- * @data_size: Size of buffer (must be large enough for the new string)
- * @nbuf: Back-reference buffer
- * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
- *	all back-reference expansions)
- * @r: Regular expression to find
- * @s: String to replace with
- * @global: true to replace all matches in @data, false to replace just the
- *	first
- * @return 0 if OK, 1 on error
- */
-static int regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
-		     const char *r, const char *s, bool global)
+int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
+		      const char *r, const char *s, bool global)
 {
 	struct slre slre;
 	char *datap = data;
@@ -325,7 +311,8 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
 
 	strcpy(data, t);
 
-	ret = regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s, global);
+	ret = setexpr_regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s,
+				global);
 	if (ret)
 		return 1;
 
diff --git a/include/command.h b/include/command.h
index e900f97df33..e229bf2825c 100644
--- a/include/command.h
+++ b/include/command.h
@@ -183,6 +183,23 @@ extern int do_env_set_efi(struct cmd_tbl *cmdtp, int flag, int argc,
 			  char *const argv[]);
 #endif
 
+/**
+ * setexpr_regex_sub() - Replace a regex pattern with a string
+ *
+ * @data: Buffer containing the string to update
+ * @data_size: Size of buffer (must be large enough for the new string)
+ * @nbuf: Back-reference buffer
+ * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
+ *	all back-reference expansions)
+ * @r: Regular expression to find
+ * @s: String to replace with
+ * @global: true to replace all matches in @data, false to replace just the
+ *	first
+ * @return 0 if OK, 1 on error
+ */
+int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
+		      const char *r, const char *s, bool global);
+
 /*
  * Error codes that commands return to cmd_process(). We use the standard 0
  * and 1 for success and failure, but add one more case - failure with a
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index de54561917c..a6940fd82dd 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -209,6 +209,95 @@ static int setexpr_test_regex_inc(struct unit_test_state *uts)
 }
 SETEXPR_TEST(setexpr_test_regex_inc, UT_TESTF_CONSOLE_REC);
 
+/* Test setexpr_regex_sub() directly to check buffer usage */
+static int setexpr_test_sub(struct unit_test_state *uts)
+{
+	char *buf, *nbuf;
+	int i;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	nbuf = map_sysmem(0x1000, BUF_SIZE);
+
+	/* Add a pattern so we can check the buffer limits */
+	memset(buf, '\xff', BUF_SIZE);
+	memset(nbuf, '\xff', BUF_SIZE);
+	for (i = BUF_SIZE; i < 0x1000; i++) {
+		buf[i] = i & 0xff;
+		nbuf[i] = i & 0xff;
+	}
+	strcpy(buf, "this is a test");
+
+	/*
+	 * This is a regression test, since a bug was found in the use of
+	 * memmove() in setexpr
+	 */
+	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE, "is",
+				      "us it is longer", true));
+	ut_asserteq_str("thus it is longer us it is longer a test", buf);
+
+	/* The following checks fail at present due to a bug in setexpr */
+	return 0;
+	for (i = BUF_SIZE; i < 0x1000; i++) {
+		ut_assertf(buf[i] == (char)i,
+			   "buf byte at %x should be %02x, got %02x)\n",
+			   i, i & 0xff, (u8)buf[i]);
+		ut_assertf(nbuf[i] == (char)i,
+			   "nbuf byte at %x should be %02x, got %02x)\n",
+			   i, i & 0xff, (u8)nbuf[i]);
+	}
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_sub, UT_TESTF_CONSOLE_REC);
+
+/* Test setexpr_regex_sub() with back references */
+static int setexpr_test_backref(struct unit_test_state *uts)
+{
+	char *buf, *nbuf;
+	int i;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	nbuf = map_sysmem(0x1000, BUF_SIZE);
+
+	/* Add a pattern so we can check the buffer limits */
+	memset(buf, '\xff', BUF_SIZE);
+	memset(nbuf, '\xff', BUF_SIZE);
+	for (i = BUF_SIZE; i < 0x1000; i++) {
+		buf[i] = i & 0xff;
+		nbuf[i] = i & 0xff;
+	}
+	strcpy(buf, "this is surely a test is it? yes this is indeed a test");
+
+	/*
+	 * This is a regression test, since a bug was found in the use of
+	 * memmove() in setexpr
+	 */
+	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE,
+				      "(this) (is) (surely|indeed)",
+				      "us \\1 \\2 \\3!", true));
+
+	/* The following checks fail at present due to bugs in setexpr */
+	return 0;
+	ut_asserteq_str("us this is surely! a test is it? yes us this is indeed! a test",
+			buf);
+
+	for (i = BUF_SIZE; i < 0x1000; i++) {
+		ut_assertf(buf[i] == (char)i,
+			   "buf byte at %x should be %02x, got %02x)\n",
+			   i, i & 0xff, (u8)buf[i]);
+		ut_assertf(nbuf[i] == (char)i,
+			   "nbuf byte@%x should be %02x, got %02x)\n",
+			   i, i & 0xff, (u8)nbuf[i]);
+	}
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_backref, UT_TESTF_CONSOLE_REC);
+
 int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct unit_test *tests = ll_entry_start(struct unit_test,
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 07/10] setexpr: Correct dropping of final unmatched string

[Simon Glass]

At present the 'nlen' variable increases with each loop. If the previous
loop had back references, then subsequent loops without back references
use the wrong value of nlen. The value is larger, meaning that the string
terminator from nbuf is copied along to the main buffer, thus terminating
the string prematurely.

This leads to the final result being truncated, e.g. missing the last
(unmatched) part of the string. So "match match tail" become
"replaced replaced" instead of "replaced replaced tail".

Fix this by resetting nlen to the correct value each time around the lop.

Fixes: 855f18ea0e6 ("setexpr: add regex substring matching and substitution")
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/setexpr.c      | 6 +-----
 test/cmd/setexpr.c | 5 ++---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index dbb43b3be2f..0cc7cf15bd7 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -147,11 +147,6 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
 	}
 
 	len = strlen(data);
-	if (s == NULL)
-		nlen = 0;
-	else
-		nlen = strlen(s);
-
 	for (loop = 0;; loop++) {
 		struct cap caps[slre.num_caps + 2];
 		const char *old;
@@ -187,6 +182,7 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
 
 		old = caps[0].ptr;
 		olen = caps[0].len;
+		nlen = strlen(s);
 
 		if (nlen + 1 >= nbuf_size) {
 			printf("## error: pattern buffer overflow: have %d, need %d\n",
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index a6940fd82dd..d06dda260e6 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -277,12 +277,11 @@ static int setexpr_test_backref(struct unit_test_state *uts)
 	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE,
 				      "(this) (is) (surely|indeed)",
 				      "us \\1 \\2 \\3!", true));
-
-	/* The following checks fail at present due to bugs in setexpr */
-	return 0;
 	ut_asserteq_str("us this is surely! a test is it? yes us this is indeed! a test",
 			buf);
 
+	/* The following checks fail at present due to a bug in setexpr */
+	return 0;
 	for (i = BUF_SIZE; i < 0x1000; i++) {
 		ut_assertf(buf[i] == (char)i,
 			   "buf byte@%x should be %02x, got %02x)\n",
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 08/10] setexpr: Correct buffer overflow bug and enable tests

[Simon Glass]

At present when more than one substitution is made this function
overwrites its buffers. Fix this bug and update the tests now that they
can pass.

Also update the debug code to show all substrings, since at present it
omits the final one.

Fixes: 855f18ea0e6 ("setexpr: add regex substring matching and substitution")
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 cmd/setexpr.c      | 10 +++++-----
 test/cmd/setexpr.c | 24 +++++++-----------------
 2 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index 0cc7cf15bd7..d364dbc2bc5 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -155,11 +155,11 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
 
 		(void) memset(caps, 0, sizeof(caps));
 
-		res = slre_match(&slre, datap, len, caps);
+		res = slre_match(&slre, datap, len - (datap - data), caps);
 
 		debug("Result: %d\n", res);
 
-		for (i = 0; i < slre.num_caps; i++) {
+		for (i = 0; i <= slre.num_caps; i++) {
 			if (caps[i].len > 0) {
 				debug("Substring %d: [%.*s]\n", i,
 					caps[i].len, caps[i].ptr);
@@ -231,7 +231,7 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
 					break;
 
 				np = substitute(np, &nlen,
-					nbuf_size,
+					nbuf_size - (np - nbuf),
 					backref, 2,
 					caps[i].ptr, caps[i].len);
 
@@ -241,8 +241,8 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
 		}
 		debug("## SUBST(2) ## %s\n", nbuf);
 
-		datap = substitute(datap, &len, data_size, old, olen,
-				   nbuf, nlen);
+		datap = substitute(datap, &len, data_size - (datap - data),
+				   old, olen, nbuf, nlen);
 
 		if (datap == NULL)
 			return 1;
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index d06dda260e6..2a897efd9bd 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -166,12 +166,10 @@ static int setexpr_test_regex(struct unit_test_state *uts)
 
 	/* Global substitution */
 	ut_assertok(run_command("setenv fred 'this is a test'", 0));
-	if (0) {
-		/* Causes a crash at present due to a bug in setexpr */
-		ut_assertok(run_command("setexpr fred gsub is us", 0));
-		val = env_get("fred");
-		ut_asserteq_str("thus us a test", val);
-	}
+	ut_assertok(run_command("setexpr fred gsub is us", 0));
+	val = env_get("fred");
+	ut_asserteq_str("thus us a test", val);
+
 	/* Global substitution */
 	ut_assertok(run_command("setenv fred 'this is a test'", 0));
 	ut_assertok(run_command("setenv mary 'this is a test'", 0));
@@ -195,14 +193,9 @@ static int setexpr_test_regex_inc(struct unit_test_state *uts)
 	buf = map_sysmem(0, BUF_SIZE);
 
 	ut_assertok(run_command("setenv fred 'this is a test'", 0));
-	if (0) {
-		/* Causes a crash at present due to a bug in setexpr */
-		ut_assertok(run_command("setexpr fred gsub is much_longer_string",
-					0));
-		val = env_get("fred");
-		ut_asserteq_str("thmuch_longer_string much_longer_string a test",
-				val);
-	}
+	ut_assertok(run_command("setexpr fred gsub is much_longer_string", 0));
+	val = env_get("fred");
+	ut_asserteq_str("thmuch_longer_string much_longer_string a test", val);
 	unmap_sysmem(buf);
 
 	return 0;
@@ -234,9 +227,6 @@ static int setexpr_test_sub(struct unit_test_state *uts)
 	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE, "is",
 				      "us it is longer", true));
 	ut_asserteq_str("thus it is longer us it is longer a test", buf);
-
-	/* The following checks fail at present due to a bug in setexpr */
-	return 0;
 	for (i = BUF_SIZE; i < 0x1000; i++) {
 		ut_assertf(buf[i] == (char)i,
 			   "buf byte@%x should be %02x, got %02x)\n",
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 09/10] setexpr: Convert to use a struct for values

[Simon Glass]

At present a ulong is used to hold operand values. This means that
strings cannot be used. While most operations are not useful for strings,
concatenation is. As a starting point to supporting strings, convert the
code to use a struct instead of a ulong for operands.

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

 cmd/setexpr.c | 111 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 44 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index d364dbc2bc5..8a3654505da 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -15,8 +15,19 @@
 #include <log.h>
 #include <mapmem.h>
 
-static ulong get_arg(char *s, int w)
+/**
+ * struct expr_arg: Holds an argument to an expression
+ *
+ * @ival: Integer value (if width is not CMD_DATA_SIZE_STR)
+ */
+struct expr_arg {
+	ulong ival;
+};
+
+static int get_arg(char *s, int w, struct expr_arg *argp)
 {
+	struct expr_arg arg;
+
 	/*
 	 * If the parameter starts with a '*' then assume it is a pointer to
 	 * the value we want.
@@ -32,26 +43,33 @@ static ulong get_arg(char *s, int w)
 			p = map_sysmem(addr, sizeof(uchar));
 			val = (ulong)*(uchar *)p;
 			unmap_sysmem(p);
-			return val;
+			arg.ival = val;
+			break;
 		case 2:
 			p = map_sysmem(addr, sizeof(ushort));
 			val = (ulong)*(ushort *)p;
 			unmap_sysmem(p);
-			return val;
+			arg.ival = val;
+			break;
 		case 4:
 			p = map_sysmem(addr, sizeof(u32));
 			val = *(u32 *)p;
 			unmap_sysmem(p);
-			return val;
+			arg.ival = val;
+			break;
 		default:
 			p = map_sysmem(addr, sizeof(ulong));
 			val = *p;
 			unmap_sysmem(p);
-			return val;
+			arg.ival = val;
+			break;
 		}
 	} else {
-		return simple_strtoul(s, NULL, 16);
+		arg.ival = simple_strtoul(s, NULL, 16);
 	}
+	*argp = arg;
+
+	return 0;
 }
 
 #ifdef CONFIG_REGEX
@@ -321,7 +339,7 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
 static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[])
 {
-	ulong a, b;
+	struct expr_arg aval, bval;
 	ulong value;
 	int w;
 
@@ -339,13 +357,12 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	w = cmd_get_data_size(argv[0], 4);
 
-	a = get_arg(argv[2], w);
+	if (get_arg(argv[2], w, &aval))
+		return CMD_RET_FAILURE;
 
 	/* plain assignment: "setexpr name value" */
-	if (argc == 3) {
-		env_set_hex(argv[1], a);
-		return 0;
-	}
+	if (argc == 3)
+		return env_set_hex(argv[1], aval.ival);
 
 	/* 5 or 6 args (6 args only with [g]sub) */
 #ifdef CONFIG_REGEX
@@ -367,39 +384,45 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (strlen(argv[3]) != 1)
 		return CMD_RET_USAGE;
 
-	b = get_arg(argv[4], w);
-
-	switch (argv[3][0]) {
-	case '|':
-		value = a | b;
-		break;
-	case '&':
-		value = a & b;
-		break;
-	case '+':
-		value = a + b;
-		break;
-	case '^':
-		value = a ^ b;
-		break;
-	case '-':
-		value = a - b;
-		break;
-	case '*':
-		value = a * b;
-		break;
-	case '/':
-		value = a / b;
-		break;
-	case '%':
-		value = a % b;
-		break;
-	default:
-		printf("invalid op\n");
-		return 1;
-	}
+	if (get_arg(argv[4], w, &bval))
+		return CMD_RET_FAILURE;
 
-	env_set_hex(argv[1], value);
+	if (w != CMD_DATA_SIZE_STR) {
+		ulong a = aval.ival;
+		ulong b = bval.ival;
+
+		switch (argv[3][0]) {
+		case '|':
+			value = a | b;
+			break;
+		case '&':
+			value = a & b;
+			break;
+		case '+':
+			value = a + b;
+			break;
+		case '^':
+			value = a ^ b;
+			break;
+		case '-':
+			value = a - b;
+			break;
+		case '*':
+			value = a * b;
+			break;
+		case '/':
+			value = a / b;
+			break;
+		case '%':
+			value = a % b;
+			break;
+		default:
+			printf("invalid op\n");
+			return 1;
+		}
+
+		env_set_hex(argv[1], value);
+	}
 
 	return 0;
 }
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 10/10] setexpr: Add support for strings

[Simon Glass]

Add support for dealing with string operands, including reading a string
from memory into an environment variable and concatenating two strings.

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

 cmd/setexpr.c      | 82 +++++++++++++++++++++++++++++++++++++++----
 test/cmd/setexpr.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 7 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index 8a3654505da..e828be39700 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -13,15 +13,21 @@
 #include <command.h>
 #include <env.h>
 #include <log.h>
+#include <malloc.h>
 #include <mapmem.h>
+#include <linux/sizes.h>
 
 /**
  * struct expr_arg: Holds an argument to an expression
  *
  * @ival: Integer value (if width is not CMD_DATA_SIZE_STR)
+ * @sval: String value (if width is CMD_DATA_SIZE_STR)
  */
 struct expr_arg {
-	ulong ival;
+	union {
+		ulong ival;
+		char *sval;
+	};
 };
 
 static int get_arg(char *s, int w, struct expr_arg *argp)
@@ -36,6 +42,8 @@ static int get_arg(char *s, int w, struct expr_arg *argp)
 		ulong *p;
 		ulong addr;
 		ulong val;
+		int len;
+		char *str;
 
 		addr = simple_strtoul(&s[1], NULL, 16);
 		switch (w) {
@@ -51,6 +59,21 @@ static int get_arg(char *s, int w, struct expr_arg *argp)
 			unmap_sysmem(p);
 			arg.ival = val;
 			break;
+		case CMD_DATA_SIZE_STR:
+			p = map_sysmem(addr, SZ_64K);
+
+			/* Maximum string length of 64KB plus terminator */
+			len = strnlen((char *)p, SZ_64K) + 1;
+			str = malloc(len);
+			if (!str) {
+				printf("Out of memory\n");
+				return -ENOMEM;
+			}
+			memcpy(str, p, len);
+			str[len - 1] = '\0';
+			unmap_sysmem(p);
+			arg.sval = str;
+			break;
 		case 4:
 			p = map_sysmem(addr, sizeof(u32));
 			val = *(u32 *)p;
@@ -65,6 +88,8 @@ static int get_arg(char *s, int w, struct expr_arg *argp)
 			break;
 		}
 	} else {
+		if (w == CMD_DATA_SIZE_STR)
+			return -EINVAL;
 		arg.ival = simple_strtoul(s, NULL, 16);
 	}
 	*argp = arg;
@@ -341,6 +366,7 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 {
 	struct expr_arg aval, bval;
 	ulong value;
+	int ret = 0;
 	int w;
 
 	/*
@@ -361,8 +387,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_FAILURE;
 
 	/* plain assignment: "setexpr name value" */
-	if (argc == 3)
-		return env_set_hex(argv[1], aval.ival);
+	if (argc == 3) {
+		if (w == CMD_DATA_SIZE_STR) {
+			ret = env_set(argv[1], aval.sval);
+			free(aval.sval);
+		} else {
+			ret = env_set_hex(argv[1], aval.ival);
+		}
+
+		return ret;
+	}
 
 	/* 5 or 6 args (6 args only with [g]sub) */
 #ifdef CONFIG_REGEX
@@ -384,10 +418,38 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (strlen(argv[3]) != 1)
 		return CMD_RET_USAGE;
 
-	if (get_arg(argv[4], w, &bval))
+	if (get_arg(argv[4], w, &bval)) {
+		if (w == CMD_DATA_SIZE_STR)
+			free(aval.sval);
 		return CMD_RET_FAILURE;
+	}
+
+	if (w == CMD_DATA_SIZE_STR) {
+		int len;
+		char *str;
 
-	if (w != CMD_DATA_SIZE_STR) {
+		switch (argv[3][0]) {
+		case '+':
+			len = strlen(aval.sval) + strlen(bval.sval) + 1;
+			str = malloc(len);
+			if (!str) {
+				printf("Out of memory\n");
+				ret = CMD_RET_FAILURE;
+			} else {
+				/* These were copied out and checked earlier */
+				strcpy(str, aval.sval);
+				strcat(str, bval.sval);
+				ret = env_set(argv[1], str);
+				if (ret)
+					printf("Could not set var\n");
+				free(str);
+			}
+			break;
+		default:
+			printf("invalid op\n");
+			ret = 1;
+		}
+	} else {
 		ulong a = aval.ival;
 		ulong b = bval.ival;
 
@@ -424,15 +486,21 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 		env_set_hex(argv[1], value);
 	}
 
-	return 0;
+	if (w == CMD_DATA_SIZE_STR) {
+		free(aval.sval);
+		free(bval.sval);
+	}
+
+	return ret;
 }
 
 U_BOOT_CMD(
 	setexpr, 6, 0, do_setexpr,
 	"set environment variable as the result of eval expression",
-	"[.b, .w, .l] name [*]value1 <op> [*]value2\n"
+	"[.b, .w, .l, .s] name [*]value1 <op> [*]value2\n"
 	"    - set environment variable 'name' to the result of the evaluated\n"
 	"      expression specified by <op>.  <op> can be &, |, ^, +, -, *, /, %\n"
+	"      (for strings only + is supported)\n"
 	"      size argument is only meaningful if value1 and/or value2 are\n"
 	"      memory addresses (*)\n"
 	"setexpr[.b, .w, .l] name [*]value\n"
diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
index 2a897efd9bd..fd6d869c0ed 100644
--- a/test/cmd/setexpr.c
+++ b/test/cmd/setexpr.c
@@ -287,6 +287,92 @@ static int setexpr_test_backref(struct unit_test_state *uts)
 }
 SETEXPR_TEST(setexpr_test_backref, UT_TESTF_CONSOLE_REC);
 
+/* Test 'setexpr' command with setting strings */
+static int setexpr_test_str(struct unit_test_state *uts)
+{
+	ulong start_mem;
+	char *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+
+	/*
+	 * Set 'fred' to the same length as we expect to get below, to avoid a
+	 * new allocation in 'setexpr'. That way we can check for memory leaks.
+	 */
+	ut_assertok(env_set("fred", "x"));
+	start_mem = ut_check_free();
+	strcpy(buf, "hello");
+	ut_asserteq(1, run_command("setexpr.s fred 0", 0));
+	ut_assertok(ut_check_delta(start_mem));
+
+	start_mem = ut_check_free();
+	ut_assertok(env_set("fred", "12345"));
+	ut_assertok(run_command("setexpr.s fred *0", 0));
+	ut_asserteq_str("hello", env_get("fred"));
+	ut_assertok(ut_check_delta(start_mem));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_str, UT_TESTF_CONSOLE_REC);
+
+
+/* Test 'setexpr' command with concatenating strings */
+static int setexpr_test_str_oper(struct unit_test_state *uts)
+{
+	ulong start_mem;
+	char *buf;
+
+	buf = map_sysmem(0, BUF_SIZE);
+	memset(buf, '\xff', BUF_SIZE);
+	strcpy(buf, "hello");
+	strcpy(buf + 0x10, " there");
+
+	ut_assertok(console_record_reset_enable());
+	start_mem = ut_check_free();
+	ut_asserteq(1, run_command("setexpr.s fred *0 * *10", 0));
+	ut_assertok(ut_check_delta(start_mem));
+	ut_assert_nextline("invalid op");
+	ut_assert_console_end();
+
+	/*
+	 * Set 'fred' to the same length as we expect to get below, to avoid a
+	 * new allocation in 'setexpr'. That way we can check for memory leaks.
+	 */
+	ut_assertok(env_set("fred", "12345012345"));
+	start_mem = ut_check_free();
+	ut_assertok(run_command("setexpr.s fred *0 + *10", 0));
+	ut_asserteq_str("hello there", env_get("fred"));
+	ut_assertok(ut_check_delta(start_mem));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_str_oper, UT_TESTF_CONSOLE_REC);
+
+/* Test 'setexpr' command with a string that is too long */
+static int setexpr_test_str_long(struct unit_test_state *uts)
+{
+	const int size = 128 << 10;  /* setexpr strings are a max of 64KB */
+	char *buf, *val;
+
+	buf = map_sysmem(0, size);
+	memset(buf, 'a', size);
+
+	/* String should be truncated to 64KB */
+	ut_assertok(run_command("setexpr.s fred *0", 0));
+	val = env_get("fred");
+	ut_asserteq(64 << 10, strlen(val));
+
+	unmap_sysmem(buf);
+
+	return 0;
+}
+SETEXPR_TEST(setexpr_test_str_long, UT_TESTF_CONSOLE_REC);
+
 int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	struct unit_test *tests = ll_entry_start(struct unit_test,
-- 
2.29.1.341.ge80a0c044ae-goog

[PATCH 10/10] setexpr: Add support for strings

[Marek Behun]

What is the purpose of + operator on strings?
Can't we use setenv "${a}${b}" ?

[PATCH 10/10] setexpr: Add support for strings

[Simon Glass]

Hi Marek,

On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote:
>
> What is the purpose of + operator on strings?
> Can't we use setenv "${a}${b}" ?

Yes, that does the same thing, although it is a bit clumsy.

setenv a *10
setenv b *100
setenv c "${a}${b}"

instead of

setexpr c *10 + *100

Regards,
Simon

[PATCH 10/10] setexpr: Add support for strings

[Marek Behun]

On Tue, 3 Nov 2020 08:12:17 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi Marek,
> 
> On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote:
> >
> > What is the purpose of + operator on strings?
> > Can't we use setenv "${a}${b}" ?  
> 
> Yes, that does the same thing, although it is a bit clumsy.
> 
> setenv a *10
> setenv b *100
> setenv c "${a}${b}"
> 
> instead of
> 
> setexpr c *10 + *100

Hi Simon,

I don't know. It provides the same functionality that exists, but only
adds code.
Is someone really going to use this?

Marek

PS: What I think would be more useful is to add substringing
functionality into hush, so e.g. ${a:3:5}, and pattern substitions:
${parameter/pattern/string} ...

[PATCH 10/10] setexpr: Add support for strings

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3SXdKaKmYCw=Q0w-JGhghAPVhwHdtG5q2dNEMNiY60Xg@mail.gmail.com> you wrote:
>
> > What is the purpose of + operator on strings?
> > Can't we use setenv "${a}${b}" ?
>
> Yes, that does the same thing, although it is a bit clumsy.
>
> setenv a *10
> setenv b *100
> setenv c "${a}${b}"
>
> instead of
>
> setexpr c *10 + *100

I don't get it.  The equivalent to "${a}${b}" would be

	setexpr c "*10*100"

which is even simpler?

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 at denx.de
"You know, after a woman's raised a family and so on,  she  wants  to
start living her own life."   "Whose life she's _been_ living, then?"
                                  - Terry Pratchett, _Witches Abroad_

[PATCH 10/10] setexpr: Add support for strings

[Wolfgang Denk]

Dear Marek,

In message <20201103173011.08e22c11@nic.cz> you wrote:
>
> PS: What I think would be more useful is to add substringing
> functionality into hush, so e.g. ${a:3:5}, and pattern substitions:
> ${parameter/pattern/string} ...

No, NAK, don't.

At least not to the current imple,entation of hush.  First, upgrade
to the recent version from BusyBox, and then we might discuss
extensions.  Actually we should eventually discuss these in BusyBox
to have such stuff in "hush mainline" ...

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 at denx.de
"There's only one way to have a happy marriage and as soon as I learn
what it is I'll get married again."                  - Clint Eastwood

[PATCH 10/10] setexpr: Add support for strings

[Simon Glass]

Hi Marek,

On Tue, 3 Nov 2020 at 09:30, Marek Behun <marek.behun@nic.cz> wrote:
>
> On Tue, 3 Nov 2020 08:12:17 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Marek,
> >
> > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > What is the purpose of + operator on strings?
> > > Can't we use setenv "${a}${b}" ?
> >
> > Yes, that does the same thing, although it is a bit clumsy.
> >
> > setenv a *10
> > setenv b *100
> > setenv c "${a}${b}"
> >
> > instead of
> >
> > setexpr c *10 + *100
>
> Hi Simon,
>
> I don't know. It provides the same functionality that exists, but only
> adds code.
> Is someone really going to use this?

I don't know. Perhaps we can wait and see if anyone cares?

>
> Marek
>
> PS: What I think would be more useful is to add substringing
> functionality into hush, so e.g. ${a:3:5}, and pattern substitions:
> ${parameter/pattern/string} ...

Yes we need to upgrade hush.

Regards,
Simon

[PATCH 10/10] setexpr: Add support for strings

[Simon Glass]

Hi Wolfgang,

On Thu, 5 Nov 2020 at 09:47, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ3SXdKaKmYCw=Q0w-JGhghAPVhwHdtG5q2dNEMNiY60Xg@mail.gmail.com> you wrote:
> >
> > > What is the purpose of + operator on strings?
> > > Can't we use setenv "${a}${b}" ?
> >
> > Yes, that does the same thing, although it is a bit clumsy.
> >
> > setenv a *10
> > setenv b *100
> > setenv c "${a}${b}"
> >
> > instead of
> >
> > setexpr c *10 + *100
>
> I don't get it.  The equivalent to "${a}${b}" would be
>
>         setexpr c "*10*100"
>
> which is even simpler?

I don't see how that works. The *10 thing in my example reads a string
out of address 10.

Regards,
Simon

[PATCH 10/10] setexpr: Add support for strings

[Marek Behun]

On Thu, 5 Nov 2020 10:27:28 -0700
Simon Glass <sjg@chromium.org> wrote:

> I don't see how that works. The *10 thing in my example reads a string
> out of address 10.

/o\ ahaaaa, OK, that makes sense. So setexpr can dereference strings.
I didn't know about that, I thouth the resulting string would be
"*10*100".

In this case
Acked-by: Marek Beh?n <marek.behun@nic.cz>

[PATCH 10/10] setexpr: Add support for strings

[Wolfgang Denk]

Dear Simon.

In message <CAPnjgZ3DmeaincEcYkjeTuRijqtMZhJiDnyx_o4eSRE4gJaVFw@mail.gmail.com> you wrote:
>
> > > setexpr c *10 + *100
> >
> > I don't get it.  The equivalent to "${a}${b}" would be
> >
> >         setexpr c "*10*100"
> >
> > which is even simpler?
>
> I don't see how that works. The *10 thing in my example reads a string
> out of address 10.

Ah, got it.  This requires your "[PATCH 10/10] setexpr: Add support
for strings" first...

But then... should there not be some '.s' size specification
somewhere?

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 at denx.de
"Unix is simple, but it takes a genius to understand the simplicity."
					             - Dennis Ritchie

[PATCH 10/10] setexpr: Add support for strings

[Simon Glass]

Hi Wolfgang,

On Thu, 5 Nov 2020 at 12:10, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon.
>
> In message <CAPnjgZ3DmeaincEcYkjeTuRijqtMZhJiDnyx_o4eSRE4gJaVFw@mail.gmail.com> you wrote:
> >
> > > > setexpr c *10 + *100
> > >
> > > I don't get it.  The equivalent to "${a}${b}" would be
> > >
> > >         setexpr c "*10*100"
> > >
> > > which is even simpler?
> >
> > I don't see how that works. The *10 thing in my example reads a string
> > out of address 10.
>
> Ah, got it.  This requires your "[PATCH 10/10] setexpr: Add support
> for strings" first...
>
> But then... should there not be some '.s' size specification
> somewhere?

Ooops yes:

setexpr.s c *10 + *100

Regards,
Simon

[PATCH 10/10] setexpr: Add support for strings

[Tom Rini]

On Thu, Nov 05, 2020 at 10:27:25AM -0700, Simon Glass wrote:
> Hi Marek,
> 
> On Tue, 3 Nov 2020 at 09:30, Marek Behun <marek.behun@nic.cz> wrote:
> >
> > On Tue, 3 Nov 2020 08:12:17 -0700
> > Simon Glass <sjg@chromium.org> wrote:
> >
> > > Hi Marek,
> > >
> > > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote:
> > > >
> > > > What is the purpose of + operator on strings?
> > > > Can't we use setenv "${a}${b}" ?
> > >
> > > Yes, that does the same thing, although it is a bit clumsy.
> > >
> > > setenv a *10
> > > setenv b *100
> > > setenv c "${a}${b}"
> > >
> > > instead of
> > >
> > > setexpr c *10 + *100
> >
> > Hi Simon,
> >
> > I don't know. It provides the same functionality that exists, but only
> > adds code.
> > Is someone really going to use this?
> 
> I don't know. Perhaps we can wait and see if anyone cares?

Sorry, does that mean this support is just speculative?  Or did I
misread this part of the thread?  Thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201106/393c248a/attachment.sig>

[PATCH 10/10] setexpr: Add support for strings

[Simon Glass]

Hi Tom,

On Fri, 6 Nov 2020 at 13:58, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Nov 05, 2020 at 10:27:25AM -0700, Simon Glass wrote:
> > Hi Marek,
> >
> > On Tue, 3 Nov 2020 at 09:30, Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Tue, 3 Nov 2020 08:12:17 -0700
> > > Simon Glass <sjg@chromium.org> wrote:
> > >
> > > > Hi Marek,
> > > >
> > > > On Sun, 1 Nov 2020 at 16:08, Marek Behun <marek.behun@nic.cz> wrote:
> > > > >
> > > > > What is the purpose of + operator on strings?
> > > > > Can't we use setenv "${a}${b}" ?
> > > >
> > > > Yes, that does the same thing, although it is a bit clumsy.
> > > >
> > > > setenv a *10
> > > > setenv b *100
> > > > setenv c "${a}${b}"
> > > >
> > > > instead of
> > > >
> > > > setexpr c *10 + *100
> > >
> > > Hi Simon,
> > >
> > > I don't know. It provides the same functionality that exists, but only
> > > adds code.
> > > Is someone really going to use this?
> >
> > I don't know. Perhaps we can wait and see if anyone cares?
>
> Sorry, does that mean this support is just speculative?  Or did I
> misread this part of the thread?  Thanks.

I think it was a misunderstanding of what it does. Marek acked the
patch and Wolfgang seems happy.

Regards,
SImon

[PATCH 01/10] test: Add some tests for setexpr

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:35PM -0700, Simon Glass wrote:

> This command currently has no tests. Add some for basic assignment and the
> integer operations.
> 
> Note that the default size for setexpr is ulong, which varies depending on
> the build machine. So for sandbox on a 64-bit host, this means that the
> default size is 64 bits.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/ce806339/attachment.sig>

[PATCH 02/10] command: Add constants for cmd_get_data_size string / error

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:36PM -0700, Simon Glass wrote:

> At present these values are open-coded in a few places. Add constants so
> the meaning is clear.
> 
> Also add a comment to cmd_get_data_size()
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/452893ec/attachment.sig>

[PATCH 03/10] setexpr: Add explicit support for 32- and 64-bit ints

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:37PM -0700, Simon Glass wrote:

> At present this function assumes that a size of 4 refers to a ulong. This
> is true on 32-bit machines but not commonly on 64-bit machines.
> 
> This means that the 'l' specify does not work correctly with setexpr.
> 
> Add an explicit case for 32-bit values so that 64-bit machines can still
> use the 'l' specifier. On 32-bit machines, 64-bit is still not supported.
> 
> This corrects the operation of the default size (which is 4 for setexpr),
> so update the tests accordingly.
> 
> The original code for reading from memory was included in 47ab5ad1457
> ("cmd_setexpr: allow memory addresses in expressions") but I am not adding
> a Fixes: tag since that code was not written with 64-bit machines in mind.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/a9169ddc/attachment.sig>

[PATCH 04/10] test: Add some setexpr regex tests

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:38PM -0700, Simon Glass wrote:

> Add tests for the setexpr regex commands.
> 
> Note that these tests currently crash on sandbox due to an existing bug in
> the setexpr implementation, so two of the tests are commented out.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/8d53de26/attachment.sig>

[PATCH 05/10] setexpr: Split the core logic into its own function

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:39PM -0700, Simon Glass wrote:

> At present this function always allocates a large amount of stack, and
> selects its own size for buffers. This makes it hard to test the code
> for buffer overflow.
> 
> Separate out the inner logic of the substitution so that tests can call
> this directly. This will allow checking that the algorithm does not
> overflow the buffer.
> 
> Fix up one of the error lines at the same time, since it should be
> printing nbuf_size, not data_size.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/e1fdf8eb/attachment.sig>

[PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:40PM -0700, Simon Glass wrote:

> Add tests to check for buffer overflow using simple replacement as well
> as back references. At present these don't fully pass.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/4ec25f53/attachment.sig>

[PATCH 07/10] setexpr: Correct dropping of final unmatched string

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:41PM -0700, Simon Glass wrote:

> At present the 'nlen' variable increases with each loop. If the previous
> loop had back references, then subsequent loops without back references
> use the wrong value of nlen. The value is larger, meaning that the string
> terminator from nbuf is copied along to the main buffer, thus terminating
> the string prematurely.
> 
> This leads to the final result being truncated, e.g. missing the last
> (unmatched) part of the string. So "match match tail" become
> "replaced replaced" instead of "replaced replaced tail".
> 
> Fix this by resetting nlen to the correct value each time around the lop.
> 
> Fixes: 855f18ea0e6 ("setexpr: add regex substring matching and substitution")
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/a196e9b7/attachment.sig>

[PATCH 08/10] setexpr: Correct buffer overflow bug and enable tests

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:42PM -0700, Simon Glass wrote:

> At present when more than one substitution is made this function
> overwrites its buffers. Fix this bug and update the tests now that they
> can pass.
> 
> Also update the debug code to show all substrings, since at present it
> omits the final one.
> 
> Fixes: 855f18ea0e6 ("setexpr: add regex substring matching and substitution")
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/982f41ef/attachment.sig>

[PATCH 09/10] setexpr: Convert to use a struct for values

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:43PM -0700, Simon Glass wrote:

> At present a ulong is used to hold operand values. This means that
> strings cannot be used. While most operations are not useful for strings,
> concatenation is. As a starting point to supporting strings, convert the
> code to use a struct instead of a ulong for operands.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/bf525274/attachment.sig>

[PATCH 10/10] setexpr: Add support for strings

[Tom Rini]

On Sun, Nov 01, 2020 at 02:15:44PM -0700, Simon Glass wrote:

> Add support for dealing with string operands, including reading a string
> from memory into an environment variable and concatenating two strings.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Acked-by: Marek Beh?n <marek.behun@nic.cz>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/d851ac24/attachment.sig>

CRASH caused by: [PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Heinrich Schuchardt]

On 11/1/20 10:15 PM, Simon Glass wrote:
> Add tests to check for buffer overflow using simple replacement as well
> as back references. At present these don't fully pass.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   cmd/setexpr.c      | 21 +++--------
>   include/command.h  | 17 +++++++++
>   test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 110 insertions(+), 17 deletions(-)
>
> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
> index fe3435b4d99..dbb43b3be2f 100644
> --- a/cmd/setexpr.c
> +++ b/cmd/setexpr.c
> @@ -134,22 +134,8 @@ static char *substitute(char *string, int *slen, int ssize,
>   	return p + nlen;
>   }
>
> -/**
> - * regex_sub() - Replace a regex pattern with a string
> - *
> - * @data: Buffer containing the string to update
> - * @data_size: Size of buffer (must be large enough for the new string)
> - * @nbuf: Back-reference buffer
> - * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
> - *	all back-reference expansions)
> - * @r: Regular expression to find
> - * @s: String to replace with
> - * @global: true to replace all matches in @data, false to replace just the
> - *	first
> - * @return 0 if OK, 1 on error
> - */
> -static int regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> -		     const char *r, const char *s, bool global)
> +int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> +		      const char *r, const char *s, bool global)
>   {
>   	struct slre slre;
>   	char *datap = data;
> @@ -325,7 +311,8 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
>
>   	strcpy(data, t);
>
> -	ret = regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s, global);
> +	ret = setexpr_regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s,
> +				global);
>   	if (ret)
>   		return 1;
>
> diff --git a/include/command.h b/include/command.h
> index e900f97df33..e229bf2825c 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -183,6 +183,23 @@ extern int do_env_set_efi(struct cmd_tbl *cmdtp, int flag, int argc,
>   			  char *const argv[]);
>   #endif
>
> +/**
> + * setexpr_regex_sub() - Replace a regex pattern with a string
> + *
> + * @data: Buffer containing the string to update
> + * @data_size: Size of buffer (must be large enough for the new string)
> + * @nbuf: Back-reference buffer
> + * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
> + *	all back-reference expansions)
> + * @r: Regular expression to find
> + * @s: String to replace with
> + * @global: true to replace all matches in @data, false to replace just the
> + *	first
> + * @return 0 if OK, 1 on error
> + */
> +int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> +		      const char *r, const char *s, bool global);
> +
>   /*
>    * Error codes that commands return to cmd_process(). We use the standard 0
>    * and 1 for success and failure, but add one more case - failure with a
> diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
> index de54561917c..a6940fd82dd 100644
> --- a/test/cmd/setexpr.c
> +++ b/test/cmd/setexpr.c
> @@ -209,6 +209,95 @@ static int setexpr_test_regex_inc(struct unit_test_state *uts)
>   }
>   SETEXPR_TEST(setexpr_test_regex_inc, UT_TESTF_CONSOLE_REC);
>
> +/* Test setexpr_regex_sub() directly to check buffer usage */
> +static int setexpr_test_sub(struct unit_test_state *uts)
> +{
> +	char *buf, *nbuf;
> +	int i;
> +
> +	buf = map_sysmem(0, BUF_SIZE);
> +	nbuf = map_sysmem(0x1000, BUF_SIZE);
> +
> +	/* Add a pattern so we can check the buffer limits */
> +	memset(buf, '\xff', BUF_SIZE);
> +	memset(nbuf, '\xff', BUF_SIZE);
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		buf[i] = i & 0xff;
> +		nbuf[i] = i & 0xff;
> +	}
> +	strcpy(buf, "this is a test");
> +
> +	/*
> +	 * This is a regression test, since a bug was found in the use of
> +	 * memmove() in setexpr
> +	 */
> +	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE, "is",
> +				      "us it is longer", true));
> +	ut_asserteq_str("thus it is longer us it is longer a test", buf);
> +
> +	/* The following checks fail at present due to a bug in setexpr */
> +	return 0;
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		ut_assertf(buf[i] == (char)i,
> +			   "buf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)buf[i]);
> +		ut_assertf(nbuf[i] == (char)i,
> +			   "nbuf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)nbuf[i]);
> +	}
> +
> +	unmap_sysmem(buf);
> +
> +	return 0;
> +}
> +SETEXPR_TEST(setexpr_test_sub, UT_TESTF_CONSOLE_REC);
> +
> +/* Test setexpr_regex_sub() with back references */
> +static int setexpr_test_backref(struct unit_test_state *uts)
> +{
> +	char *buf, *nbuf;
> +	int i;
> +
> +	buf = map_sysmem(0, BUF_SIZE);
> +	nbuf = map_sysmem(0x1000, BUF_SIZE);

This test is compiled for all boards enabling CONFIG_UNIT_TEST and
CONFIG_CMDLINE.

On the sipeed_maix_smode_defconfig trying to access NULL results in a crash:

Test: setexpr_test_backref
Unhandled exception: Store/AMO access fault
EPC: 00000000805b1152 RA: 00000000805b3bce TVAL: 0000000000001000
EPC: 0000000080056152 RA: 0000000080058bce reloc adjusted

Why did you opt for hard-coded addresses instead of malloc()?

We should be able to run the C unit tests on all boards not only on the
sandbox except where sandbox drivers are involved.

Could you, please, provide a correction.

Best regards

Heinrich

> +
> +	/* Add a pattern so we can check the buffer limits */
> +	memset(buf, '\xff', BUF_SIZE);
> +	memset(nbuf, '\xff', BUF_SIZE);
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		buf[i] = i & 0xff;
> +		nbuf[i] = i & 0xff;
> +	}
> +	strcpy(buf, "this is surely a test is it? yes this is indeed a test");
> +
> +	/*
> +	 * This is a regression test, since a bug was found in the use of
> +	 * memmove() in setexpr
> +	 */
> +	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE,
> +				      "(this) (is) (surely|indeed)",
> +				      "us \\1 \\2 \\3!", true));
> +
> +	/* The following checks fail at present due to bugs in setexpr */
> +	return 0;
> +	ut_asserteq_str("us this is surely! a test is it? yes us this is indeed! a test",
> +			buf);
> +
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		ut_assertf(buf[i] == (char)i,
> +			   "buf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)buf[i]);
> +		ut_assertf(nbuf[i] == (char)i,
> +			   "nbuf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)nbuf[i]);
> +	}
> +
> +	unmap_sysmem(buf);
> +
> +	return 0;
> +}
> +SETEXPR_TEST(setexpr_test_backref, UT_TESTF_CONSOLE_REC);
> +
>   int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
>   	struct unit_test *tests = ll_entry_start(struct unit_test,
>

CRASH caused by: [PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Simon Glass]

Hi Heinrich,

On Sun, 17 Jan 2021 at 15:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/1/20 10:15 PM, Simon Glass wrote:
> > Add tests to check for buffer overflow using simple replacement as well
> > as back references. At present these don't fully pass.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >   cmd/setexpr.c      | 21 +++--------
> >   include/command.h  | 17 +++++++++
> >   test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 110 insertions(+), 17 deletions(-)

Yes this is intended for sandbox.

Do you really want to run this test on a board? If the compiler is
working, then the sandbox test should be enough.

I can certainly update it to run on boards, but that was not the intent.

Regards,
Simon

CRASH caused by: [PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Heinrich Schuchardt]

On 1/19/21 7:06 PM, Simon Glass wrote:
> Hi Heinrich,
>
> On Sun, 17 Jan 2021 at 15:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 11/1/20 10:15 PM, Simon Glass wrote:
>>> Add tests to check for buffer overflow using simple replacement as well
>>> as back references. At present these don't fully pass.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>>    cmd/setexpr.c      | 21 +++--------
>>>    include/command.h  | 17 +++++++++
>>>    test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 110 insertions(+), 17 deletions(-)
>
> Yes this is intended for sandbox.
>
> Do you really want to run this test on a board? If the compiler is
> working, then the sandbox test should be enough.
>
> I can certainly update it to run on boards, but that was not the intent.
>
> Regards,
> Simon

The idea of our Python test environment is that we can run it on
physical boards. Amongst others differences in bitness or endianness can
lead to test failures that occur on some boards but not on others. So
preferably tests should be executable on all boards. If this is not
reasonably possible, e.g. when using sandbox drivers, the test author
must ensure that no error occurs when building or testing on other boards.

CONFIG_CMD_SETEXPR is selected by default by
sipeed_maix_smode_defconfig. Hence we should allow testing the command
on the board.

Best regards

Heinrich

CRASH caused by: [PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Tom Rini]

On Tue, Jan 19, 2021 at 11:06:18AM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Sun, 17 Jan 2021 at 15:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 11/1/20 10:15 PM, Simon Glass wrote:
> > > Add tests to check for buffer overflow using simple replacement as well
> > > as back references. At present these don't fully pass.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > >   cmd/setexpr.c      | 21 +++--------
> > >   include/command.h  | 17 +++++++++
> > >   test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 110 insertions(+), 17 deletions(-)
> 
> Yes this is intended for sandbox.
> 
> Do you really want to run this test on a board? If the compiler is
> working, then the sandbox test should be enough.
> 
> I can certainly update it to run on boards, but that was not the intent.

I think it is good to run tests on hardware when there's no reason not
to.  It ends up adding to the "if we're up and doing stuff for a while,
is the platform stable?" form of testing, which is harder to replicate
but useful at times.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210119/29f4b206/attachment.sig>

CRASH caused by: [PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Simon Glass]

Hi Tom, Heinrich,

On Tue, 19 Jan 2021 at 12:45, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jan 19, 2021 at 11:06:18AM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Sun, 17 Jan 2021 at 15:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 11/1/20 10:15 PM, Simon Glass wrote:
> > > > Add tests to check for buffer overflow using simple replacement as well
> > > > as back references. At present these don't fully pass.
> > > >
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > >   cmd/setexpr.c      | 21 +++--------
> > > >   include/command.h  | 17 +++++++++
> > > >   test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >   3 files changed, 110 insertions(+), 17 deletions(-)
> >
> > Yes this is intended for sandbox.
> >
> > Do you really want to run this test on a board? If the compiler is
> > working, then the sandbox test should be enough.
> >
> > I can certainly update it to run on boards, but that was not the intent.
>
> I think it is good to run tests on hardware when there's no reason not
> to.  It ends up adding to the "if we're up and doing stuff for a while,
> is the platform stable?" form of testing, which is harder to replicate
> but useful at times.

Yes, more testing is good. One reason not to is that it adds to code
size, depending on the test type. But we can do a special build that
enables CONFIG_UNIT_TEST for a board, so that can be avoided.

Another reason is that if we do have a test failure in common code,
then having just one test that covers if means we see a single test
failure, which is easier to diagnose than a failure on 110 boards,
particularly if it is sandbox and so does not require real hardware.

Python tests can often be enabled without adding to code size, but
they are very slow. C tests require special enablement but we can run
1000s of them quickly. Maybe we should write up the trade-offs and
come up with a test policy.

Regards,
Simon