Merge lp:~jamesodhunt/upstart/correct-job_process_spawn-error-handling into lp:upstart

Proposed by James Hunt
Status: Merged
Merged at revision: 1374
Proposed branch: lp:~jamesodhunt/upstart/correct-job_process_spawn-error-handling
Merge into: lp:upstart
Diff against target: 188 lines (+55/-18)
2 files modified
init/job_process.c (+50/-17)
init/job_process.h (+5/-1)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/correct-job_process_spawn-error-handling
Reviewer Review Type Date Requested Status
Steve Langasek Approve
Review via email: mp+117837@code.launchpad.net

Description of the change

* job_process_spawn(): Corrected error handling in child where an error
  condition could result in an assertion failure due to failure to raise
  a system error or invalid use of nih_return_system_error()
  (LP: #1032101).

To post a comment you must log in.
Revision history for this message
Steve Langasek (vorlon) wrote :

Looks good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'init/job_process.c'
2--- init/job_process.c 2012-03-16 21:02:13 +0000
3+++ init/job_process.c 2012-08-02 08:59:17 +0000
4@@ -584,29 +584,35 @@
5 sigemptyset (&ignore.sa_mask);
6
7 if (sigaction (SIGCHLD, &ignore, &act) < 0) {
8- job_process_error_abort (fds[1], JOB_PROCESS_ERROR_OPENPT_MASTER, 0);
9+ nih_error_raise_system ();
10+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_SIGNAL, 0);
11 }
12
13 if (grantpt (pty_master) < 0) {
14- job_process_error_abort (fds[1], JOB_PROCESS_ERROR_OPENPT_MASTER, 0);
15+ nih_error_raise_system ();
16+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GRANTPT, 0);
17 }
18
19 /* Restore child handler */
20 if (sigaction (SIGCHLD, &act, NULL) < 0) {
21- job_process_error_abort (fds[1], JOB_PROCESS_ERROR_OPENPT_MASTER, 0);
22+ nih_error_raise_system ();
23+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_SIGNAL, 0);
24 }
25
26 if (unlockpt (pty_master) < 0) {
27+ nih_error_raise_system ();
28 job_process_error_abort (fds[1], JOB_PROCESS_ERROR_UNLOCKPT, 0);
29 }
30
31 if (ptsname_r (pty_master, pts_name, sizeof(pts_name)) < 0) {
32+ nih_error_raise_system ();
33 job_process_error_abort (fds[1], JOB_PROCESS_ERROR_PTSNAME, 0);
34 }
35
36 pty_slave = open (pts_name, O_RDWR | O_NOCTTY);
37
38 if (pty_slave < 0) {
39+ nih_error_raise_system ();
40 job_process_error_abort (fds[1], JOB_PROCESS_ERROR_OPENPT_SLAVE, 0);
41 }
42
43@@ -618,8 +624,10 @@
44 */
45 if ((script_fd != -1) && (script_fd != JOB_PROCESS_SCRIPT_FD)) {
46 int tmp = dup2 (script_fd, JOB_PROCESS_SCRIPT_FD);
47- if (tmp < 0)
48+ if (tmp < 0) {
49+ nih_error_raise_system ();
50 job_process_error_abort (fds[1], JOB_PROCESS_ERROR_DUP, 0);
51+ }
52 close (script_fd);
53 script_fd = tmp;
54 }
55@@ -658,22 +666,23 @@
56 * session in the chroot via D-Bus, so disallow
57 * all jobs in such an environment.
58 */
59- nih_return_error (-1, EPERM, "user jobs not supported in chroots");
60+ nih_error_raise (EPERM, "user jobs not supported in chroots");
61+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_CHROOT, 0);
62 }
63
64 pw = getpwuid (uid);
65
66- if (!pw)
67- nih_return_system_error (-1);
68+ if (!pw) {
69+ nih_error_raise_system ();
70+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETPWUID, 0);
71+ }
72
73 nih_assert (pw->pw_uid == uid);
74
75 if (! pw->pw_dir) {
76- nih_local char *message = NIH_MUST (nih_sprintf (NULL,
77- "no home directory for user with uid %d",
78- uid));
79-
80- nih_return_error (-1, ENOENT, message);
81+ nih_error_raise_printf (ENOENT,
82+ "no home directory for user with uid %d", uid);
83+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_GETPWUID, 0);
84
85 }
86
87@@ -682,8 +691,10 @@
88 */
89 user_dir = nih_strdup (NULL, pw->pw_dir);
90
91- if (! user_dir)
92- nih_return_no_memory_error (-1);
93+ if (! user_dir) {
94+ nih_error_raise_no_memory ();
95+ job_process_error_abort (fds[1], JOB_PROCESS_ERROR_ALLOC, 0);
96+ }
97
98 /* Ensure the file associated with fd 9
99 * (/proc/self/fd/9) is owned by the user we're about to
100@@ -1126,6 +1137,11 @@
101 err, _("unable to getgrnam: %s"),
102 strerror (err->errnum)));
103 break;
104+ case JOB_PROCESS_ERROR_GETPWUID:
105+ err->error.message = NIH_MUST (nih_sprintf (
106+ err, _("unable to getpwuid: %s"),
107+ strerror (err->errnum)));
108+ break;
109 case JOB_PROCESS_ERROR_BAD_SETUID:
110 err->error.message = NIH_MUST (nih_sprintf (
111 err, _("unable to find setuid user")));
112@@ -1151,7 +1167,7 @@
113 break;
114 case JOB_PROCESS_ERROR_OPENPT_MASTER:
115 err->error.message = NIH_MUST (nih_sprintf (
116- err, _("unable to open pt master: %s"),
117+ err, _("unable to open pty master: %s"),
118 strerror (err->errnum)));
119 break;
120 case JOB_PROCESS_ERROR_UNLOCKPT:
121@@ -1159,6 +1175,11 @@
122 err, _("unable to unlockpt: %s"),
123 strerror (err->errnum)));
124 break;
125+ case JOB_PROCESS_ERROR_GRANTPT:
126+ err->error.message = NIH_MUST (nih_sprintf (
127+ err, _("unable to granpt: %s"),
128+ strerror (err->errnum)));
129+ break;
130 case JOB_PROCESS_ERROR_PTSNAME:
131 err->error.message = NIH_MUST (nih_sprintf (
132 err, _("unable to get ptsname: %s"),
133@@ -1166,7 +1187,17 @@
134 break;
135 case JOB_PROCESS_ERROR_OPENPT_SLAVE:
136 err->error.message = NIH_MUST (nih_sprintf (
137- err, _("unable to open pt slave: %s"),
138+ err, _("unable to open pty slave: %s"),
139+ strerror (err->errnum)));
140+ break;
141+ case JOB_PROCESS_ERROR_SIGNAL:
142+ err->error.message = NIH_MUST (nih_sprintf (
143+ err, _("unable to modify signal handler: %s"),
144+ strerror (err->errnum)));
145+ break;
146+ case JOB_PROCESS_ERROR_ALLOC:
147+ err->error.message = NIH_MUST (nih_sprintf (
148+ err, _("unable to allocate memory: %s"),
149 strerror (err->errnum)));
150 break;
151 default:
152@@ -2162,8 +2193,10 @@
153 return;
154
155 new = dup (*fd);
156- if (new < 0)
157+ if (new < 0) {
158+ nih_error_raise_system ();
159 job_process_error_abort (error_fd, JOB_PROCESS_ERROR_DUP, 0);
160+ }
161 close (*fd);
162 *fd = new;
163 }
164
165=== modified file 'init/job_process.h'
166--- init/job_process.h 2012-03-07 12:13:28 +0000
167+++ init/job_process.h 2012-08-02 08:59:17 +0000
168@@ -82,6 +82,7 @@
169 JOB_PROCESS_ERROR_EXEC,
170 JOB_PROCESS_ERROR_GETPWNAM,
171 JOB_PROCESS_ERROR_GETGRNAM,
172+ JOB_PROCESS_ERROR_GETPWUID,
173 JOB_PROCESS_ERROR_BAD_SETUID,
174 JOB_PROCESS_ERROR_BAD_SETGID,
175 JOB_PROCESS_ERROR_SETUID,
176@@ -89,8 +90,11 @@
177 JOB_PROCESS_ERROR_CHOWN,
178 JOB_PROCESS_ERROR_OPENPT_MASTER,
179 JOB_PROCESS_ERROR_UNLOCKPT,
180+ JOB_PROCESS_ERROR_GRANTPT,
181 JOB_PROCESS_ERROR_PTSNAME,
182- JOB_PROCESS_ERROR_OPENPT_SLAVE
183+ JOB_PROCESS_ERROR_OPENPT_SLAVE,
184+ JOB_PROCESS_ERROR_SIGNAL,
185+ JOB_PROCESS_ERROR_ALLOC
186 } JobProcessErrorType;
187
188 /**

Subscribers

People subscribed via source and target branches