[-- Attachment #1: Type: text/plain, Size: 2882 bytes --] Dear maintainers: When ZSH reads bytes from binary, ZSH will adjust the allocated memory according to binary size. But if the binary size is large enough, ZSH will fail to reallocate memory. The problem is that, when ZSH failed to reallocate memory, ZSH didn’t check the return value of `realloc` function and handle the error. This will make ZSH crash. On line 6923 of `zsh/Src/builtin.c`, the correct code should like the following: ``` c If (buf = realloc(buf, bsiz *= 2)) { // same as previous code } else { free(buf); return EXIT_FAILURE; } ``` Environments: * Ubuntu 20.04.5 LTS x86_64 * Ubuntu clang version 10.0.1-++20211003084855+ef32c611aa21-1~exp1~20211003085243.2 * ZSH source code commit: eb738c793a6f9f293fc655c6aa87effc3dd9e44f (latest) Steps to Reproduce: ``` shell # clone and build zsh shell from source git clone git@github.com:zsh-users/zsh.git<mailto:git@github.com:zsh-users/zsh.git> cd zsh ./Util/preconfig # compile ZSH with Address Sanitizer CFLAGS=" -g -O0 " LDFLAGS=" -fsanitize=address " ../zsh/configure make -j$(nproc) # generate large binary. dd if=/dev/zero of=large.bin iflag=fullblock bs=1M count=600 # trigger segment fault. ./Src/zsh -c "read byte < ./large.bin" # Segment fault ``` ASAN log: ``` shell ================================================================= ==883996==ERROR: AddressSanitizer: requested allocation size 0xffffffff80000000 (0xffffffff80001000 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0) #0 0x7f3bfc697c3e in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:163 #1 0x55c82f1a6ad6 in bin_read /data/song/zsh-crash/zsh/Src/builtin.c:6923 #2 0x55c82f18d8bb in execbuiltin /data/song/zsh-crash/zsh/Src/builtin.c:506 #3 0x55c82f1b6dc1 in execcmd_exec /data/song/zsh-crash/zsh/Src/exec.c:4148 #4 0x55c82f1b06c6 in execpline2 /data/song/zsh-crash/zsh/Src/exec.c:1960 #5 0x55c82f1af35c in execpline /data/song/zsh-crash/zsh/Src/exec.c:1689 #6 0x55c82f1ae671 in execlist /data/song/zsh-crash/zsh/Src/exec.c:1444 #7 0x55c82f1adced in execode /data/song/zsh-crash/zsh/Src/exec.c:1221 #8 0x55c82f1adbb1 in execstring /data/song/zsh-crash/zsh/Src/exec.c:1187 #9 0x55c82f1d7432 in init_misc /data/song/zsh-crash/zsh/Src/init.c:1389 #10 0x55c82f1d89b6 in zsh_main /data/song/zsh-crash/zsh/Src/init.c:1780 #11 0x55c82f18c95c in main main.c:93 #12 0x7f3bfc1f1082 in __libc_start_main ../csu/libc-start.c:308 ==883996==HINT: if you don't care about these errors you may set allocator_may_return_null=1 SUMMARY: AddressSanitizer: allocation-size-too-big ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:163 in __interceptor_realloc ==883996==ABORTING ``` Sincerely, Song [-- Attachment #2: Type: text/html, Size: 11181 bytes --]
> 2022/09/11 23:43, Liu, Song <songliu@psu.edu> wrote:
>
> The problem is that, when ZSH failed to reallocate memory, ZSH didn’t check the return value of `realloc` function and handle the error. This will make ZSH crash.
>
> On line 6923 of `zsh/Src/builtin.c`, the correct code should like the following:
>
> ``` c
> If (buf = realloc(buf, bsiz *= 2)) {
> // same as previous code
> } else {
> free(buf);
> return EXIT_FAILURE;
> }
There are many other places where the return value of realloc() is not checked.
Can we simply replace them by zrealloc()?
On Mon, Sep 12, 2022 at 12:37 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: > > > 2022/09/11 23:43, Liu, Song <songliu@psu.edu> wrote: > > > > The problem is that, when ZSH failed to reallocate memory, ZSH didn’t check the return value of `realloc` function and handle the error. This will make ZSH crash. > > There are many other places where the return value of realloc() is not checked. > Can we simply replace them by zrealloc()? Replacing them with zrealloc() will still result in immediate exit of the shell, just with a different error message and no stack trace. > > } else { > > free(buf); > > return EXIT_FAILURE; > > } The specific question here is whether having the "read" builtin return failure at this point will allow the shell to recover from the out-of-memory state, or whether it's just delaying the inevitable death. (Aside, we use EXIT_FAILURE exactly nowhere, so if we do not exit, this should just be "return 1" as if end-of-file was encountered per docs.) In most cases we've assumed that once the shell is out of memory, there's no way back, so how it dies is not that interesting.
> 2022/09/15 6:43, Bart Schaefer <schaefer@brasslantern.com> wrote: > > On Mon, Sep 12, 2022 at 12:37 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote: >> >> Can we simply replace them by zrealloc()? > > Replacing them with zrealloc() will still result in immediate exit of > the shell, just with a different error message and no stack trace. On Linux zsh coredumps without any error message if realloc() is used. But getting core would make debug easier... > The specific question here is whether having the "read" builtin return > failure at this point will allow the shell to recover from the > out-of-memory state, or whether it's just delaying the inevitable > death. Consider a script something like: if read x < $file; then # do something else rm $file # it's empty and we can remove it fi Yes, this script is wrong since read returns 1 even if $file is not empty (if there is no \n). But I fear there may be some (only a few?) users using scripts like this.
> On 15/09/2022 11:19 Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
> Consider a script something like:
>
> if read x < $file; then
> # do something
> else
> rm $file # it's empty and we can remove it
> fi
>
> Yes, this script is wrong since read returns 1 even if $file is not empty
> (if there is no \n). But I fear there may be some (only a few?) users
> using scripts like this.
There is an argument that we might be able to put in enough checking without
a rewrite to catch some particularly nasty cases --- and reading something
not under your control certainly seems to qualify as one of those.
It's still tough to catch all the levels necessary for this, though, and
it's a marginal additional benefit rather than a fix. But I'd admire
anyone with the determination to have a go.
pws