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

Fish breaks flow control in child processes #2315

Closed
mwm opened this issue Aug 18, 2015 · 47 comments
Closed

Fish breaks flow control in child processes #2315

mwm opened this issue Aug 18, 2015 · 47 comments

Comments

@mwm
Copy link
Contributor

mwm commented Aug 18, 2015

If I start a command and then decide there's to much output and try and stop it with C-S, it doesn't stop. This works properly in most other shells.

Seems that fish purposely turns off flow control to make screen users with odd configurations happy. See issue #814: #814 for more info. I will say that being able to pause the output has nothing to do with vt100s, and everything to do with wanting to scroll through more output than your terminal emulator has lines configured.

My take is that the solution should be to make terminal_mode_for_executing_programs be set to the terminal mode after each command exits, rather than being whatever it was when the shell started with some random set of tweaks added. This would make stty work properly, so users could set it themselves.

Downside would be that if some command screws up the tty settings, it would propagate to future commands (but not suspended jobs). Just like it did back when I still used screen. But it also means that they can fix it with a simple "stty sane" (at least, they can on my Unix boxes).

I'm looking into creating a pull request.

@krader1961
Copy link
Contributor

See also issue #1041 which correctly points out another problem with the current approach to handling tty modes: A user can't change things like the character that generates a SIGINT.

@krader1961
Copy link
Contributor

I'll start reviewing the pull request associated with this issue. But before I do I want to make it clear that any change to fish's behavior needs to address multiple problems with the current implementation. Fish needs to not only honor changes to the tty modes made by stty when it runs a subsequent external command fish also needs to honor those changes when it becomes the foreground process. Consider the case noted in issue #1041 about not being able to change the interrupt key.

On the other hand, fish should protect itself, and future commands, against programs that exit without restoring the tty modes to sane values (or someone who uses the stty command to set non-sane modes). Fish should filter the current tty modes when it returns to the foreground by internally doing the equivalent of stty sane on the current tty modes.

@fpcomplete-support
Copy link

I disagree about needing to "protect the user". You're assuming you know the users needs better than they do, which isn't particularly user friendly, and definitely limits expressiveness. That Unix gave users all the rope they wanted whether or not that was enough to hang themselves is a feature, not a bug.

Worse yet, you're risking making things that should be simple - let the user and their programs set the tty modes to whatever they feel they need when fish isn't in control - to incredibly complicated. "stty sane" may not be the right way to fix the thing that is wrong. What if some program is suspended by an external act with odd modes that it needs? Do you handle that case differently? Do you need to keep track of the tty mode set by every running program, and restore it properly when you foreground the program?

Other shells don't seem to generate multiple bugs around tty handling, probably because they don't take it upon themselves to keep the user from doing what they want.

@krader1961
Copy link
Contributor

Those other shells require the user to recognize that they need to type stty sane\cJ to restore the terminal to a sane mode that allows the use of the [enter] key for issuing commands to the shell. I'm a gray-beard. I've been programming since 1976 (on a Teletype Model 33 terminal) and using UNIX since the mid 1980's. Just because you and I are accustomed to dealing with this issue doesn't mean we should continue to inflict that pain on younger users.

Your other point regarding the handling of tty modes for programs suspended by a SIGSTOP signal (such as by the tty mode dsusp character, typically \cZ) is a good one. That definitely needs to be handled intelligently if fish is going to override tty modes. The question is whether fish can improve on the legacy shell experience in this regard. If fish cannot improve the experience without removing useful functionality then I agree with your implicit suggestion that fish stop trying to improve this situation.

@krader1961
Copy link
Contributor

The pull request as it exists is incomplete even given the very narrow scope of the initial problem statement. I'm okay with keeping the scope narrow and addressing the other problems I raised in a different issue. But only if the scope of this issue is increased to include a mechanism for ensuring the tty modes read in term_steal are sane. If we're going to propagate the modes as they exist when an external process exits we need to ensure they're sane for the next external command.

