| Message ID | 20201101211544.3579850-7-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | d422c77ae8e0cb1211b34eb4af442600b0da8d5b |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | setexpr: Correct various bugs and add tests plus string support | expand |
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) > { > 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, >
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
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,
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(-)