Skip to content

cat /dev/zero | dd > /test breaks Fish #7038

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

Closed
jos128 opened this issue May 25, 2020 · 10 comments
Closed

cat /dev/zero | dd > /test breaks Fish #7038

jos128 opened this issue May 25, 2020 · 10 comments
Labels
bug Something that's not working as intended regression Something that used to work, but was broken, especially between releases
Milestone

Comments

@jos128
Copy link

jos128 commented May 25, 2020

Please tell us which fish version you are using...

user@host fish$> echo $version; echo $TERM; uname -vosr
3.1.0
xterm-256color
Linux 5.4.0-31-generic #35-Ubuntu SMP Thu May 7 20:20:34 UTC 2020 GNU/Linux

Edit:

Problem persists on Fish 3.1.2 installed via PPA.

Please tell us if you tried fish without third-party customizations...

Yes, the problem still persists with vanilla Fish.

Tell us how to reproduce the problem.

1. Gnome Terminal Bash as "base shell":

user@host bash$> fish
user@host fish$> cat /dev/zero | dd > /test
<W> fish: An error occurred while redirecting file '/test'
open: Permission denied
user@host fish$>
  • Fish becomes unresponsive; not recovering.

2. Gnome Terminal Fish as "base shell"

user@host fish$> cat /dev/zero | dd > /test
<W> fish: An error occurred while redirecting file '/test'
open: Permission denied
user@host fish$>
  • On keystroke the Gnome Terminal Window closes/crashes.

Notes

  • Bash seems to handle this.
user@host bash$> cat /dev/zero | dd > /test
bash: /test: Permission denied
user@host bash$>
user@host bash$>
  • The problem seem to be with the pipe. This works fine:
user@host fish$> cat /dev/zero > /test
<W> fish: An error occurred while redirecting file '/test'
open: Permission denied
user@host fish$>
  • This crashes the terminal window:
user@host fish$> cat /dev/zero | cat > /test
<W> fish: An error occurred while redirecting file '/test'
open: Permission denied
user@host fish$>
@faho
Copy link
Member

faho commented May 25, 2020 via email

@jos128
Copy link
Author

jos128 commented May 25, 2020

I upgraded via PPA, the problem unfortunately persists:

user@host fish$> echo $version
3.1.2
user@host fish$> cat /dev/zero | cat > /test
<W> fish: An error occurred while redirecting file '/test'
open: Permission denied
user@host fish$>
  • Gnome Terminal window crashes on keystroke.
  • If run as subshell in bash, it does not react to CTRL + C, but terminates on kill $PID with the PID of the cat <defunct> process or fish. Maybe that helps.

@zanchey
Copy link
Member

zanchey commented May 25, 2020

Yes, I can reproduce this on cbf5362.

@zanchey zanchey added this to the fish-future milestone May 25, 2020
@zanchey zanchey added the bug Something that's not working as intended label May 25, 2020
@zanchey
Copy link
Member

zanchey commented May 25, 2020

fish doesn't actually crash, it reads EOF from stdin and exits normally.

@krobelus
Copy link
Contributor

When running this in a fish inside another shell, it does not exit or react to Ctrl-C as you observed.
Fish keeps receiving SIGTTIN ("Background process attempting read.") so quickly that it maxes out one CPU.

@jos128
Copy link
Author

jos128 commented May 25, 2020

The frozen fish shell (in bash) goes 100% CPU on any keystroke. Also stays at 100% CPU usage.

@krobelus
Copy link
Contributor

Bisects to d62576c.

For some reason, after running cat /dev/null | cat > /test, fish is no longer the process group leader. Simple fix: make fish the process group leader after open fails. I'm not yet sure where this should go.

diff --git a/src/io.cpp b/src/io.cpp
index 081354b1b..4045beee7 100644
--- a/src/io.cpp
+++ b/src/io.cpp
@@ -254,2 +254,4 @@ bool io_chain_t::append_from_specs(const redirection_spec_list_t &specs, const w
                         if (should_flog(warning)) wperror(L"open");
+                        pid_t shell_pgid = getpgrp();
+                        tcsetpgrp(STDIN_FILENO, shell_pgid);
                     }

@krobelus krobelus added the regression Something that used to work, but was broken, especially between releases label May 25, 2020
@mqudsi
Copy link
Contributor

mqudsi commented May 26, 2020

Thanks for bisecting that, @krobelus.

Currently if setup_child_process fails, then all bets are off regarding both redirections and process groups because they may have been changed before the file descriptor was resolved (or, as in this case, failed to resolve). Reasserting ownership of the terminal input could be performed there so that it's not undetermined and that should propagate to all the other edge cases.

More correctly though, I think there needs to be some feedback mechanism for dup2_list_t that lets the code be more deterministic by clearly indicating at which point in the chain the failure occurred. The old io_chain_t approach was fully integrated in handle_child_io and I believe that prevented the "all bets are off" result.

I've mentioned this in passing before, but imho all the pgroup checks and sets that we have all over the codebase is something we should be refactoring to cut down on. Context switches between user and kernel modes are getting more and more expensive with each speculative execution mitigation on a monthly basis and they make reasoning about groups really hard.

@mqudsi mqudsi closed this as completed in bc756a9 May 30, 2020
@mqudsi
Copy link
Contributor

mqudsi commented May 30, 2020

I "fixed" this in bc756a9 but with the caveat that fish's handling of pipelines broken by failed job executions is now different. In particular, a bad redirection mid-pipeline results in the job continuing to run but with the broken fd replaced with a closed fd. This gives fish a chance to recover after having passed control of the terminal to another process.

To illustrate with an example, the original bug reported in this PR now exhibits the following behavior:

mqudsi@ZBOOK ~/r/fish-shell> cat /dev/zero | cat > /test
warning: An error occurred while redirecting file '/test'
open: Permission denied
cat: standard output: Bad file descriptor

(Personally, I think this is more correct than the previous model even setting the regression aside but I will bat-signal @ridiculousfish for his seal of approval. This does also eliminate an entire class of bugs resulting from aborted job cleanup.)

@krobelus krobelus modified the milestones: fish-future, fish 3.2.0 May 30, 2020
@jos128

This comment has been minimized.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended regression Something that used to work, but was broken, especially between releases
Projects
None yet
Development

No branches or pull requests

5 participants