ridiculousfish pushed a commit that referenced this issue Dec 27, 2015
My PR #2578 had the unexpected side-effect of altering the tty modes of
commands run via "fish -c command" or "fish scriptname". This change fixes
that; albeit incompletely. The correct solution is to unconditionally set
shell tty modes if stdin is attached to a tty and restore the appropriate
modes whenever an external command is run -- regardless of the path used to
run the external command. The proper fix should be done as part of addressing
issues #2315 and #1041.

Resolves issue #2619
@floam floam added the bug Something that's not working as intended label Nov 28, 2016
@faho faho added enhancement RFC and removed bug Something that's not working as intended labels Dec 14, 2016
@trulyliu
Copy link

I need CTRL-S and CTRL-Q flow control feature.
Any way to bring it back?

@mwm
Copy link
Contributor Author

mwm commented Dec 15, 2016

Not with the existing code. I submitted a pull request in #2317 that lets you turn them on and off with stty like any rational user expects. Not clear it works against the current build, because this fix is more important to me than anything that's gone into fish since, so I haven't bothered trying to update fish.

@ridiculousfish
Copy link
Member

Flow control is a feature that's hard to love. Most uses are accidental (this is pretty popular!) and those keys could be better put to use elsewhere. Having flow control on by default is nuts.

Still it's very reasonable that fish should allow the user to modify the tty modes deliberately. But cat < nonsense.bin leading to busted ttys is a real issue too. I think our options are, from least to most permissive:

  1. Stomp the tty modes always (current behavior)
  2. Stomp the tty modes, except special case stty
  3. Set the tty modes at startup, and try to sanitize them after every command
  4. Set the tty modes at startup, but then leave them alone thereafter (bash-like behavior)

I don't have any opinion yet - thoughts?

@krader1961
Copy link
Contributor

I recommend option 3 with an exception for the flow-control bit. That is, reset the tty modes to sane values after running an external command but retain the flow-control setting set by that command.

If the user explicitly reenables flow control by running stty ixon we should honor that. I'm not aware of any non-broken program which deliberately changes that setting. The primary question is whether we want to protect fish users against running a broken program which inadvertently changes the flow-control setting. Doing so would require special casing the stty command which bothers me.

@mwm
Copy link
Contributor Author

mwm commented Dec 15, 2016

Maybe the behavior should depend on the system behavior. On systems that do what they think is best for the user (windows, Mac, redhat/debian and probably other Linux distros) always sanitize the tty modes. On systems that do what the user wants (bsd, Solaris, and possibly some Linux distros) leave the tty modes alone.

That said, stty is just an expensive no-op with the current behavior. Making it a built-in in order to special case is doesn't seem unreasonable. Or if the special case really bothers you, add a built-in that saves the tty modes after running the user command that's the rest of the line, then add stty as a function that runs the stty binary under the control of that built-in.

@zanchey
Copy link
Member

zanchey commented Dec 15, 2016

I think some form of option 3 is the way to go; alternatively, save and restore the terminal mode unaltered after and before every command so that the shell isn't affected by stty changes but other commands are? I don't think there's a perfect answer here.

@faho
Copy link
Member

faho commented Dec 15, 2016

There are more options, among them a variable to enable flow control - set -g fish_enable_flow_control 1. That of course doesn't scale well when we want to allow other terminal bits to be changed as well, but it's nice and explicit when it's only about this one setting.

@ridiculousfish
Copy link
Member

Option 2 sounds more appealing if it's making stty a builtin (wrapping the command), and not some special case in the execution path. Option 3 sounds good too.

Are there any tty settings one might want to change via stty other than flow control? Looking at the man page, it looks like mostly what stty does is break things.

@mwm
Copy link
Contributor Author

mwm commented Dec 16, 2016

Fish already special cases flow control by explicitly turning it off. That's why there are multiple bug reports about this. Almost all the values stty can set are useful, but once set to the values the users prefer all it does is break things. As demonstrated by fish using it to break flow control for users who prefer it be enabled.

Once you start special casing bits of stty functionality, looking at a man page won't tell you the entire story. You'll need to deal with the possibilities for every system that fish is ported to, as they may not all have the same functionality.

@ridiculousfish
Copy link
Member

Some of what stty does falls in the "user preference" category, including flow control. But other settings are just incompatible with fish, like echo mode and canonical mode; of course fish should disable those for at least its interactive use. The question is which category is bigger, and how we deal with that difference.

