[PATCH 00/10] setexpr: Correct various bugs and add tests plus string support
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
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
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!
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
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!
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);
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!
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,
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!
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" */
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!
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 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,
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!
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)@@ -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);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,
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
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
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.
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
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 at %x should be %02x, got %02x)\n",
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!
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 at %x should be %02x, got %02x)\n",
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!
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; }
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!
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,
What is the purpose of + operator on strings? Can't we use setenv "${a}${b}" ?
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
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} ...
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
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
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.
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
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
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
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
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
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
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!
participants (5)
-
Heinrich Schuchardt -
Marek Behun -
Simon Glass -
Tom Rini -
Wolfgang Denk