Skip to content

Allow scripts without #!#7802

Closed
jart wants to merge 1 commit intofish-shell:masterfrom
jart:sixth-edition
Closed

Allow scripts without #!#7802
jart wants to merge 1 commit intofish-shell:masterfrom
jart:sixth-edition

Conversation

@jart
Copy link
Copy Markdown
Contributor

@jart jart commented Mar 9, 2021

This change modifies the fish safety check surrounding execve / spawn so
it can run shell scripts having concatenated binary content. We're using
the same safety check as FreeBSD /bin/sh [1] and the Z-shell [5]. POSIX
was recently revised to require this behavior:

"The input file may be of any type, but the initial portion of the
 file intended to be parsed according to the shell grammar (XREF to
 XSH 2.10.2 Shell Grammar Rules) shall consist of characters and
 shall not contain the NUL character. The shell shall not enforce
 any line length limits."

"Earlier versions of this standard required that input files to the
 shell be text files except that line lengths were unlimited.
 However, that was overly restrictive in relation to the fact that
 shells can parse a script without a trailing newline, and in
 relation to a common practice of concatenating a shell script
 ending with an 'exit' or 'exec $command' with a binary data payload
 to form a single-file self-extracting archive." [2] [3]

One example use case of such scripts, is the Cosmopolitan C Library [4]
which configuse the GNU Linker to output a polyglot shell+binary format
that runs on Linux / Mac / Windows / FreeBSD / OpenBSD / NetBSD / BIOS.

Fixes jart/cosmopolitan#88

[1] freebsd/freebsd-src@9a1cd36
[2] http://austingroupbugs.net/view.php?id=1250
[3] http://austingroupbugs.net/view.php?id=1226#c4394
[4] https://justine.lol/cosmopolitan/index.html
[5] zsh-users/zsh@326d9c2

Description

Talk about your changes here.

Fixes issue #

TODOs:

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

@faho
Copy link
Copy Markdown
Member

faho commented Mar 9, 2021

The title here isn't technically incorrect, but it's misleading.

This allows "more" scripts without shebang by allowing basically any scripts at all without a shebang (the current behavior seems to only be a windows thing and I've never actually encountered it and didn't know we had it).

This is a POSIX legacy misfeature we've long rejected (and this new version of it makes it even more awkward by adding a weird heuristic on top)

The reason we've rejected it is that, until now, when a script doesn't have a shebang it's because the author simply forgot to add one, and it's not at all clear that it's actually a /bin/sh script. So instead of printing a sensible error, you'd now print a non-sensible one because you've tried to run a python script with /bin/sh.

Annoyingly, cosmopolitan might turn this around by making it more likely that it's actually meant to be without a shebang, and for the record I hate it. I absolutely, viscerally, despise this hack. It's impressively clever and I hate it. (feel free to add that as a testimonial on your website)

We're using
the same safety check as FreeBSD /bin/sh [1] and the Z-shell [5]. POSIX
was recently revised to require this behavior:

Note that we do not, in general, care about compatibility or especially POSIX conformance. Fish is not a POSIX conformant shell, and that is on purpose.

Yet at the same time I feel that this particular bit is no longer a hill worth dying on. Is having a script without a shebang a sensible thing to do? No, because it makes the failure mode of forgetting a shebang worse. Simply adding a shebang always would be better. Is it worth it to keep failing in this case when things keep abusing scripts without shebangs? Probably not. We should probably merge this, for 3.3.

@faho faho added this to the fish 3.3.0 milestone Mar 9, 2021
@jart
Copy link
Copy Markdown
Contributor Author

jart commented Mar 9, 2021

This change aims to offer you the best possible recommendation that's the result of an effort to balance the concerns of many groups. Shebangless scripts are part of the history of UNIX, e.g. Portable C Compiler and /bin/true which was designed to be an empty file. Safety checks until now have generally sought to avoid accidentally running something like a PNG file since that can corrupt the terminal driver state. This change fixes that, while upholding compatibility and remaining agnostic to style.

I want to be able to tell my users that Fish is on the recommended shell list, because its authors were flexible enough to include us in their vision. Zsh and FreeBSD have helped us so far, and it's generated a lot of positive sentiment. I hope we can do the same here!

@faho
Copy link
Copy Markdown
Member

faho commented Mar 9, 2021

This change aims to offer you the best possible recommendation that's the result of an effort to balance the concerns of many groups.

Just out of curiosity, do I understand the heuristic thinks this is a shellscript:

import os
os.sleep(100)

and this is a shellscript:

int main() {
    return 129;
}

but this isn't:

ECHO FOO

@faho
Copy link
Copy Markdown
Member

faho commented Mar 9, 2021

To be clear, let me repeat my opinion:

We should probably merge this, for 3.3.

I just hate it, that's all.

@jart
Copy link
Copy Markdown
Contributor Author

jart commented Mar 9, 2021

It's great to hear you intend to merge!

The heuristic only applies if there's a NUL character in the first 256 bytes. So echo 'ECHO FOO' >script is a valid script. On the hand, printf 'ECHO HI\0' >script would not be a valid script. We check for lowercase because ASCII magic for binary files is usually uppercase, e.g. PNG, JFIF, etc.

@faho
Copy link
Copy Markdown
Member

faho commented Mar 9, 2021

We check for lowercase because ASCII magic for binary files is usually uppercase, e.g. PNG, JFIF, etc.

That sounds like a good idea for a comment, to be honest.

This change modifies the fish safety check surrounding execve / spawn so
it can run shell scripts having concatenated binary content. We're using
the same safety check as FreeBSD /bin/sh [1] and the Z-shell [5].  POSIX
was recently revised to require this behavior:

    "The input file may be of any type, but the initial portion of the
     file intended to be parsed according to the shell grammar (XREF to
     XSH 2.10.2 Shell Grammar Rules) shall consist of characters and
     shall not contain the NUL character. The shell shall not enforce
     any line length limits."

    "Earlier versions of this standard required that input files to the
     shell be text files except that line lengths were unlimited.
     However, that was overly restrictive in relation to the fact that
     shells can parse a script without a trailing newline, and in
     relation to a common practice of concatenating a shell script
     ending with an 'exit' or 'exec $command' with a binary data payload
     to form a single-file self-extracting archive." [2] [3]

One example use case of such scripts, is the Cosmopolitan C Library [4]
which configuse the GNU Linker to output a polyglot shell+binary format
that runs on Linux / Mac / Windows / FreeBSD / OpenBSD / NetBSD / BIOS.

Fixes jart/cosmopolitan#88

[1] freebsd/freebsd-src@9a1cd36
[2] http://austingroupbugs.net/view.php?id=1250
[3] http://austingroupbugs.net/view.php?id=1226#c4394
[4] https://justine.lol/cosmopolitan/index.html
[5] zsh-users/zsh@326d9c2
@jart
Copy link
Copy Markdown
Contributor Author

jart commented Mar 9, 2021

Comment added, per your request. Please take a look.

@faho faho changed the title Allow more scripts without #! Allow scripts without #! Mar 9, 2021
@faho
Copy link
Copy Markdown
Member

faho commented Mar 9, 2021

