8000 Fix crash when running ELF w/ interpreter missing by ksyx · Pull Request #10199 · fish-shell/fish-shell · GitHub
[go: up one dir, main page]
Skip to content

Fix crash when running ELF w/ interpreter missing#10199

Merged
faho merged 1 commit intofish-shell:Integration_3.7.0from
ksyx:interpreter-patch
Jan 9, 2024
Merged

Fix crash when running ELF w/ interpreter missing#10199
faho merged 1 commit intofish-shell:Integration_3.7.0from
ksyx:interpreter-patch

Conversation

@ksyx
Copy link
Contributor
@ksyx ksyx commented Jan 9, 2024 •
edited
Loading

Description

The function stat as defined in include/x86_64-linux-gnu/sys/stat.h marks its arguments as nonnull as in below. This UB causes crash in release builds with variable interpreter assumed to be nonnull. Along with failing stat returning nonzero value, this ultimately causes strlen to be called with NULL as argument. This PR fixes the problem by prefixing stat call with a check to avoid calling it with NULL argument.

Definition of stat:

extern int stat (const char *__restrict __file,
		 struct stat *__restrict __buf) __THROW __nonnull ((1, 2));

Reproduce:

> # interp.c is any vaild single file C source
> gcc ./interp.c -Wl,--dynamic-linker=/bad -o interp
> echo './interp' > in.txt
> ./fish < in.txt
'./fish < in.txt' terminated by signal SIGSEGV (Address boundary error)

Problem identified and fixed together with @moodyhunter

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
    • No change to usage
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst
    • No user-visible change

@ksyx @moodyhunter
fix: crash when running ELF w/ interpreter missing
9f2b584 
The function `stat` as defined in `include/x86_64-linux-gnu/sys/stat.h`
marks its arguments as nonnull as in below. This UB causes crash in
release builds with variable `interpreter` assumed to be nonnull. Along
with failing stat returning nonzero value, this ultimately causes
`strlen` to be called with NULL as argument.

Definition of `stat`:
```
extern int stat (const char *__restrict __file,
		 struct stat *__restrict __buf) __THROW __nonnull ((1, 2));
```
Reproduce:
```
> # interp.c is any vaild single file C source
> gcc ./interp.c -Wl,--dynamic-linker=/bad -o interp
> echo './interp' > in.txt
> ./fish < in.txt
'./fish < in.txt' terminated by signal SIGSEGV (Address boundary error)
```

Co-authored-by: Moody Liu <mooodyhunter@outlook.com>
@zanchey
Copy link
Member
zanchey commented Jan 9, 2024

I can't reproduce this crash with 3.6.0, 3.7.0 or master on Linux. What environment are you using?

(I get the expected exec: Failed to execute process './interp': The file exists and is executable. Check the interpreter or linker?)

(I probably wouldn't roll a 3.7.1 for this fix alone.)

@moodyhunter
Copy link
Contributor
moodyhunter commented Jan 9, 2024 •
edited
Loading

Hi @zanchey, it is an Arch Linux build with GCC 13

The build script is at
https://gitlab.archlinux.org/archlinux/packaging/packages/fish/-/blob/main/PKGBUILD?ref_type=heads#L32

> gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/13.2.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --enable-languages=ada,c,c++,d,fortran,go,lto,objc,obj-c++ --enable-bootstrap --prefix=/usr --libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/ --with-build-config=bootstrap-lto --with-linker-hash-style=gnu --with-system-zlib --enable-__cxa_atexit --enable-cet=auto --enable-checking=release --enable-clocale=gnu --enable-default-pie --enable-default-ssp --enable-gnu-indirect-function --enable-gnu-unique-object --enable-libstdcxx-backtrace --enable-link-serialization=1 --enable-linker-build-id --enable-lto --enable-multilib --enable-plugin --enable-shared --enable-threads=posix --disable-libssp --disable-libstdcxx-pch --disable-werror
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.1 20230801 (GCC)

Passing NULL to stat itself is already an undefined behavior, regardless of whether fish crashes or not :)

@faho faho merged commit 001f797 into fish-shell:Integration_3.7.0 Jan 9, 2024
@faho
Copy link
< 8000 span data-view-component="true" class="Label ml-1">Member
faho commented Jan 9, 2024 •
edited
Loading

I can reproduce this and I don't see any reason not to merge it.

We already have #10187 on Integration_3.7.0, so if we do end up making another C++-release this'll be included. Thanks!

(to be clear: What crashes is the "postfork" fish, so the outer fish sees a segfault. it's not a huge deal, just a misleading error)

@faho faho added the bug Something that's not working as intended label Jan 9, 2024
@faho faho added this to the fish 3.7.1 milestone Jan 9, 2024
@faho faho mentioned this pull request Jan 26, 2024
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something that's not working as intended

Projects

None yet

Milestone

fish 3.7.1

Development

Successfully merging this pull request may close these issues.

4 participants

@ksyx @zanchey @moodyhunter @faho
0