Newer versions of killall (Ubuntu 18.04+) fail when signal specified in capitals

[Hamish McIntyre-Bhatty]

[Impact]

 * Not all Uppercase signal names are accepted by killall in the new
   version (INT + ILL + VTALRM will not work)

 * by that the man page refers to things no more working and old scripts
   might break.

[Test Case]
 * run this and check if all signals were accepted by the commandline of killall

#!/bin/bash
TNAME="uninitialized"

testkillall () {
        local tsig=${1}
        /tmp/${TNAME} /dev/urandom &
        local tpid=$!;
        echo "TEST -${tsig}";
        killall -${tsig} "${TNAME}"
        kill -9 "${tpid}" 2>/dev/null;
        kill -9 "${TNAME}" 2>/dev/null;
}

for sig in $(killall -l | xargs); do
        TNAME="test${sig}"
        cp /usr/bin/md5sum "/tmp/${TNAME}"
        testkillall "$(echo ${sig} | tr '[:lower:]' '[:upper:]')"
        testkillall "SIG$(echo ${sig} | tr '[:lower:]' '[:upper:]')"
        rm -f "/tmp/${TNAME}"
        sync
done

[Regression Potential]

 * This reworks more of the argument parsing than one would like, but this
   is due to one of such reworks being the breaking change in the former
   version and a more minimal fix is hard to come up with.
   Due to that obviously there could be "new" issues added to the argument
   parsing. At least in this case we checked all signals and they worked
   fine. Furthermore the new code also added a bunch of tests which work
   fine since adding that.

[Other Info]

 * This is a regression as of bionic.
---

[Original Description]

Newer versions of killall (Ubuntu 18.04+) display usage when the signal is passed like:

killall -INT <name>

and instead require it to be passed as:

killall -int <name>

In previous versions from Ubuntu 14.04 and 16.04, this doesn't happen. Is this a bug or a feature change? If so perhaps it should be noted in the usage information? The man page suggests capitals with "-SIGNAL". but that doesn't work any more.

Hamish

[Hamish McIntyre-Bhatty (hamishmb)]

Note: Looks like this is present in Fedora as well, do I need to report this upstream somehow as well?

[Karl Stenerud (kstenerud)]

Hi Hamish,

Yes, please do follow upstream to find the source of the problem, open a bug there if necessary, and post a link here + whatever other distro is affected. It will make things easier on everyone to coordinate a fix across platforms :)

[Hamish McIntyre-Bhatty (hamishmb)]

Looks like this was fixed in v23.2, despite the misleading title of the bug. I will also notify the Fedora team now.

[Hamish McIntyre-Bhatty (hamishmb)]

Link to the bug: https://gitlab.com/psmisc/psmisc/issues/13

[Robie Basak (racb)]

> This is a regression, and so there would be no regression potential in a fix.

I disagree. The upstream fix involves quite a bit of refactoring in argument passing. A regression in attempt to fix this could manifest in some edge case form of argument to parse incorrectly.

[Robie Basak (racb)]

> in argument passing

parsing

[Christian Ehrhardt  (paelzer)]

This is fixed in 23.2-1,
would someone confirm that this fixes all you initial bugs so that we can condier backporting the fix.

BTW: fix at https://gitlab.com/psmisc/psmisc/commit/258ee9166e585f87005d3a9686938a4fa26669f9

Also I agree to @rbasak that this clearly has regression potential, just we'd hope less than what it actually fixes especially for 3rd party scripts of upgraders from 16.04

[Christian Ehrhardt  (paelzer)]

All lower case i dangerous, as it actually (can) mean the lower case options.
So for example

-int means "interactive" and it will ask
  + killall -int testINT
  + /tmp/testINT /dev/urandom
  Kill testINT(4931) ? (y/N)

But the upper cases should work as specified.
Just the text is misleading it is not "when specified in capitals" as -int != -INT

But it is true that some options do not work.

Test:
#!/bin/bash
#set -x
TNAME="uninitialized"

testkillall () {
        local tsig=${1}
        /tmp/${TNAME} /dev/urandom &
        local tpid=$!;
        echo "TEST -${tsig}";
        killall -${tsig} "${TNAME}"
        kill -9 "${tpid}" 2>/dev/null;
        kill -9 "${TNAME}" 2>/dev/null;
}