bash stomps the tty mode for programs that exit unsuccessfully or via a signal; otherwise it preserves it.

@mwm
Copy link
Contributor Author

mwm commented Dec 16, 2016

Are the settings used by fish for it's own interactions relevant here? I thought this was about the settings that fish used for executing programs; the ones stored in terminal_mode_for_executing_programs. Since that mode isn't used for fish'es interactions, it doesn't seem like them being compatible with fish should matter.

@ridiculousfish
Copy link
Member

Mmm, good point, I did lose sight of the fact that we're just talking about the tty modes for external commands. With that in mind, the bash behavior of saving the tty modes for successful external commands seems reasonable.

Concretely: fish initializes tty modes like it does today. If a command exits normally (not via a signal) with status 0, overwrite tty_modes_for_external_cmds with the tty state. The behavior of shell_modes is not changed.

Is there an argument against this? There's still the cat < nonsense.bin issue, but usually when I make that mistake, I ctrl-C to cancel, in which case the tty modes won't be changed since cat won't exit successfully.

@mwm
Copy link
Contributor Author

mwm commented Dec 17, 2016

Sounds like a good plan to me. How does it interact with programs that don't exit, but suspend? And whoever implements this might want to look at how fish sets the tty modes when it exits or suspends. I don't think anything will need to change, but think it's worth looking at.

@trulyliu
Copy link

Most of shells support flow control, try fish shell remove this feature to compromise new users' mistake?
New users press CTRL-S accidentally to cause terminal freeze, That's not the problem of shell.
New user should learn to use CTRL-S and CTRL-Q.
Can fish shell enable control flow by default and give some hints when user press CTRL-S accidentally ?

@krader1961
Copy link
Contributor

krader1961 commented Dec 19, 2016

@mwm: Citation needed for your previous assertion(s). Whatever we do we do not want the behavior that plagues other shells where it is far too easy to run a program which alters the termios settings in a fashion that leaves the shell seemingly unusable. That is, characters are not echoed and you have to use [ctrl-J] rather than [enter] to execute a command. It isn't sufficient to check the exit status of the program; although that does make it less likely to leave the tty in a weird state.

Too, the reality is that the vast majority of the termios settings are no longer relevant in any environment where fish might be used. This includes such arcana as RTS/CTS flow control, character size, parity handling, etcetera. I have no doubt there are people still using UNIX in those legacy environments who need to be able to modify those settings. But I see no reason to burden fish with supporting those environments. Any setting that is relevant in a semi-modern environment would be whitelisted in a manner similar to flow control and special characters. The alternative is to use a blacklist approach but that is far more difficult and prone to letting people/programs change things that can cause problems.

Remember that the "F" in Fish shell means "friendly". If someone really needs to alter some obscure termios setting that fish doesn't allow to be changed via an external command they simply need to do so before launching fish. Fish assumes that whatever termios settings are in effect when it starts are sane.

P.S., The term "consensus" does not mean that everyone agrees; only that there is general agreement. This is an engineering decision which involves risk/benefit analysis. There will be some people, like yourself, who would prefer a different decision. That's fine and if you can convince the core devs that fish should use whatever termios settings have been made by an external command then that becomes the consensus.

@krader1961
Copy link
Contributor

There has been discussion about special casing the stty command. That is not a viable approach. What if I have embedded stty in a shell script (i.e., an external command) that does other things? Why shouldn't those termios changes be honored?

Of the four solutions mentioned by @ridiculousfish in this comment the only two that make sense are 3 and 4. It seems like most of us prefer option 3 while @mwm prefers option 4. As I've already said I don't like option 4 even though I am accustomed to typing stty sane[ctrl-J] after exiting a program that has left the tty in a weird state. It isn't friendly.

The only sane choice is option 3. The question then becomes whether we use a whitelist or blacklist based solution. The whitelist option is simpler to reason about and implement. And, as I said in my previous comment, if someone really needs to change a termios setting that fish would not normally allow being changed they only need to do so before running fish.

@mwm
Copy link
Contributor Author

mwm commented Dec 19, 2016

While I agree about what a consensus is, you're the only one I see saying we should restrict things. That's not a consensus.