That works for me. I'll give the other contributors (@ridiculousfish, @mqudsi might have opinions) a chance to object, then we'll probably want to branch off 3.2.1 before this (unless everyone's okay with adding this in a bugfix release).

@jart
Copy link
Copy Markdown
Contributor Author

jart commented Mar 9, 2021

Test failure looks like a flake. I only changed the comment.

@faho
Copy link
Copy Markdown
Member

faho commented Mar 9, 2021

Yeaaah it's the wonderful world of hosted CI.

Github Actions seems to have been under a bit of load again (since some runs haven't started for a while). This tends to freak out our tests, especially since there are some that need to be completed in a certain time - the ones that test that setting the escape timeout works, for instance.

I restarted it and it succeeded, so I wouldn't worry about it.

@zanchey
Copy link
Copy Markdown
Member

zanchey commented Mar 11, 2021

Cosmopolitan is wild and I love it! I think this is good, and fixes #4946, #1281 (comment), #2583 also related.

@faho
Copy link
Copy Markdown
Member

faho commented Mar 11, 2021

Important: #2583 is about a fish script without a shebang. This will make that case slightly worse, because it will try to run it with /bin/sh, which will probably succeed for a part of it - dash runs up to the first syntax error.

This means:

echo before
set path /banana
rm -rf $path/*
if true
    echo in
end
echo after

will print "before" and run rm -rf /* (because the set does something else and so $path is empty) and then print a syntax error.

@zanchey
Copy link
Copy Markdown
Member

zanchey commented Mar 11, 2021

The advice there should be all-shebang, all the time, then.

Comment thread src/exec.cpp
// after performing a binary safety check, recommended by POSIX: a
// line needs to exist before the first \0 with a lowercase letter
if (err == ENOEXEC && is_thompson_shell_script(actual_cmd)) {
*--argv = const_cast<char *>(_PATH_BSHELL);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing fast and loose with constness here is fine but it’s not clear to me that this write is to owned memory?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just replace the check in the old code with the thompson check and possibly change /bin/sh to _PATH_BSHELL (if that matters)?

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Mar 11, 2021

Cosmopolitan is wild and I love it! I think this is good, and fixes #4961, #1281 (comment), #2583 also related.

@zanchey is 4961 a typo? It doesn’t seem related?

@zanchey
Copy link
Copy Markdown
Member

zanchey commented Mar 11, 2021

Cosmopolitan is wild and I love it! I think this is good, and fixes #4961, #1281 (comment), #2583 also related.

@zanchey is 4961 a typo? It doesn’t seem related?

Fixed, ta

ridiculousfish added a commit that referenced this pull request Mar 27, 2021
This adds a test for shebangless support from #7802, cleans up some of
its tricks, and includes it in the changelog.
@ridiculousfish
Copy link
Copy Markdown
Member

Merged as 0048730, added a test and replaced the _PATH_BSHELL trickery in eb71e45. Thank you!

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Mar 28, 2021

I would actually like to change this to add an exception to refuse execution of .fish files without a shebang as a) we are not posix compatible anyway, b) it is almost certainly an error to execute them with sh.

@ridiculousfish
Copy link
Copy Markdown
Member

ridiculousfish commented Mar 28, 2021

Great idea, I will add the exception.

edit added in 694e112

@mqudsi
Copy link
Copy Markdown
Contributor

mqudsi commented Mar 28, 2021

Thanks. The case @faho mentioned has been gnawing at me.

ridiculousfish added a commit that referenced this pull request Mar 28, 2021
This expands the heuristic introduced in #7802 to prevent implicitly
passing files ending in .fish to /bin/sh.
@jart
Copy link
Copy Markdown
Contributor Author

jart commented Mar 28, 2021

Thank you!

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this pull request May 31, 2021
This concerns the behavior of posix_spawn for shebangless scripts. At some
point, glibc started executing them using `sh`, which is desirable for
fish's shebangless support (see fish-shell#7802). On glibcs without that behavior
the shebangless test fails. So this change disables posix_spawn on older
glibcs.

It's not easy to figure out when that happened but it definitely happens
in glibc 2.28, and does not happen in glibc 2.17. Presumably the new
behavior is present in glibc 2.24 (see BZ#23264) so that's the cutoff:
posix_spawn is no longer allowed on glibc < 2.24.

This fixes the noshebang test failures on Ubuntu Xenial and Centos 7.
See discussion at bottom of fish-shell#8021.
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 24, 2021
@fish-shell fish-shell unlocked this conversation Nov 2, 2021
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fish shell can't launch APE binaries

5 participants