From d1d2ccf732fec40e0d48640f1d23a476a002d987 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Thu, 4 Jun 2015 22:07:49 -0700 Subject: [PATCH 1/3] skill: fix command line with signal, again Have skill_sig_option sanitize the command line by properly decrementing *argc after moving the arguments to remove the -signal one. One bug caused by this issue was when running `kill -1`, then the code would interpret -1 as both SIGHUP and as process group -1 and send SIGHUP to all of them. Or `kill -28` which would send SIGWINCH to process group -2 (in another bug, the -pgid support only accepts a single digit, fix for that bug will follow.) This also reverts commit 7610b3128e6ac4 ("skill: fix command line with signal") which worked around this bug in `skill` and also removes the "sigopt" hack which worked around this bug in `kill`. The skill_sig_option implementation is compatible with signal_option() from pgrep.c. I plan to factor them out into a single source file in a follow up commit, to prevent the duplication. This commit fixes the issues reported above. I also tested the issues from commit 7610b3128e6ac4, `skill -9 -t pts/0` works as expected, also tried `kill` with -signal and a number of pids and it worked as expected. Also tested that `make check` and `make distcheck` keep working. Signed-off-by: Filipe Brandenburger --- skill.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/skill.c b/skill.c index 9a19a8bb..9e499739 100644 --- a/skill.c +++ b/skill.c @@ -398,17 +398,15 @@ static void __attribute__ ((__noreturn__)) skillsnice_usage(FILE * out) static int skill_sig_option(int *argc, char **argv) { - int i, nargs = *argc; + int i; int signo = -1; - for (i = 1; i < nargs; i++) { + for (i = 1; i < *argc; i++) { if (argv[i][0] == '-') { signo = signal_name_to_number(argv[i] + 1); if (-1 < signo) { - if (nargs - i) { - nargs--; - memmove(argv + i, argv + i + 1, - sizeof(char *) * (nargs - i)); - } + memmove(argv + i, argv + i + 1, + sizeof(char *) * (*argc - i)); + (*argc)--; return signo; } } @@ -421,7 +419,6 @@ static void __attribute__ ((__noreturn__)) kill_main(int argc, char **argv) { int signo, i; - int sigopt = 0; int loop = 1; long pid; int exitvalue = EXIT_SUCCESS; @@ -446,8 +443,6 @@ static void __attribute__ ((__noreturn__)) signo = skill_sig_option(&argc, argv); if (signo < 0) signo = SIGTERM; - else - sigopt++; opterr=0; /* suppress errors on -123 */ while (loop == 1 && (i = getopt_long(argc, argv, "l::Ls:hV", longopts, NULL)) != -1) @@ -495,7 +490,7 @@ static void __attribute__ ((__noreturn__)) kill_usage(stderr); } - argc -= optind + sigopt; + argc -= optind; argv += optind; for (i = 0; i < argc; i++) { @@ -588,10 +583,8 @@ static void skillsnice_parse(int argc, prino = snice_prio_option(&argc, argv); else if (program == PROG_SKILL) { signo = skill_sig_option(&argc, argv); - if (-1 < signo) { + if (-1 < signo) sig_or_pri = signo; - argc -= 1; - } } pid_count = 0; From 27b2937d2d4151d595c1170fb9381d5a0a0f67da Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Thu, 4 Jun 2015 22:27:03 -0700 Subject: [PATCH 2/3] kill: print usage if no pid is passed in command line This makes a command such as `kill -TERM` or `kill -9` fails and prints usage, instead of silently succeeding. The behavior is consistent with how `kill` behaves without an explicit signal, or with the behavior of the `kill` builtin in a shell like bash. Signed-off-by: Filipe Brandenburger --- skill.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/skill.c b/skill.c index 9e499739..f8f4e499 100644 --- a/skill.c +++ b/skill.c @@ -493,6 +493,9 @@ static void __attribute__ ((__noreturn__)) argc -= optind; argv += optind; + if (argc < 1) + kill_usage(stderr); + for (i = 0; i < argc; i++) { pid = strtol_or_err(argv[i], _("failed to parse argument")); if (!kill((pid_t) pid, signo)) From 9646f7cba47e078855d1fc5e3be9fb05b1b89629 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Thu, 4 Jun 2015 22:33:02 -0700 Subject: [PATCH 3/3] pkill: reject -signal number with trailing garbage This commit prevents pkill from accepting something like `-1garbage` as a SIGHUP. The previous code was using atoi() which does not check for trailing garbage and would parse the above as 1. Handling numeric signals in signal_option() is not really necessary, since signal_name_to_number() will recognize numeric signals and parse them properly using strtol() and checking for trailing garbage. It also checks that the numeric signals are in the proper range. So all we need to do is remove the buggy numeric signal handling here. Tested with `pkill -1garbage sleep`, after this patch it will complain that "1" is not a valid option, which is the expected. Signed-off-by: Filipe Brandenburger --- pgrep.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/pgrep.c b/pgrep.c index 9a61e6d5..539a2d84 100644 --- a/pgrep.c +++ b/pgrep.c @@ -661,8 +661,6 @@ static int signal_option(int *argc, char **argv) for (i = 1; i < *argc; i++) { if (argv[i][0] == '-') { sig = signal_name_to_number(argv[i] + 1); - if (sig == -1 && isdigit(argv[i][1])) - sig = atoi(argv[i] + 1); if (-1 < sig) { memmove(argv + i, argv + i + 1, sizeof(char *) * (*argc - i));