for sig in $(killall -l | xargs); do
        TNAME="test${sig}"
        cp /usr/bin/md5sum "/tmp/${TNAME}"
        testkillall "$(echo ${sig} | tr '[:lower:]' '[:upper:]')"
        testkillall "SIG$(echo ${sig} | tr '[:lower:]' '[:upper:]')"
        #testkillall "$(echo ${sig} | tr '[:upper:]' '[:lower:]')"
        rm -f "/tmp/${TNAME}"
        sync
done

Results:
- Disco with 23.2-1 all good
- Cosmic with 23.1-1 broken on: INT + ILL + VTALRM

[Christian Ehrhardt  (paelzer)]

To fix it we also need which modify arg parsing together:
cf80d70 killall: Fix -INT option parsing
b769da1 killall: change to getopt_long
and the already identified
258ee91 killall: Another go at option parsing

And there is the lovely
1e2f38a2 killall: increase comm len to 64, fix indenting
(of course you'd combine a global indenting change with an actual change)

[Christian Ehrhardt  (paelzer)]

Experimental PPA at https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3561/+packages
MPs for packaging to review at
- https://code.launchpad.net/~paelzer/ubuntu/+source/psmisc/+git/psmisc/+merge/360687
- https://code.launchpad.net/~paelzer/ubuntu/+source/psmisc/+git/psmisc/+merge/360686

I'll need to check if build and tests works on that.
If it does I'll ping here and ask for your verification on the PPA as well if possible.
Then I'll open up the MPs for review to then finally upload it to the SRU process for Bionic&Cosmic

[Christian Ehrhardt  (paelzer)]

With the fixes in the PPA I can pass the test just fine on Bionic and Cosmic.
I think the fix is good and also worth especially for upgraders from xenial (I tested the same, it is good there).

Opening up the MPs for review.

[Christian Ehrhardt  (paelzer)]

Uploaded to Bionic and Cosmic for SRU processing.

[Brian Murray (brian-murray)]

Hello Hamish, or anyone else affected,

Accepted psmisc into cosmic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/psmisc/23.1-1ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-cosmic to verification-done-cosmic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-cosmic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Brian Murray (brian-murray)]

Hello Hamish, or anyone else affected,

Accepted psmisc into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/psmisc/23.1-1ubuntu0.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Hamish McIntyre-Bhatty (hamishmb)]

Noted that the lower-case version is not equivalent.

Just tested, and working in bionic - the script in the description works successfully for all options.

Now testing in cosmic.

[Hamish McIntyre-Bhatty (hamishmb)]

The script is passing in cosmic as well.

Changing verification-needed-bionic and verification-needed-cosmic to verification-done-bionic and verification-done-cosmic.

I'm leaving the original verification-needed tag for the moment, in case anyone else wants to test / extra testing is needed.

[Launchpad Janitor (janitor)]

This bug was fixed in the package psmisc - 23.1-1ubuntu1.1

---------------
psmisc (23.1-1ubuntu1.1) cosmic; urgency=medium

  * fix killall option parsing (LP: #1806060)
    - d/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch
    - d/patches/lp1806060-0002-killall-change-to-getopt_long.patch
    - d/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch

 -- Christian Ehrhardt <email address hidden> Tue, 11 Dec 2018 16:46:04 +0100

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for psmisc has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Christian Ehrhardt  (paelzer)]

@SRU Team on Bionic there are some test failures, but GVFS was a flaky test (thanks for rerunning that already Lukasz and mariadb is just prema-failing unrelated to this upload.
So I'd ask you to also release the Bionic portion of this SRU.

[Launchpad Janitor (janitor)]

This bug was fixed in the package psmisc - 23.1-1ubuntu0.1

---------------
psmisc (23.1-1ubuntu0.1) bionic; urgency=medium

  * fix killall option parsing (LP: #1806060)
    - d/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch
    - d/patches/lp1806060-0002-killall-change-to-getopt_long.patch
    - d/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch

 -- Christian Ehrhardt <email address hidden> Tue, 11 Dec 2018 16:46:04 +0100

[Hamish McIntyre-Bhatty (hamishmb)]

Tested on my production system (Mint 19.1 aka Bionic), and the test script provided in the description is now working fine.