Merge lp:~ted/ubuntu-app-launch/process-group-kill into lp:ubuntu-app-launch/14.04

Proposed by Ted Gould on 2014-04-11
Status: Merged
Approved by: Oliver Grawert on 2014-04-11
Approved revision: 148
Merged at revision: 143
Proposed branch: lp:~ted/ubuntu-app-launch/process-group-kill
Merge into: lp:ubuntu-app-launch/14.04
Diff against target: 26 lines (+10/-2)
1 file modified
upstart-jobs/application-click.conf.in (+10/-2)
To merge this branch: bzr merge lp:~ted/ubuntu-app-launch/process-group-kill
Reviewer Review Type Date Requested Status
Oliver Grawert 2014-04-11 Approve on 2014-04-11
PS Jenkins bot (community) continuous-integration Approve on 2014-04-11
Review via email: mp+215475@code.launchpad.net

Commit message

Kill all jobs in process group on exit

Description of the change

After starting record the PID so that we can ensure the PID's process group gets destroyed using pkill after the job stops. A big stick, but insurance.

To post a comment you must log in.
148. By Ted Gould on 2014-04-11

Forgot to move over the kill

Oliver Grawert (ogra) wrote :

tested, works around the issues in bug 1303676

review: Approve
Tyler Hicks (tyhicks) wrote :

While this should work for the purposes of bug 1303676, lets be sure we don't depend, from a security standpoint, on this change to kill all processes spawned by an application. An application could create another process group and move all of its processes to that group and this change would not kill those processes.

I don't think the intent of this change is to fully clean up, with 100% assurance, after an application. As long as that's the case, it looks good to me! :)

Oliver Grawert (ogra) wrote :

This change is solely to make the phone image releaseable, we shoulld definitely go on fixing the issue properly.

Ted Gould (ted) wrote :

On Fri, 2014-04-11 at 23:05 +0000, Tyler Hicks wrote:

> While this should work for the purposes of bug 1303676, lets be sure we don't depend, from a security standpoint, on this change to kill all processes spawned by an application. An application could create another process group and move all of its processes to that group and this change would not kill those processes.
>
> I don't think the intent of this change is to fully clean up, with 100% assurance, after an application. As long as that's the case, it looks good to me! :)

Yes, that isn't the intent. The question would be whether we pursue a
cgroups based solution for Upstart or wait to implement it on systemd.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'upstart-jobs/application-click.conf.in'
2--- upstart-jobs/application-click.conf.in 2014-02-13 22:43:55 +0000
3+++ upstart-jobs/application-click.conf.in 2014-04-11 18:33:36 +0000
4@@ -12,6 +12,7 @@
5 env APP_DIR
6 # For Surface Flinger
7 env APP_DESKTOP_FILE
8+env APP_PROCESS_GROUP
9
10 env UPSTART_APP_LAUNCH_ARCH="@upstart_app_launch_arch@"
11 export UPSTART_APP_LAUNCH_ARCH
12@@ -29,5 +30,12 @@
13 # Remember, this is confined
14 exec @pkglibexecdir@/exec-line-exec
15
16-post-start exec @pkglibexecdir@/zg-report-app open
17-post-stop exec @pkglibexecdir@/zg-report-app close
18+post-start script
19+ initctl set-env APP_PROCESS_GROUP=`upstart-app-pid ${APP_ID}`
20+ @pkglibexecdir@/zg-report-app open
21+end script
22+
23+post-stop script
24+ pkill -KILL -g "${APP_PROCESS_GROUP}" || true
25+ @pkglibexecdir@/zg-report-app close
26+end script

Subscribers

People subscribed via source and target branches