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

Command substitution leaks processes if they expect input (command substitutions run in their own process group) #1362

Closed
ghost opened this Issue Mar 24, 2014 · 35 comments

Comments

Projects
None yet
@ghost

ghost commented Mar 24, 2014

Example: set -l var (cat)

Expected: $var contains value of user input.

Actual: $var contains an empty string, and if you run jobs, a stopped but not reaped cat process lingers unless you run kill -9 on it.

I understand that in this case, I could use read, but this error would still be a thing.

@zanchey zanchey added this to the fish-future milestone Mar 26, 2014

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Mar 27, 2014

Member

In bash and zsh, command substitutions run in the process group of the shell. In fish, they run in their own process group. So when they try to read from stdin, they get SIGTTIN or whatever, and stop. Then fish notices they stopped and just continues on, allowing the command substitution to return.

Member

ridiculousfish commented Mar 27, 2014

In bash and zsh, command substitutions run in the process group of the shell. In fish, they run in their own process group. So when they try to read from stdin, they get SIGTTIN or whatever, and stop. Then fish notices they stopped and just continues on, allowing the command substitution to return.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey
Member

zanchey commented Mar 29, 2014

@dimonomid

This comment has been minimized.

Show comment
Hide comment
@dimonomid

dimonomid Mar 29, 2014

zanchey, thanks for pointing me to this issue.

So yes, as I already stated in the mailing list, I'm unable to use fzf in fish shell, and this makes fish completely out for me (fzf is SO useful)

ridiculousfish, from your message I would assume this behavior can't be changed by definition, like "it's feature, not bug". Am I right? Or, will it be fixed?

dimonomid commented Mar 29, 2014

zanchey, thanks for pointing me to this issue.

So yes, as I already stated in the mailing list, I'm unable to use fzf in fish shell, and this makes fish completely out for me (fzf is SO useful)

ridiculousfish, from your message I would assume this behavior can't be changed by definition, like "it's feature, not bug". Am I right? Or, will it be fixed?

@neersighted

This comment has been minimized.

Show comment
Hide comment
@neersighted

neersighted Jun 23, 2014

I'd also like to bump this issue. I'm seeing similar problems using the same program (fzf), but this also applies in general to any program that needs to accept input and output to stdout.

/cc @junegunn

neersighted commented Jun 23, 2014

I'd also like to bump this issue. I'm seeing similar problems using the same program (fzf), but this also applies in general to any program that needs to accept input and output to stdout.

/cc @junegunn

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Sep 15, 2014

Contributor

@ridiculousfish What is the reason behind command substitutions running in their own process group?

Contributor

kballard commented Sep 15, 2014

@ridiculousfish What is the reason behind command substitutions running in their own process group?

@ByScripts

This comment has been minimized.

Show comment
Hide comment
@ByScripts

ByScripts Nov 17, 2014

Contributor

Is there any workaround for this ?

I would like to store the result of a mysql query with set result (mysql -uroot -p -e "...") but since the password is never requested, it does not work.

Contributor

ByScripts commented Nov 17, 2014

Is there any workaround for this ?

I would like to store the result of a mysql query with set result (mysql -uroot -p -e "...") but since the password is never requested, it does not work.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Jan 16, 2015

Member

fzf recommends a workaround which may be helpful in some instances - see the fzf README for an example.

Member

zanchey commented Jan 16, 2015

fzf recommends a workaround which may be helpful in some instances - see the fzf README for an example.

@paulcarey

This comment has been minimized.

Show comment
Hide comment
@paulcarey

paulcarey Jan 16, 2015

Rather than calling $ vim (git ls-files | peco) which fails for the reason described above, xargs can be used to the same effect.

 $ git ls-files | peco | xargs -I _ fish -c 'vim _ < /dev/tty'

I'd guess that the original issue with set could be worked around with a wrapper function that set a universal variable, but I haven't tried this.

paulcarey commented Jan 16, 2015

Rather than calling $ vim (git ls-files | peco) which fails for the reason described above, xargs can be used to the same effect.

 $ git ls-files | peco | xargs -I _ fish -c 'vim _ < /dev/tty'

I'd guess that the original issue with set could be worked around with a wrapper function that set a universal variable, but I haven't tried this.

@jcelliott

This comment has been minimized.

Show comment
Hide comment
@jcelliott

jcelliott Jan 27, 2015

Contributor

Another vote for finding a way to fix this. Is it something that can reasonably be changed?

Contributor

jcelliott commented Jan 27, 2015

