| Message ID | 5be806a5.4FBQv2BWNLonvLir%rmy@frippery.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | parser: preserve characters on heap in backquote parsing | expand |
Ron Yorston <rmy@frippery.org> wrote: > This bug report for BusyBox ash also applies to dash: > > https://bugs.busybox.net/show_bug.cgi?id=9246 > > With an 8MB stack the test case results in a segfault. > > Instead of using alloca() to preserve characters keep them on the > memalloc stack. With this change the test case returns: > > $ dash test_case > test_case: 3141: test_case: Syntax error: Unterminated quoted string > > If the heap is reduced to the same size as the stack, 8MB: > > $ ulimit -S -d 8192 > $ dash test_case > test_case: 0: test_case: Out of space > > Signed-off-by: Ron Yorston <rmy@pobox.com> Sorry, but this is not the only place where dash relies on alloca. So you're bound to run into this problem again if you have a script that attempts to push dash to use more than 8MB in places like this. So I'm not going to accept this patch as alloca makes things a lot simpler in some cases. Thanks,
Herbert Xu wrote: >Sorry, but this is not the only place where dash relies on alloca. >So you're bound to run into this problem again if you have a script >that attempts to push dash to use more than 8MB in places like this. Sure, but the same problem existing in other places isn't a reason not to fix it here. >So I'm not going to accept this patch as alloca makes things a lot >simpler in some cases. Granted, I recall the simplification of the code that resulted from the replacement of malloc by alloca in this case. This patch retains that simplicity. Ron
Ron Yorston <rmy@frippery.org> wrote: > > Sure, but the same problem existing in other places isn't a reason not > to fix it here. If this were a real script then I might agree with you. But given that this came from a deliberately crafted script to breach the 8MB size limit, it would be pointless to fix it in just one spot because you could easily craft another script to exceed the size limit in any of the other places where we use alloca. Cheers,
diff --git a/src/parser.c b/src/parser.c index c4e6378..6efa8e2 100644 --- a/src/parser.c +++ b/src/parser.c @@ -1373,8 +1373,7 @@ parsebackq: { str = NULL; savelen = out - (char *)stackblock(); if (savelen > 0) { - str = alloca(savelen); - memcpy(str, stackblock(), savelen); + str = grabstackstr(out); } if (oldstyle) { /* We must read until the closing backquote, giving special
This bug report for BusyBox ash also applies to dash: https://bugs.busybox.net/show_bug.cgi?id=9246 With an 8MB stack the test case results in a segfault. Instead of using alloca() to preserve characters keep them on the memalloc stack. With this change the test case returns: $ dash test_case test_case: 3141: test_case: Syntax error: Unterminated quoted string If the heap is reduced to the same size as the stack, 8MB: $ ulimit -S -d 8192 $ dash test_case test_case: 0: test_case: Out of space Signed-off-by: Ron Yorston <rmy@pobox.com> --- src/parser.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)