From 14c7cfa84b563b4e037995080c4a72ae8c7f93f6 Mon Sep 17 00:00:00 2001
From: Kurtis Rader <krader@skepticism.us>
Date: Mon, 4 Jul 2016 20:28:21 -0700
Subject: [PATCH] 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
---
 .../__fish_make_completion_signals.fish       | 32 +++++++++++++------
 src/builtin_complete.cpp                      | 16 ++++++++++
 tests/test6.err                               |  4 +++
 tests/test6.in                                |  7 ++++
 4 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/share/functions/__fish_make_completion_signals.fish b/share/functions/__fish_make_completion_signals.fish
index b41735022..5cf4acd04 100644
--- a/share/functions/__fish_make_completion_signals.fish
+++ b/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
diff --git a/src/builtin_complete.cpp b/src/builtin_complete.cpp
index a3e14fdea..9dd180106 100644
--- a/src/builtin_complete.cpp
+++ b/src/builtin_complete.cpp
@@ -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': {
@@ -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': {
diff --git a/tests/test6.err b/tests/test6.err
index e69de29bb..f259ceb70 100644
--- a/tests/test6.err
+++ b/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
diff --git a/tests/test6.in b/tests/test6.in
index 0d6ef4878..de217fe57 100644
--- a/tests/test6.in
+++ b/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)