Another vote for finding a way to fix this. Is it something that can reasonably be changed?

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 27, 2015

Member

Oh sure, we just have to fix it.

Member

ridiculousfish commented Jan 27, 2015

Oh sure, we just have to fix it.

@jcelliott

This comment has been minimized.

Show comment
Hide comment
@jcelliott

jcelliott Jan 28, 2015

Contributor

Any pointers on where to start looking?

Contributor

jcelliott commented Jan 28, 2015

Any pointers on where to start looking?

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 28, 2015

Member

The key change is to run command substitutions in the foreground process group, and to properly handle stopped jobs when doing command substitutions .It would be interesting to investigate how bash handles this - what happens if you stop (e.g. ctrl-Z) a job in a command substitution?

Member

ridiculousfish commented Jan 28, 2015

The key change is to run command substitutions in the foreground process group, and to properly handle stopped jobs when doing command substitutions .It would be interesting to investigate how bash handles this - what happens if you stop (e.g. ctrl-Z) a job in a command substitution?

@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Feb 18, 2015

Looks like it just kills it.

bash-interrupt-bg

JoshCheek commented Feb 18, 2015

Looks like it just kills it.

bash-interrupt-bg

rahulg added a commit to rahulg/agentmgr that referenced this issue Feb 20, 2015

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Mar 11, 2015

Member

It swallows Ctrl-Z, but running kill -SIGSTOP from another session just queues input until sent SIGCONT.

Member

zanchey commented Mar 11, 2015

It swallows Ctrl-Z, but running kill -SIGSTOP from another session just queues input until sent SIGCONT.

@zanchey zanchey changed the title from Command substitution leaks processes if they expect input to Command substitution leaks processes if they expect input (command substitutions run in their own process group) Mar 11, 2015

@tbodt

This comment has been minimized.

Show comment
Hide comment
@tbodt

tbodt Dec 25, 2016

@ridiculousfish Any more progress on this since March?

tbodt commented Dec 25, 2016

@ridiculousfish Any more progress on this since March?

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Dec 26, 2016

Member

No, I haven't done any work on this. I guess the first step would be to answer the question of, what should fish do if a command substitution stops instead of exits?

Member

ridiculousfish commented Dec 26, 2016

No, I haven't done any work on this. I guess the first step would be to answer the question of, what should fish do if a command substitution stops instead of exits?

@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Dec 27, 2016

Not 100% sure I understand (and my prev gif seems like its wrong to me right now)

This is what I think you're saying is happening:

  • User enters the command $ a (b)
  • A process group is created
  • B a new process is made to run b
    • Its in the new process group
    • Its standard input is the shell's standard input
    • It runs in the background
  • b tries to read from standard input
  • b is put to sleep (by the OS) because it's not in the foreground (active?) process group (this is because the process itself is in the background?)
  • Fish sees b went to sleep
  • Fish continues as if b had exited successfully and printed nothing, invoking a ""
  • The world forgets about poor old b... well, except for those of us in this thread

If that's correct, then these are potential solutions? (note that they handle this case, which is probably the common case, but they don't address the general case of a command substitution stopping)

  • Give it an empty stream for standard input (looking at fzf, this is probably the wrong answer.)
  • Run command substitutions in the foreground (what does that mean, why aren't they run in it currently? I'm guessing separate process groups are used so we can kill it and all its children if we need? If so, do w actually need that? If so, do is there a way to get both? I don't know a lot about process management)
  • Somehow ferry stdin to the process? eg if fish's stdin is a tty then run b through a pty, using select to or something similar to detect when its trying to pull from stdin, pulling from the real stdin in fish, and printing it to the b's stdin (seems fragile, but who knows)

JoshCheek commented Dec 27, 2016

Not 100% sure I understand (and my prev gif seems like its wrong to me right now)

This is what I think you're saying is happening:

  • User enters the command $ a (b)
  • A process group is created
  • B a new process is made to run b
    • Its in the new process group
    • Its standard input is the shell's standard input
    • It runs in the background
  • b tries to read from standard input
  • b is put to sleep (by the OS) because it's not in the foreground (active?) process group (this is because the process itself is in the background?)
  • Fish sees b went to sleep
  • Fish continues as if b had exited successfully and printed nothing, invoking a ""
  • The world forgets about poor old b... well, except for those of us in this thread

If that's correct, then these are potential solutions? (note that they handle this case, which is probably the common case, but they don't address the general case of a command substitution stopping)

  • Give it an empty stream for standard input (looking at fzf, this is probably the wrong answer.)
  • Run command substitutions in the foreground (what does that mean, why aren't they run in it currently? I'm guessing separate process groups are used so we can kill it and all its children if we need? If so, do w actually need that? If so, do is there a way to get both? I don't know a lot about process management)
  • Somehow ferry stdin to the process? eg if fish's stdin is a tty then run b through a pty, using select to or something similar to detect when its trying to pull from stdin, pulling from the real stdin in fish, and printing it to the b's stdin (seems fragile, but who knows)
@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Dec 27, 2016

Member

This is really a UI question - what ought fish to do if a command substitution stops without exiting?

Realistically I think we have three options:

  1. Wait forever
  2. kill the process
  3. orphan the process

It looks like bash and zsh use option 1.

Member

ridiculousfish commented Dec 27, 2016

This is really a UI question - what ought fish to do if a command substitution stops without exiting?

Realistically I think we have three options:

  1. Wait forever
  2. kill the process
  3. orphan the process

It looks like bash and zsh use option 1.

@ninjaaron

This comment has been minimized.

Show comment
Hide comment
@ninjaaron

ninjaaron Jan 19, 2017

seems like a fairly acceptable 90% solution.

ninjaaron commented Jan 19, 2017

seems like a fairly acceptable 90% solution.

@maletor

This comment has been minimized.

Show comment
Hide comment
@maletor

maletor Jan 24, 2017

+1, wait forever.

Like with this issue.

Just kidding. I love you.

maletor commented Jan 24, 2017

+1, wait forever.

Like with this issue.

Just kidding. I love you.

@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Jan 25, 2017

@maletor that probably was intended to be funny, but it comes off as condescending, maybe enjoy the laugh without sharing next time :)

JoshCheek commented Jan 25, 2017

@maletor that probably was intended to be funny, but it comes off as condescending, maybe enjoy the laugh without sharing next time :)

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 29, 2017

Member

This is really a UI question - what ought fish to do if a command substitution stops without exiting?

@ridiculousfish: Honestly, the UI question comes later. The first thing we ought to do is remove the reason these things are stopping in the first place - because they got SIGTTOU, because we don't allow that they use the terminal. That's the most common reason for stopping, so if we remove that, 99% of use for that UI disappears.

Note also that currently echo (cat | string trim) will hang with no obvious way out because we wait on that forever.

I'll PR a possible fix for this now.

Member

faho commented Mar 29, 2017

This is really a UI question - what ought fish to do if a command substitution stops without exiting?

@ridiculousfish: Honestly, the UI question comes later. The first thing we ought to do is remove the reason these things are stopping in the first place - because they got SIGTTOU, because we don't allow that they use the terminal. That's the most common reason for stopping, so if we remove that, 99% of use for that UI disappears.

Note also that currently echo (cat | string trim) will hang with no obvious way out because we wait on that forever.

I'll PR a possible fix for this now.

faho added a commit to faho/fish-shell that referenced this issue Mar 29, 2017

@faho faho referenced this issue Mar 29, 2017

Closed

Also give subshells the terminal #3922

0 of 2 tasks complete

@krader1961 krader1961 closed this in d7ef7eb Apr 1, 2017

faho added a commit that referenced this issue Apr 1, 2017

@faho faho modified the milestones: 2.6.0, fish-future Apr 10, 2017

develop7 added a commit to develop7/fish-shell that referenced this issue Apr 17, 2017

develop7 added a commit to develop7/fish-shell that referenced this issue Apr 17, 2017

miquella added a commit to miquella/vaulted that referenced this issue Apr 27, 2017

Fix suggested fish command to load variables
The previous command ended up running all of the lines together on a
single line and since the first line begins with a comment character,
the entire script was effectively commented out!

For additional information, see:
 * fish-shell/fish-shell#159
 * fish-shell/fish-shell#3993

Additionally, commands run in a fish command substitution do not have
access to the TTY, preventing vaulted from prompting for a password.
Using piping avoids this issue all together.

For additional information, see:
 * fish-shell/fish-shell#1362
 * fish-shell/fish-shell#3922

tpickett66 added a commit to miquella/vaulted that referenced this issue Apr 27, 2017

Fix suggested fish command to load variables
The previous command ended up running all of the lines together on a
single line and since the first line begins with a comment character,
the entire script was effectively commented out!

For additional information, see:
 * fish-shell/fish-shell#159
 * fish-shell/fish-shell#3993

Additionally, commands run in a fish command substitution do not have
access to the TTY, preventing vaulted from prompting for a password.
Using piping avoids this issue all together.

For additional information, see:
 * fish-shell/fish-shell#1362
 * fish-shell/fish-shell#3922

godfat added a commit to godfat/dev-tool that referenced this issue Oct 5, 2017

@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Apr 18, 2018

I hit this issue again both yesterday and today. I didn't reread the thread, the last comment makes it look like this might be a known deficiency, but I reported it here as it seems like a regression. After some debugging, I was able to identify that the issue occurs when I pipe bash -i into a function. SS included to make it easier to read, source included to make it easier to reproduce.

screen shot 2018-04-18 at 4 04 39 pm

# ===== Demo the issue =====

# hangs (I'll press C-c)
$ bash -i -c 'echo hello >&2' | grep whatever
Job 3, '$ bash -i -c 'echo hello >&2' |…' has stopped

# works (use the command instead of the function)
$ bash -i -c 'echo hello >&2' | command grep whatever
hello

# works (remove the -i flag)
$ bash -c 'echo hello >&2' | grep whatever
hello


# ===== Other potentially useful info =====

# the grep function
$ type grep
grep is a function with definition
function grep
    command grep --color=auto $argv
end

# the fish version
$ fish --version
fish, version 2.7.1

# this is as up-to-date as is installable
$ brew upgrade fish
Error: fish 2.7.1 already installed

JoshCheek commented Apr 18, 2018

I hit this issue again both yesterday and today. I didn't reread the thread, the last comment makes it look like this might be a known deficiency, but I reported it here as it seems like a regression. After some debugging, I was able to identify that the issue occurs when I pipe bash -i into a function. SS included to make it easier to read, source included to make it easier to reproduce.

screen shot 2018-04-18 at 4 04 39 pm

# ===== Demo the issue =====

# hangs (I'll press C-c)
$ bash -i -c 'echo hello >&2' | grep whatever
Job 3, '$ bash -i -c 'echo hello >&2' |…' has stopped

# works (use the command instead of the function)
$ bash -i -c 'echo hello >&2' | command grep whatever
hello

# works (remove the -i flag)
$ bash -c 'echo hello >&2' | grep whatever
hello


# ===== Other potentially useful info =====

# the grep function
$ type grep
grep is a function with definition
function grep
    command grep --color=auto $argv
end

# the fish version
$ fish --version
fish, version 2.7.1

# this is as up-to-date as is installable
$ brew upgrade fish
Error: fish 2.7.1 already installed
@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Apr 18, 2018

Oh. One other maybe relevant thing: I pressed C-c, but it really stopped the process. ps shows a "T" (I assume this is SIGSTOP) both before and after I press C-c. After I fg, it prints "hello" and exits normally.

This kind of implies to me that it could be a different issue (b/c it resumes when I foreground it, but the the original had to be SIGKILLed, I believe). Let me know if you want me to post this as its own issue.

JoshCheek commented Apr 18, 2018

Oh. One other maybe relevant thing: I pressed C-c, but it really stopped the process. ps shows a "T" (I assume this is SIGSTOP) both before and after I press C-c. After I fg, it prints "hello" and exits normally.

This kind of implies to me that it could be a different issue (b/c it resumes when I foreground it, but the the original had to be SIGKILLed, I believe). Let me know if you want me to post this as its own issue.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Apr 19, 2018

Member

This kind of implies to me that it could be a different issue

Very much a different issue - this one is explicitly about command substitutions - echo (this stuff).

The problem with bash -i seems to be that it grabs the terminal for itself - strace -e ioctl shows it doing one with TIOCSPGRP - which is the underlying syscall for tcsetpgrp on linux.

Member

faho commented Apr 19, 2018

This kind of implies to me that it could be a different issue

Very much a different issue - this one is explicitly about command substitutions - echo (this stuff).

The problem with bash -i seems to be that it grabs the terminal for itself - strace -e ioctl shows it doing one with TIOCSPGRP - which is the underlying syscall for tcsetpgrp on linux.

@JoshCheek

This comment has been minimized.

Show comment
Hide comment
@JoshCheek

JoshCheek Apr 19, 2018

Very much a different issue - this one is explicitly about command substitutions - echo (this stuff).

Okay, made a new issue for it here

JoshCheek commented Apr 19, 2018

Very much a different issue - this one is explicitly about command substitutions - echo (this stuff).

Okay, made a new issue for it here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment