Bug 14031 - Segmentation fault on ash when invoking quoted string substitution with long input
Summary: Segmentation fault on ash when invoking quoted string substitution with long ...
Status: NEW
Alias: None
Product: Busybox
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.33.x
Hardware: All Linux
: P5 minor
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-18 18:50 UTC by Manu
Modified: 2021-07-18 19:06 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:


Attachments
Potential patch for this bug (5.61 KB, application/octet-stream)
2021-07-18 18:50 UTC, Manu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Manu 2021-07-18 18:50:26 UTC
Created attachment 9066 [details]
Potential patch for this bug

# Summary

I've encountered a bug in the string substitution logic in shell/ash.c resulting
in segmentation fault. Run this, for example:

```shell
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo "${A///}"'
```

This bug is still present in the current master branch (as of dabbeeb7). It was
introduced in da2e46df (somewhere between 1.31.1 and 1.32.1). I'm also providing
a potential patch.

Besides that, I think it might be beneficial to use fuzzing to discover similar
bugs in the shell string substitution feature.

NOTE: Possibly a duplicate of https://bugs.busybox.net/show_bug.cgi?id=13556


# The bug

The bug is related to the shell string substitution logic implemented in
shell/ash.c. It's only seen in very particular circumstances, namely:
- Using musl
- Performing the substitution within quotes
- Having relatively long content in the variable being substituted
- Having special characters (like "*" or "?") in the content of the variable
  being substituted

## How to reproduce

I'm using busybox version 1.33.1 under Docker for illustrative purposes:

```shell
# Fails (musl + ? character + 2047 bytes + string substitution + quoted expansion)
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo "${A///}"'

# Works (shorter content)
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes ? | head -n 20 | tr -d "\n"); echo "${A///}"'

# Works (longer content)
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes ? | head -n 5000 | tr -d "\n"); echo "${A///}"'

# Works (unquoted expansion)
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo ${A///}'

# Works (different character)
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes x | head -n 2047 | tr -d "\n"); echo "${A///}"'

# Works (no string substitution)
docker run --rm -it busybox:1.33.1-musl -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo "${A}"'

# Works (glibc)
docker run --rm -it busybox:1.33.1-glibc sh -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo "${A///}"'

# Works (uclibc)
docker run --rm -it busybox:1.33.1-uclibc sh -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo "${A///}"'
```

It might be also interesting to run this, which shows how it segfaults only for
certain sizes (the first one being i=883):

```shell
for i in $(seq 1 2500); do echo $i;
  [[ $(./busybox_unstripped sh -c 'A=$(yes ? | head -n '$i' | tr -d "\n"); echo "${A///}"' | wc -c) -eq $(( i + 1)) ]] || echo "Failed";
done
```

## When was the bug introduced

The following `git bisect` operation

```shell
git checkout dabbeeb7; git bisect start; git bisect bad; git bisect good 1_31_1; git bisect run ../bisect.sh
```

using this bisect.sh script as test for the bug

```shell
#/bin/sh
set -e
make clean distclean > /dev/null
yes "" | make oldconfig > /dev/null
make CC=musl-gcc -j12 > /dev/null 2>&1
set +e
./busybox_unstripped sh -c 'A=$(yes ? | head -n 2047 | tr -d "\n"); echo "${A///}"' > /dev/null
[ $? -eq 0 ]
```

yields that the bug exists since commit da2e46df.


# The potential patch

See attachment.

I say *potential* because, even though it seems to fix the issue indeed, I don't
know enough about the implementation to be confident that this is an appropriate
fix.

I'm adding a test case mostly for illustrative purposes as well, but it feels
like it might be better tested with fuzzing and not with a concrete input.
Comment 1 Manu 2021-07-18 19:06:50 UTC
(In reply to Manu from comment #0)
NOTE, there is a typo on all docker run examples: `sh` is missing before `-c`.