Improve dangerous/undefined PID expansion behavior #1008

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@lunixbochs
Contributor

lunixbochs commented Sep 19, 2013

Backstory

In Bash, % means something like 'last job backgrounded'. In upstream Fish, it means either "all backgrounded jobs" or "all processes owned by your user" if there are no processes backgrounded.

I usually run kill -9 % in Fish if I actually wanted to hard kill the last job backgrounded (useful if it wasn't responding). This actually matched for all jobs backgrounded, but I never noticed until today.

Even further, it turns out kill -9 % in Fish kills all processes owned by your user if you ever accidentally run it without backgrounding something first.

Go ahead, echo % to get a feel for how it works right now.


Change 1

Use Bash-like expansion for empty searches (when you just use a '%' by
itself).

'%' will now only match the last valid backgrounded process.
If there are no such processes, an expansion error will be generated.

'%' by itself would previously match either all backgrounded
processes, or failing that, all processes owned by your user. If you
ever tried to run kill -9 %, it would either kill all backgrounded
processes or all of your processes. I'm not sure why anyone would ever
want that to be a single keystroke away. You could almost typo it.

As a result of this change, fg %, bg %, kill %, etc will all now operate
on the last process touched by job control (or throw an expansion error).


Change 2

Don't run 'by-name' matches when the search term is numeric.

This prevents you from running a command like kill %1 and accidentally
killing a process named something like "1Command". Overloaded behavior
can be dangerous, and we probably shouldn't play fast and loose with
expansion characters that generate process IDs.

Improve dangerous/undefined PID expansion behavior
1. Use Bash-like expansion for empty searches (when you just use a '%' by
itself).

'%' will now *only* match the last valid backgrounded process.
If there are no such processes, an expansion error will be generated.

'%' by itself would previously match either *all* backgrounded
processes, or failing that, all processes owned by your user. If you
ever tried to run `kill -9 %`, it would either kill all backgrounded
processes or *all* of your processes. I'm not sure why anyone would ever
want that to be a single keystroke away. You could almost typo it.

As a result, `fg %`, `bg %`, `kill %`, etc will all operate on the last
process touched by job control.

2. Don't run 'by-name' matches when the search term is numeric.

This prevents you from running a command like `kill %1` and accidentally
killing a process named something like "1Command". Overloaded behavior
can be dangerous, and we probably shouldn't play fast and loose with
expansion characters that generate process IDs.
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Sep 19, 2013

Member

That is terrific behaviour, not unlike Solaris' killall.

Member

zanchey commented Sep 19, 2013

That is terrific behaviour, not unlike Solaris' killall.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Sep 19, 2013

Member

I wonder if we should just error out on the expansion '%', or at least document it better.

Member

zanchey commented Sep 19, 2013

I wonder if we should just error out on the expansion '%', or at least document it better.

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Sep 19, 2013

Member

Indeed, it sounds dangerous. My suggestion would be to avoid current silly behavior of % (list all processes starting with something), but instead use globs as argument to add more flexibility. For example, current % would be %*, and current %cat would be %cat* (killing cat is dangerous if you run program like catchsegv), and possibly move %self to something like $fish_pid.

Member

xfix commented Sep 19, 2013

Indeed, it sounds dangerous. My suggestion would be to avoid current silly behavior of % (list all processes starting with something), but instead use globs as argument to add more flexibility. For example, current % would be %*, and current %cat would be %cat* (killing cat is dangerous if you run program like catchsegv), and possibly move %self to something like $fish_pid.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Sep 19, 2013

Member

fg % is a pretty common idiom in sh-land, but fg % is better written as fg.

Member

zanchey commented Sep 19, 2013

fg % is a pretty common idiom in sh-land, but fg % is better written as fg.

@lunixbochs

This comment has been minimized.

Show comment
Hide comment
@lunixbochs

lunixbochs Sep 19, 2013

Contributor

My biggest use case for solo % was kill -9 % as stated.

Contributor

lunixbochs commented Sep 19, 2013

My biggest use case for solo % was kill -9 % as stated.

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Sep 21, 2013

Member

@lunixbochs: Useless use of kill -9.

But aside of that – even normal kill would be dangerous (in fish), and sh users are confused, because % does way less dangerous thing in bash. I don't like % matching the latest process, you can use %1 for that after all – and it would introduce yet another special case. In my opinion, it should simply fail – there is no reason to continue, when it's not known what user wants, and suggest using %1 instead.

Member

xfix commented Sep 21, 2013

@lunixbochs: Useless use of kill -9.

But aside of that – even normal kill would be dangerous (in fish), and sh users are confused, because % does way less dangerous thing in bash. I don't like % matching the latest process, you can use %1 for that after all – and it would introduce yet another special case. In my opinion, it should simply fail – there is no reason to continue, when it's not known what user wants, and suggest using %1 instead.

@lunixbochs

This comment has been minimized.

Show comment
Hide comment
@lunixbochs

lunixbochs Sep 21, 2013

Contributor

What exactly does solo % do in bash that's less dangerous?

I found this article: http://web.mit.edu/gnu/doc/html/features_5.html#SEC33 which implies I was correct about the purpose of a standalone % in bash: "The symbols %% and %+ refer to the shell's notion of the current job, which is the last job stopped while it was in the foreground." unless there's some overloaded behavior of a standalone % in bash I haven't yet noticed. That's the behavior I was aiming to replicate with this pull request.

This is fish (with my patch):

~ vim
Job 1, 'vim' has stopped
~ kill %
~ kill -15 %
~ kill -9 %
fish: Job 1, 'vim' terminated by signal SIGKILL (Forced quit)
~ 

This is bash:

~ vim
[1]+  Stopped                 vim
~ kill %
[1]+  Stopped                 vim
~ kill -15 %
[1]+  Stopped                 vim
~ kill -9 %
[1]+  Stopped                 vim
~ 
[1]+  Killed: 9               vim

The entire point of my habit is to hard kill a badly behaving process that responds to ^Z but not ^D, ^C, ^\, and sometimes even kill 15, 2, 1. It's nice to theorize about the correct method of doing things, but in practice kill -9 on a non-critical process has never given me the slightest nasty side effect and saves 3+ steps. If you s/9/15/g in this discussion, the current behavior of % in fish is near as dangerous. This thread isn't a pull request on my shell habits. It's on fish.

Contributor

lunixbochs commented Sep 21, 2013

What exactly does solo % do in bash that's less dangerous?

I found this article: http://web.mit.edu/gnu/doc/html/features_5.html#SEC33 which implies I was correct about the purpose of a standalone % in bash: "The symbols %% and %+ refer to the shell's notion of the current job, which is the last job stopped while it was in the foreground." unless there's some overloaded behavior of a standalone % in bash I haven't yet noticed. That's the behavior I was aiming to replicate with this pull request.

This is fish (with my patch):

~ vim
Job 1, 'vim' has stopped
~ kill %
~ kill -15 %
~ kill -9 %
fish: Job 1, 'vim' terminated by signal SIGKILL (Forced quit)
~ 

This is bash:

~ vim
[1]+  Stopped                 vim
~ kill %
[1]+  Stopped                 vim
~ kill -15 %
[1]+  Stopped                 vim
~ kill -9 %
[1]+  Stopped                 vim
~ 
[1]+  Killed: 9               vim

The entire point of my habit is to hard kill a badly behaving process that responds to ^Z but not ^D, ^C, ^\, and sometimes even kill 15, 2, 1. It's nice to theorize about the correct method of doing things, but in practice kill -9 on a non-critical process has never given me the slightest nasty side effect and saves 3+ steps. If you s/9/15/g in this discussion, the current behavior of % in fish is near as dangerous. This thread isn't a pull request on my shell habits. It's on fish.

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Sep 21, 2013

Member

Yeah, sorry. I was referring to % being insecure in fish.

Member

xfix commented Sep 21, 2013

Yeah, sorry. I was referring to % being insecure in fish.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Sep 22, 2013

Member

OK, so this patch which leads to % (as a standalone expansion) expanding to the latest job iff it exists, and raising an error otherwise.

It sounds like @lunixbochs is opposed to entirely removing the expansion of % as a standalone token. In the absence of consensus, I think this is worth merging as the current behaviour is not ok.

Member

zanchey commented Sep 22, 2013

OK, so this patch which leads to % (as a standalone expansion) expanding to the latest job iff it exists, and raising an error otherwise.

It sounds like @lunixbochs is opposed to entirely removing the expansion of % as a standalone token. In the absence of consensus, I think this is worth merging as the current behaviour is not ok.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Sep 22, 2013

Member

Merged with rebase:

To git@github.com:fish-shell/fish-shell.git
4ea92a9..f2a5237 master -> master

Thanks again!

Member

zanchey commented Sep 22, 2013

Merged with rebase:

To git@github.com:fish-shell/fish-shell.git
4ea92a9..f2a5237 master -> master

Thanks again!

@zanchey zanchey closed this Sep 22, 2013

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Sep 23, 2013

Member

We should update the docs to reflect this new behavior.

Member

ridiculousfish commented Sep 23, 2013

We should update the docs to reflect this new behavior.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Sep 23, 2013

Member

The documentation is now less incorrect than it was - the current text reads

The following expansions are performed:

  • If the string is the entire word self, the shell's PID is the result.
  • Otherwise, if the string is the ID of a job, the result is the process group ID of the job.
  • Otherwise, if any child processes match the specified string, their PIDs are the result of the expansion.
  • Otherwise, if any processes owned by the user match the specified string, their PIDs are the result of the expansion.
  • If none of these matches apply, an error is produced.
Member

zanchey commented Sep 23, 2013

The documentation is now less incorrect than it was - the current text reads

The following expansions are performed:

  • If the string is the entire word self, the shell's PID is the result.
  • Otherwise, if the string is the ID of a job, the result is the process group ID of the job.
  • Otherwise, if any child processes match the specified string, their PIDs are the result of the expansion.
  • Otherwise, if any processes owned by the user match the specified string, their PIDs are the result of the expansion.
  • If none of these matches apply, an error is produced.
@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Sep 23, 2013

Member

So it is. Good work!

Member

ridiculousfish commented Sep 23, 2013

So it is. Good work!

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