Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when running ELF w/ interpreter missing #10199

Merged
merged 1 commit into from Jan 9, 2024

Conversation

ksyx
Copy link
Contributor

@ksyx ksyx commented Jan 9, 2024

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

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

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
5 of 6 checks passed
@faho
Copy link
Member

faho commented Jan 9, 2024

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants