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

Bug #1806060 reported by Hamish McIntyre-Bhatty on 2018-11-30
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
psmisc (Ubuntu)
High
Unassigned
Bionic
High
Christian Ehrhardt 
Cosmic
High
Christian Ehrhardt 

Bug Description

[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

Related branches

Changed in psmisc (Ubuntu):
status: New → Confirmed
description: updated
Changed in psmisc (Ubuntu):
importance: Undecided → High
tags: added: server-next

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

Karl Stenerud (kstenerud) wrote :

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 :)

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

Robie Basak (racb) wrote :

> 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) wrote :

> in argument passing

parsing

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

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

Changed in psmisc (Ubuntu):
status: Confirmed → Fix Released
Changed in psmisc (Ubuntu Bionic):
status: New → Confirmed
Changed in psmisc (Ubuntu Cosmic):
status: New → Confirmed
description: updated

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)

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

Changed in psmisc (Ubuntu Bionic):
status: Confirmed → Triaged
Changed in psmisc (Ubuntu Cosmic):
status: Confirmed → Triaged
description: updated

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.

Changed in psmisc (Ubuntu Bionic):
status: Triaged → In Progress
Changed in psmisc (Ubuntu Cosmic):
status: Triaged → In Progress
Changed in psmisc (Ubuntu Bionic):
importance: Undecided → High
Changed in psmisc (Ubuntu Cosmic):
importance: Undecided → High
Changed in psmisc (Ubuntu Bionic):
assignee: nobody → Christian Ehrhardt  (paelzer)
Changed in psmisc (Ubuntu Cosmic):
assignee: nobody → Christian Ehrhardt  (paelzer)

Uploaded to Bionic and Cosmic for SRU processing.

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.

Changed in psmisc (Ubuntu Cosmic):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-cosmic
Changed in psmisc (Ubuntu Bionic):
status: In Progress → Fix Committed
tags: added: verification-needed-bionic
Brian Murray (brian-murray) wrote :

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.

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.

tags: added: verification-done-bionic
removed: verification-needed-bionic

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.

tags: added: verification-done-cosmic
removed: verification-needed-cosmic
Launchpad Janitor (janitor) wrote :

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

Changed in psmisc (Ubuntu Cosmic):
status: Fix Committed → Fix Released

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.

@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) wrote :

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

Changed in psmisc (Ubuntu Bionic):
status: Fix Committed → Fix Released

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

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers