zsh-workers
 help / color / mirror / code / Atom feed
* ZSH crashed when reading bytes from a large binary
@ 2022-09-11 14:43 Liu, Song
  2022-09-12  7:36 ` Jun T
  0 siblings, 1 reply; 5+ messages in thread
From: Liu, Song @ 2022-09-11 14:43 UTC (permalink / raw)
  To: zsh-workers; +Cc: Hu, Hong

[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: ZSH crashed when reading bytes from a large binary
  2022-09-11 14:43 ZSH crashed when reading bytes from a large binary Liu, Song
@ 2022-09-12  7:36 ` Jun T
  2022-09-14 21:43   ` Bart Schaefer
  0 siblings, 1 reply; 5+ messages in thread
From: Jun T @ 2022-09-12  7:36 UTC (permalink / raw)
  To: zsh-workers


> 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()?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: ZSH crashed when reading bytes from a large binary
  2022-09-12  7:36 ` Jun T
@ 2022-09-14 21:43   ` Bart Schaefer
  2022-09-15 10:19     ` Jun T
  0 siblings, 1 reply; 5+ messages in thread
From: Bart Schaefer @ 2022-09-14 21:43 UTC (permalink / raw)
  To: Jun T; +Cc: zsh-workers

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: ZSH crashed when reading bytes from a large binary
  2022-09-14 21:43   ` Bart Schaefer
@ 2022-09-15 10:19     ` Jun T
  2022-09-15 10:41       ` Peter Stephenson
  0 siblings, 1 reply; 5+ messages in thread
From: Jun T @ 2022-09-15 10:19 UTC (permalink / raw)
  To: zsh-workers


> 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: ZSH crashed when reading bytes from a large binary
  2022-09-15 10:19     ` Jun T
@ 2022-09-15 10:41       ` Peter Stephenson
  0 siblings, 0 replies; 5+ messages in thread
From: Peter Stephenson @ 2022-09-15 10:41 UTC (permalink / raw)
  To: zsh-workers

> 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-09-15 10:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-11 14:43 ZSH crashed when reading bytes from a large binary Liu, Song
2022-09-12  7:36 ` Jun T
2022-09-14 21:43   ` Bart Schaefer
2022-09-15 10:19     ` Jun T
2022-09-15 10:41       ` Peter Stephenson

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).