The argument please argument about "not burdening fish" isn't relevant in this case. We're not adding features to do things - and I'll be first in line to not add nearly useless feature - we're adding features to prevent users from doing things. Which I find distinctly unfriendly, so I'm going to get in line again to not add nearly useless features.

I'm not sure which claim you want a citation for? While the claim about bad reasons is an opinion, it's part of the UNIX philosophy. See slide five in https://www.google.com/url?sa=t&source=web&rct=j&url=http://www.cs.virginia.edu/~drl7x/unixslides/unixslides.pdf&ved=0ahUKEwiLk7HyqoDRAhUL0oMKHbGqAscQFggfMAE&usg=AFQjCNG4ctTIwyRlQV27FQNenMOuJAepTw&sig2=T8xtEG9EHluDWHGI22MBmg. And the reasons you cite for the proposed behavior have been cited for the current behavior as well. If making sure uses don't have more rope than you think they need is part of the design philosophy, that ought to be listed in the docs. It would have avoided this argument.

Having something that behaves differently on two nearly identical things is pretty much the definition of POLA.

For different systems having different ioctls, compare the BSD man page to the Linux one.

Frankly, the only reason I care is that it would be nice to have a current version of fish that didn't treat me like an idiot. But running an out of date version means I stopped contributing my changes.

@faho
Copy link
Member

faho commented Dec 19, 2016

While I agree about what a consensus is, you're the only one I see saying we should restrict things. That's not a consensus.

That's actually correct, though I didn't think so as well. @krader1961: @ridiculousfish never said anything about restricting it to this subset.

His last comment says:

Concretely: fish initializes tty modes like it does today. If a command exits normally (not via a signal) with status 0, overwrite tty_modes_for_external_cmds with the tty state. The behavior of shell_modes is not changed.

Personally, I can't see a use for 90% of sttys options, but then again I also never had a need for flow control.

Stuff like setting the baud rate just seems archaic - if you have a high-latency connection, chances are it's gonna be via ssh (or even mosh), not directly via tty.

But the question is if this actually happens accidentally - if it doesn't, we can just let it happen. If it screws anything up, it's the users fault. If it does happen by accident, restricting it might be useful.

From experimentation, it seems that stty -icanon doesn't echo any sequences to the tty, it uses an ioctl. So I can't see this happening with cat /dev/random.

So, if that's true, I can't see a reason for restricting the user.

We should still keep flow control off by default, because it's confusing if you don't know about it and because it allows us to bind more keys - \cs is a nice combination for e.g. history searching.

I think stty ixon is a weird way to express "I'd like flow control", which is why I proposed a more explicit way of setting it, but that idea seems to not receive much love.

@mwm
Copy link
Contributor Author

mwm commented Dec 19, 2016

This change wouldn't affect our ability to bind flow control characters, as we aren't changing the shell tty modes at all. And while I think flow control should never have been turned off because doing that confuses people who are used to it, changing the default now seems like a bad idea.

@mwm
Copy link
Contributor Author

mwm commented Dec 19, 2016

You know, the more I think about it, the crazier option 3 seems to be.

First, implementation is a nightmare. Do you set the non-approved options to the values you think they ought to have, or leave them at the system default? One way seems to require setting all the options you deem unsafe individually after grabbing the new settings, which means you're going to leave the user exposed to breakage by system-specific extensions. The other seems to require copying allowable changes from the new settings to the system defaults, which means you have to deal with requests to enable system-specific extensions as users stumble over them.

Second, the real damage from catting a binary isn't from it clobbering the tty settings.It's from it clobbering the terminal (emulator) settings. Putting it into graphics mode, or clearing the scrolling region, or overwriting the window title (many Linux distros in giving the user what the dev thought they should have broke my window titles on OSX, and did it in a way that required root access to disable. Which is why I quit running bash), or switching to a larger font, or changing font colors, or - well, you get the idea. The only thing that fish (well, the year+ old version I run because it's got my fixes for this bug in it) does about this is resets the colors.

Trying to protect users from themselves is simply not the Unix way of doing things, and for good reason. Not only does it make working on the system harder (which the developers - who were all users - hated), it's a never-ending task. Every command has dangers in it, so you have to check them all. Many modern shells guard against "rm *", but fish (ditto) doesn't. That's a lot more dangerous than stty, as there's no simple command to recover from it. Nuts, giving users access to a a shell is like giving them a loaded gun. Breaking stty is like replacing one of the live rounds with a blank. It might make a politician happy because they "did something", but in the end it's pointless.

@faho
Copy link
Member

faho commented Dec 19, 2016

which means you have to deal with requests to enable system-specific extensions as users stumble over them.

What system-specific extensions are there? To me it seems like there's not a lot changing in this space so it doesn't seem unwieldy to handle.

Trying to protect users from themselves is simply not the Unix way of doing things, and for good reason

Again, it's not about protecting users from themselves. It's about protecting users from accidental changes. E.g. if you were to run cat SOMEFILE and it would change tty modes because there's some weird sequence embedded there, that would be bad. And that we should protect against, especially if it e.g. made the return key non-functional (requiring ctrl-j instead). Unless you know that ctrl-j would help here, that's effectively an unusable shell.

However, that seems to not be the case, so option 3 would only serve to annoy users who really want to change this stuff (for reasons I don't understand). That is, of course, if I'm correct (which might very well not be the case).

@mwm
Copy link
Contributor Author

mwm commented Dec 19, 2016

It's impossible to provide a list of every available system-specific extension without having access to every system that fish builds on, and I don't have that. Doubt that anyone does. But the one that comes to mind every time I think of this is the BSD SIGINFO settings. You're piss off a lot of BSD & OSX users if you break that. On a vaguely related not, I pissed off my boss when I first installed this patch on our BSD systems, because it kept her from logging in. I couldn't imagine anyone using control characters in a password, but she had done that. A lesson in making assumptions about what people might want.

Yeah, the user probably broke the tty modes accidentally But who do you think typed the command? zsh catching "rm *" catches a common typo of mine in fat-thumbing the space bar. Sure, it's not my intention, but it's my fat thumb. You're not protecting the user from their intentions, you're protecting them from their mistakes - but it's still them you're protecting them from.

@mwm
Copy link
Contributor Author

mwm commented Dec 19, 2016

This is to rich to pass up. I found this quote in the issue #1511

Anyone using a CLI like fish should be presumed to know they're playing with fire.

I couldn't agree more. Great argument for not trying to special case tty setting.

@ridiculousfish
Copy link
Member

ridiculousfish commented Dec 21, 2016

When bash suspends (ctrl-Z) a job, it resets the tty to whatever the settings were before. If you then restore the job, it does not bother to restore the job's tty settings. This seems OK to me - tracking the tty settings per job might sound good at first, but it's easy to imagine someone backgrounding a job specifically to set some tty setting, and they would want that to be reflected. I think it's better to be simple here.

To make this concrete, I understand the leading candidate for a consensus to be:

  1. fish's shell modes stay as they are today
  2. external command modes are initialized as they are today
  3. if a command exits with status 0, set the external command modes to the current tty modes, with some TBD sanitization

Regarding the level of sanitation:, @krader1961 you expressed concerns about altering settings that leave the shell unusable. But here we're discussing only the modes for external commands, so the shell itself won't be at risk. Do you still feel as strongly about sanitizing for external commands? If so, has your view changed on whitelist vs blacklist? mwm's SIGINFO example is a strong argument for a blacklist IMO.

@krader1961
Copy link
Contributor

It is not sufficient to remember termios changes made by external commands and use them only for subsequent external commands. If someone changes the interrupt character, for example, it should apply to the current fish shell not just subsequent external commands.

As for @mwm's SIGINFO example that would be handled by my proposal as they expect since it would preserve all "special characters" for the current interactive shell and subsequent external commands.

My recent testing of an unrelated problem on Cygwin and BSD left the non-fish shell unusable. Specifically, characters were not echoed and [enter] did not execute the command I typed. I had to run stty sane[ctrl-J] to restore my session to a usable state. Do we really want to require people to know how to deal with such situations?

Too, @mwm's use of my

Anyone using a CLI like fish should be presumed to know they're playing with fire.

quote is an example of using a quote out of context.

@mwm
Copy link
Contributor Author

mwm commented Dec 26, 2016

You're right that fish ought to let users set a variety of tty features in shell mode as well. Please open another issue for it, though.

The current behavior of other shells is only relevant if they implement the proposed blacklist. As far as I know, none of them do that.

You're right, it's out of context: in that one, you're arguing that we shouldn't make it easy for users to protect themselves from a typo that results in executing "rm *", as opposed to taking away their ability to set the tty mode the way they want. I'm curious - if "stty sane' is to hard to expect users to remember it, what's the nice easy solution for 'rm *' that you expect them to use?

You haven't responded to my pointing out that we're not protecting the user from commands that set the terminal itself to some strange unusable state. For instance, try running "printf '\33(0\33)B" in your shell and tell me how usable it is.

@dniku
Copy link

dniku commented Aug 13, 2017

I'd like to request some status update as well. I was brought here because stty intr <whatever> doesn't work in fish, and I'd love to see it working so that copy/paste shortcuts could be unified with the rest of the system.

@frankseide
Copy link

Hi, would it be possible to implement anything to freeze screen output? I work with tools that write a lot of information to the screen. Sometimes need to scroll back in the log output of a tool to find a specific line. However, each time a new line is printed, the terminal jumps right back to the end. It is impossible to scroll back while a program is running under fish.

It does not really matter to me whether it is Ctrl-S/Q. It can be any weird key combination, just let me freeze the terminal. It is also OK to just make this a configurable option, where I must define my own key binding.

I love many things of the fish shell, and have switched exclusively to it, but this one issue is making fish unusable for me in one critical scenario.

@cowgod
Copy link

cowgod commented May 6, 2019

I've had a recurring issue with fish for years, and my best guess is that it's related to this bug. When I SSH to other machines, the terminal settings get screwed up. The most noticeable symptom is that the Enter key no longer works properly inside the SSH session, instead sending a ^M. Running 'stty sane' in the SSH session resolves the issue, until the next time I connect to the server.

For example:

image

FWIW, I'm always running inside gnu screen, both my fish session on my local machine (a MacBook Pro running macOS) and on the remote server (running various versions of Linux).

@wilhelmy
Copy link
Contributor

Since the use case for flow control doesn't seem to be completely clear, I would like to be able to use flow control to stop fast-scrolling logs so I can read further up in the window about what's going on without further output scrolling the terminal all the way to the bottom.

There's still no way to get it working?

@frankseide
Copy link

I have the same use case as @wilhelmy.

@wilhelmy
Copy link
Contributor

Maybe this use case can be solved in urxvt, though. I've been thinking about (from an urxvt context) halting scrolling from perl, but now that I'm thinking about it and since the terminal emulator probably knows about whether flow control says "stop the flow", that a warning "flow is currently stopped" could be displayed (in reality, this means patching a high amount of terminal emulators though, unfortunately, if we care about a 100% solution). ^S/^Q have serious usability issues mostly because they're not visible and introduce random breakage for users who do not expect them, so I can see why there's reluctance in enabling them by default.

@frankseide
Copy link

It would be great to even have this as a non-default option, something that power users can enable.

@faho faho closed this as completed in 29754f3 May 16, 2020
@faho
Copy link
Member

faho commented May 16, 2020

Yeah, I just changed it so we keep the terminal modes for external commands to whatever they were changed to. We didn't do any alterations before (we just kept what they were on startup), so I decided not to do so now.

That means if you want to enable flow control, use stty ixon ixoff and have fun.

The shell modes remain untouched, pristine, nice, usable modes. There's no stopping fish or enabling echo or anything, so if you break anything, use stty sane and press return - no need for ctrl-j shenanigans.

@zanchey zanchey modified the milestones: fish-future, fish 3.2.0 May 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020
@faho
Copy link
Member

faho commented Feb 15, 2021

Alright, this was refined in #7133 to fixup modes that fish doesn't expect, many even for external commands - this includes stuff like OPOST, which needs to be on unless a program is explicitly made for it off, in which case it can just set it itself.

Then in #7704 we decided to copy ixon/ixoff into the shell's modes, so if you enable flow control it will also be enabled inside fish.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests