Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
make kill/pkill completions more robust (#3200)
Someone running fish in an unusual locale reported that an `assert()` was
firing when they typed `pkill c`. I traced it to two bugs. First, the
__fish_make_completion_signals command was producing a weird result. Second,
the builtin `complete` command wasn't adequately verifying its arguments.

Fixes #3129
  • Loading branch information
Kurtis Rader committed Jul 8, 2016
1 parent 2f0cb2a commit 14c7cfa
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 9 deletions.
32 changes: 23 additions & 9 deletions share/functions/__fish_make_completion_signals.fish
@@ -1,17 +1,31 @@
function __fish_make_completion_signals --description 'Make list of kill signals for completion'
set -q __kill_signals; and return 0
set -q __kill_signals
and return 0

if kill -L ^/dev/null >/dev/null
# Some systems use the GNU coreutils kill command where `kill -L` produces an extended table
# format that looks like this:
#
# 1 HUP Hangup: 1
# 2 INT Interrupt: 2
#
# The procps `kill -L` produces a more compact table. We can distinguish the two cases by
# testing whether it supports `kill -t`; in which case it is the coreutils `kill` command.
if kill -t ^/dev/null >/dev/null
# Posix systems print out the name of a signal using 'kill -l SIGNUM'.
complete -c kill -s l --description "List names of available signals"
for i in (seq 31)
set -g __kill_signals $__kill_signals $i" "(kill -l $i | tr '[:lower:]' '[:upper:]')
end
else
# Debian and some related systems use 'kill -L' to write out a numbered list
# of signals. Use this to complete on both number _and_ on signal name.
complete -c kill -s L --description "List codes and names of available signals"
set -g __kill_signals (kill -L | sed -e 's/\([0-9][0-9]*\) *\([A-Z,0-9][A-Z,0-9]*\)/\1 \2\n/g;s/ +/ /g' | sed -e 's/^ \+//' | __fish_sgrep -E '^[^ ]+')
else
# Posix systems print out the name of a signal using 'kill -l
# SIGNUM', so we use this instead.
complete -c kill -s l --description "List names of available signals"
for i in (seq 31)
set -g __kill_signals $__kill_signals $i" "(kill -l $i)
set -g __kill_signals
kill -L | sed -e 's/^ //; s/ */ /g; y/ /\n/' | while read -l signo
test -z "$signo"
and break # the sed above produces one blank line at the end
read -l signame
set -g __kill_signals $__kill_signals "$signo $signame"
end
end
end
16 changes: 16 additions & 0 deletions src/builtin_complete.cpp
Expand Up @@ -208,6 +208,10 @@ int builtin_complete(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
}
case 'd': {
desc = w.woptarg;
if (w.woptarg[0] == '\0') {
streams.err.append_format(L"%ls: -d requires a non-empty string\n", argv[0]);
res = true;
}
break;
}
case 'u': {
Expand All @@ -220,14 +224,26 @@ int builtin_complete(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
}
case 's': {
short_opt.append(w.woptarg);
if (w.woptarg[0] == '\0') {
streams.err.append_format(L"%ls: -s requires a non-empty string\n", argv[0]);
res = true;
}
break;
}
case 'l': {
gnu_opt.push_back(w.woptarg);
if (w.woptarg[0] == '\0') {
streams.err.append_format(L"%ls: -l requires a non-empty string\n", argv[0]);
res = true;
}
break;
}
case 'o': {
old_opt.push_back(w.woptarg);
if (w.woptarg[0] == '\0') {
streams.err.append_format(L"%ls: -o requires a non-empty string\n", argv[0]);
res = true;
}
break;
}
case 'a': {
Expand Down
4 changes: 4 additions & 0 deletions tests/test6.err
@@ -0,0 +1,4 @@
complete: -o requires a non-empty string
complete: -d requires a non-empty string
complete: -l requires a non-empty string
complete: -s requires a non-empty string
7 changes: 7 additions & 0 deletions tests/test6.in
@@ -1,5 +1,12 @@
# vim: set filetype=fish:

# Regression test for issue #3129. In previous versions these statements would
# cause an `assert()` to fire thus killing the shell.
complete -c pkill -o ''
complete -c pkill -d ''
complete -c pkill -l ''
complete -c pkill -s ''

# Test that conditions that add or remove completions don't deadlock, etc.
# We actually encountered some case that was effectively like this (Issue 2 in github)

Expand Down

0 comments on commit 14c7cfa

Please sign in to comment.