[PATCH] ash: preserve characters on heap in backquote parsing

[Ron Yorston]

This bug:

https://bugs.busybox.net/show_bug.cgi?id=9246

demonstrates a test case where BusyBox ash segfaults due to a stack
overflow.

Instead of using alloca() to preserve characters keep them on the
shell's heap-based stack. With this change the test case returns:

$ busybox sh test_case
test_case: line 3141: syntax error: unterminated quoted string

That is, the test runs to completion. If the heap is reduced to 8MB:

$ ulimit -S -d 8192
$ busybox sh test_case
sh: out of memory

Heap exhaustion is detected and the shell continues to run.

function old new delta
readtoken1 3265 3203 -62
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-62) Total: -62 bytes

Signed-off-by: Ron Yorston <***@pobox.com>
---
shell/ash.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/shell/ash.c b/shell/ash.c
index 90eaf6faf..5ee36db93 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -12599,18 +12599,8 @@ parsebackq: {

str = NULL;
savelen = out - (char *)stackblock();
- if (savelen > 0) {
- /*
- * FIXME: this can allocate very large block on stack and SEGV.
- * Example:
- * echo "..<100kbytes>..`true` $(true) `true` ..."
- * allocates 100kb for every command subst. With about
- * a hundred command substitutions stack overflows.
- * With larger prepended string, SEGV happens sooner.
- */
- str = alloca(savelen);
- memcpy(str, stackblock(), savelen);
- }
+ if (savelen > 0)
+ str = grabstackstr(out);

if (oldstyle) {
/* We must read until the closing backquote, giving special

[Denys Vlasenko]

Hi Ron,

As usual, since those years back when dash resumed (semi-)active
maintenance of ash codebase, I would like to be easily
pick up fixes from them.

In order to do that, it would be very useful to keep codebase
from diverging further than it has already diverged.

For example, bbox ash had a fix for nested override envvars.
When dash also fixed the same bug, in a different way,
I had to spend time ripping out my fix, then porting theirs.
PITA.

The location you are touching here is the same in dash.
Looks like it needs to be fixed there as well.
Can you submit this fix to dash? Then I'll port it from there,
and code will stay in sync.

Post by Ron Yorston
https://bugs.busybox.net/show_bug.cgi?id=9246
demonstrates a test case where BusyBox ash segfaults due to a stack
overflow.
Instead of using alloca() to preserve characters keep them on the
$ busybox sh test_case
test_case: line 3141: syntax error: unterminated quoted string
$ ulimit -S -d 8192
$ busybox sh test_case
sh: out of memory
Heap exhaustion is detected and the shell continues to run.
function old new delta
readtoken1 3265 3203 -62
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-62) Total: -62 bytes
---
shell/ash.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/shell/ash.c b/shell/ash.c
index 90eaf6faf..5ee36db93 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -12599,18 +12599,8 @@ parsebackq: {
str = NULL;
savelen = out - (char *)stackblock();
- if (savelen > 0) {
- /*
- * FIXME: this can allocate very large block on stack and SEGV.
- * echo "..<100kbytes>..`true` $(true) `true` ..."
- * allocates 100kb for every command subst. With about
- * a hundred command substitutions stack overflows.
- * With larger prepended string, SEGV happens sooner.
- */
- str = alloca(savelen);
- memcpy(str, stackblock(), savelen);
- }
+ if (savelen > 0)
+ str = grabstackstr(out);
if (oldstyle) {
/* We must read until the closing backquote, giving special
--
2.19.1
_______________________________________________
busybox mailing list
http://lists.busybox.net/mailman/listinfo/busybox

[Ron Yorston]

Post by Denys Vlasenko
The location you are touching here is the same in dash.
Looks like it needs to be fixed there as well.
Can you submit this fix to dash? Then I'll port it from there,
and code will stay in sync.

I submitted the same fix to the dash mailing list shortly
after sending it here. No response yet.

Ron

[Ron Yorston]

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.

Ron