apport uses sys.argv instead of named arguments

Bug #1732962 reported by Brian Murray
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Apport
Fix Released
Low
Julian Andres Klode
apport (Ubuntu)
Fix Released
Low
Unassigned
Trusty
Won't Fix
Undecided
Unassigned
Xenial
Fix Released
Undecided
Unassigned
Bionic
Fix Released
Undecided
Unassigned
Cosmic
Won't Fix
Undecided
Unassigned
Disco
Fix Released
Undecided
Unassigned

Bug Description

SRU Description

[Impact]
data/apport which processes core files expects a certain quantity of arguments in a specific order. This ended up causing an issue with some security updates where we were trying to support a new version of apport on a host system and one inside a container.
This SRU for xenial and bionic based on the work made in cosmic enabled proper handling of named argument.
Note that this is disabled for now on ALL series

[Test Case]
No real test here since apport general behavior should be unchanged Just to check that the feature is disable, /proc/sys/kernel/core_pattern
content should remain unchanged :

$> cat /proc/sys/kernel/core_pattern
|/usr/share/apport/apport %p %s %c %d %P %E

[Regression Potential]
The new feature is not enabled so the regression risk is fairly low. this will take place in a future coordinated SRU across all LTS but in the meanwhile we can make sure that there's no regression by making sure apport still behave properly. starting and killing xeyes should trigger apport normal operation and start apport.
$> xeyes &
$> kill -SEGV $!

End SRU Description

data/apport which processes core files expects a certain quantity of arguments in a specific order. This ended up causing an issue with some security updates where we were trying to support a new version of apport on a host system and one inside a container. Here's something of an example:

347 # Normal startup
348 if len(sys.argv) not in (5, 6):
349 try:
350 print('Usage: %s <pid> <signal number> <core file ulimit> <dump mode> [global pid]' % sys.argv[0])
351 print('The core dump is read from stdin.')

We could not maintain backwards compatibility because "global pid" is an optional argument and "dump mode" was a new argument. So if there were five arguments its possible the last one was "dump mode" (no global pid) or "global pid" (no support for dump mode).

Its possible to use strings in /proc/sys/kernel/core_pattern so we could use those and have apport accept named arguments e.g:

$ cat /proc/sys/kernel/core_pattern
|/usr/share/apport/apport --pid=%p --signal=%s --core-size=%c --dump-mode=%d --global-pid=%P

['/home/bdmurray/source-trees/apport/artful/data/apport', '--pid=5870', '--signal=11', '--core-size=0', '--dump-mode=1', '--global-pid=5870']

Tyler said "that's probably a nice cleanup to make no matter what because the magic arg ordering is dangerous".

Related branches

Changed in apport:
importance: Undecided → Low
status: New → In Progress
assignee: nobody → Julian Andres Klode (juliank)
Revision history for this message
Julian Andres Klode (juliank) wrote :

Um, we can do that. But I think there's a problem with containers and compatibility in forwarding messages over the socket (think new container, old host).

I wonder if /proc/sys/kernel/core_pattern is per-namespace too. If it is, that would be great. If not, a new host with old containers would break too: The containers would not understand the arguments. Specifically, containers need to get the host_pid in sys.argv[5], they do not care about the other arguments, so these could be -- options like that:

--pid=%p --signal=%s --core-size=%c --dump-mode=%d %P

but we could not add more options then either, because the forwarding only works with 6 arguments.

Revision history for this message
Julian Andres Klode (juliank) wrote :

So the situation is:

- We need to keep the argument count at 5-6 no matter what, otherwise old containers in new hosts break
- In containers, we do not parse any arguments except for 5.
- The core_pattern is defined on the host.

I suggest we just split the command-line by "," as well. So if we have

--pid=%p --signal=%s --core-size=%c --dump-mode=%d %P

we can extend that to

--pid=%p --signal=%s --core-size=%c --dump-mode=%d,foo-bar=%d %P

or we just drop the -- alltogether:

pid=%p signal=%s core-size=%c dump-mode=%d,foo-bar=%? %P

That should work:

host=old, container=old => everything as before
host=old, container=new => core_pattern as before, no effective changes
host=new, container=old => new core_pattern, container just forwards to host, host does parsing
host=new, container=new => fine too

Revision history for this message
Julian Andres Klode (juliank) wrote :

Let's just generalize that into

--version2 --version2 --variables pid=%p,.... %P

Revision history for this message
Brian Murray (brian-murray) wrote :

I'm sorry I think I lost something here. What does --version2 mean exactly?

Revision history for this message
Julian Andres Klode (juliank) wrote :

It's just a silly fill up argument so we end up with 5/6 arguments to make old clients in containers happy. And well, it easily allows you to distinguish the new commandline from the old positional style :)

Revision history for this message
Julian Andres Klode (juliank) wrote :
Revision history for this message
Julian Andres Klode (juliank) wrote :

OK, so that's not all: core_pattern itself only stores 128 bytes.

tags: added: id-5a0f2a3d5a9357e93d7f6816
Revision history for this message
Julian Andres Klode (juliank) wrote :

Just to summarize the situation: It seems that if you have apport installed inside an LXC container, that apport creates reports, rather than doing the forwarding dance. Unfortunately, the core_pattern does not know about name spaces.

So if we change the host to use a new pattern with named arguments, older apport in containers is broken - and needs to be SRUed. So this needs some sort of flag day transition. To minimize the impact of the transition, a good strategy seems to be the following:

1. Upload apport that understands named arguments and SRU it everywhere
2. Wait some time
3. Change core_pattern in devel
4. Wait some time
5. Change core_pattern everywhere else

Right?

Revision history for this message
Brian Murray (brian-murray) wrote :

Given what Tyler said about "magic arg ordering being dangerous" I wonder if we could instead do that is a security update simultaneously for all releases thereby avoiding the multiple uploads and flag day transition.

Revision history for this message
Julian Andres Klode (juliank) wrote :

If we do that, I'd say we built them as security updates, copy them to proposed, and then once they are fit for release, copy them to the -security in addition to -updates; so we get the usual SRU checks.

I still think that at least rolling out support first via an SRU makes sense.

Changed in apport (Ubuntu):
status: New → Fix Committed
Changed in apport:
status: In Progress → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apport - 2.20.10-0ubuntu23

---------------
apport (2.20.10-0ubuntu23) disco; urgency=medium

  * Fix python coding style issue introduced in previous upload.

 -- Julian Andres Klode <email address hidden> Tue, 05 Mar 2019 10:35:17 +0100

Changed in apport (Ubuntu Disco):
status: Fix Committed → Fix Released
Changed in apport (Ubuntu Cosmic):
status: New → Won't Fix
Changed in apport (Ubuntu Trusty):
status: New → Fix Committed
status: Fix Committed → Won't Fix
Changed in apport (Ubuntu Xenial):
status: New → In Progress
Changed in apport (Ubuntu Bionic):
status: New → In Progress
description: updated
Revision history for this message
Brian Murray (brian-murray) wrote :

I'll go ahead and sponsor these today.

Revision history for this message
Matthieu Clemenceau (mclemenceau) wrote :
Revision history for this message
Matthieu Clemenceau (mclemenceau) wrote :
Revision history for this message
Matthieu Clemenceau (mclemenceau) wrote :

Updated debdiff and made sure autopkgtest were successful on both xenial and bionic

Revision history for this message
Brian Murray (brian-murray) wrote :

Sponsored for bionic.

Revision history for this message
Brian Murray (brian-murray) wrote :

Also sponsored for xenial.

description: updated
description: updated
Revision history for this message
Timo Aaltonen (tjaalton) wrote : Please test proposed package

Hello Brian, or anyone else affected,

Accepted apport into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/apport/2.20.9-0ubuntu7.18 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, what testing has been performed on the package 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.

Changed in apport (Ubuntu Bionic):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-bionic
Changed in apport (Ubuntu Xenial):
status: In Progress → Fix Committed
tags: added: verification-needed-xenial
Revision history for this message
Timo Aaltonen (tjaalton) wrote :

Hello Brian, or anyone else affected,

Accepted apport into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/apport/2.20.1-0ubuntu2.25 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, what testing has been performed on the package and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. 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.

Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (apport/2.20.9-0ubuntu7.18)

All autopkgtests for the newly accepted apport (2.20.9-0ubuntu7.18) for bionic have finished running.
The following regressions have been reported in tests triggered by the package:

apport/2.20.9-0ubuntu7.18 (amd64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/bionic/update_excuses.html#apport

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Matthieu Clemenceau (mclemenceau) wrote :

Verification completed on both bionic and xenial using proposed pockets

tags: added: verification-done verification-done-bionic verification-done-xenial
removed: verification-needed verification-needed-bionic verification-needed-xenial
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apport - 2.20.1-0ubuntu2.25

---------------
apport (2.20.1-0ubuntu2.25) xenial; urgency=medium

  * data/apport: Introduce support for non-positional arguments so we can
    easily extend core_pattern in the future (LP: #1732962)

 -- Matthieu Clemenceau <email address hidden> Fri, 21 Aug 2020 10:24:13 -0500

Changed in apport (Ubuntu Xenial):
status: Fix Committed → Fix Released
Revision history for this message
Brian Murray (brian-murray) wrote : Update Released

The verification of the Stable Release Update for apport has completed successfully and the package is now being 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.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apport - 2.20.9-0ubuntu7.18

---------------
apport (2.20.9-0ubuntu7.18) bionic; urgency=medium

  * data/apport: Introduce support for non-positional arguments so we can
    easily extend core_pattern in the future (LP: #1732962)

 -- Matthieu Clemenceau <email address hidden> Fri, 21 Aug 2020 10:12:55 -0500

Changed in apport (Ubuntu Bionic):
status: Fix Committed → Fix Released
Benjamin Drung (bdrung)
Changed in apport:
milestone: none → 2.21.0
status: Fix Committed → Fix Released
Benjamin Drung (bdrung)
Changed in apport (Ubuntu):
importance: Undecided → Low
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.