Merge lp:~jamesodhunt/upstart/bug-1222705-reprise into lp:upstart

Proposed by James Hunt on 2014-07-16
Status: Merged
Merged at revision: 1656
Proposed branch: lp:~jamesodhunt/upstart/bug-1222705-reprise
Merge into: lp:upstart
Diff against target: 304 lines (+45/-86)
3 files modified
ChangeLog (+8/-0)
init/environ.c (+12/-14)
init/tests/test_environ.c (+25/-72)
To merge this branch: bzr merge lp:~jamesodhunt/upstart/bug-1222705-reprise
Reviewer Review Type Date Requested Status
Colin Watson (community) 2014-07-16 Approve on 2014-07-16
Dimitri John Ledkov 2014-07-16 Needs Information on 2014-07-16
Steve Langasek please review memoves against (potentially) nih_alloced char** array 2014-07-16 Pending
Review via email: mp+226983@code.launchpad.net
To post a comment you must log in.
Dimitri John Ledkov (xnox) wrote :

See inline comment about memove call.

review: Needs Information
Colin Watson (cjwatson) wrote :

memcpy is unsafe for that, but the entire point of memmove is to be safe in this situation.

Dimitri John Ledkov (xnox) wrote :

memmove's are all good. Not sure if it's safe to use them against nih_alloced objects, given they have NihAllocCtx after them. Someone else should review this bit. See also the inline comment.

review: Needs Information
1657. By James Hunt on 2014-07-16

* init/environ.c: environ_remove(): Shrink memory to fit what we're
  actually using.
* init/tests/test_environ.c: test_remove(): Remove now-invalid ENOMEM
  checks.

James Hunt (jamesodhunt) wrote :

It's safe because all the environ_*() calls operate on the result of nih_str_array_new(). That function will return a block of memory big enough to hold a single 'char *' pointer, and set it to NULL. The NihAllocCtx is situated _below_ the return value of nih_str_array_new() in memory and that single NihAllocCtx will be used to keep track of the size and child allocations of the entire array (should it grow bigger than its current size of zero (the terminator is not counted)).

Subsequent to nih_str_array_new() being called, that zero-sized array will be increased in size by calls to environ_add(), which in turn calls nih_str_array_addp(). That latter function then calls nih_realloc() to increase the size of the single NihAllocCtx previously mentioned.

So, if the array contains pointers to 10 env vars, it has size 11 (10 + 1 for the NULL terminator), and has a single NihAllocCtx just before the zeroth element. So it's actual byte size is:

  sizeof (NihAllocCtx) + (sizeof (char *) * (number_of_elements + 1))

As such, it is safe to call memmove to shuffle those pointers since they are the user-accessible way to reference the strings the array points to - Nih keeps track itself using the NihAllocCtx object.

I've added a call to nih_relloc() to compact the size of the array (from NIH's perspective) as it is wasteful (but not actually incorrect) not to call nih_realloc() and leave 'sizeof (char *)' bytes lying around until the array eventually gets deallocated.

Colin Watson (cjwatson) wrote :

I agree with your analysis re NihAllocCtx. This looks right, and the form of the new code matches a similar section from environ_add, so that's good. Just one style comment.

review: Approve
James Hunt (jamesodhunt) wrote :

To be convinced this fix works:

$ mkdir /tmp/conf /tmp/log
$ cat >>/tmp/conf/bug-1222705.conf<<EOT
start on startup

exec sleep 999
post-stop exec initctl unset-env wibble
EOT
$ gdb --args init/init --confdir /tmp/conf --logdir /tmp/log --debug
(gdb) run

In separate window:

$ initctl list-sessions
$ UPSTART_SESSION=unix:abstract=/com/ubuntu/upstart-session/$uid/$pid restart bug-1222705

1658. By James Hunt on 2014-07-16

* init/environ.c: environ_remove(): Style tweak courtesy of cjwatson's
  review.

James Hunt (jamesodhunt) wrote :

Thanks - style tweak applied.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ChangeLog'
2--- ChangeLog 2014-07-11 21:32:38 +0000
3+++ ChangeLog 2014-07-16 15:55:55 +0000
4@@ -1,3 +1,11 @@
5+2014-07-16 James Hunt <james.hunt@ubuntu.com>
6+
7+ * init/environ.c: environ_remove():
8+ - Ensure removed entry is deref'd (LP: #1222705).
9+ - Avoid recreating array.
10+ * init/tests/test_environ.c: test_remove(): Add checks to ensure
11+ removed entry freed as expected.
12+
13 2014-07-11 James Hunt <james.hunt@ubuntu.com>
14
15 * NEWS: Release 1.13
16
17=== modified file 'init/environ.c'
18--- init/environ.c 2013-10-24 13:33:05 +0000
19+++ init/environ.c 2014-07-16 15:55:55 +0000
20@@ -176,9 +176,7 @@
21 {
22 size_t _len;
23 size_t keylen;
24- size_t new_len = 0;
25 char **e;
26- char **new_env;
27
28 nih_assert (env);
29 nih_assert (str);
30@@ -195,10 +193,6 @@
31 if (*len < 1)
32 return NULL;
33
34- new_env = nih_str_array_new (NULL);
35- if (! new_env)
36- return NULL;
37-
38 for (e = *env; e && *e; e++) {
39 keylen = strcspn (*e, "=");
40
41@@ -206,17 +200,21 @@
42 * name=value pair, or a bare name), so don't copy it to
43 * the new environment.
44 */
45- if (! strncmp (str, *e, keylen))
46- continue;
47-
48- if (! environ_add (&new_env, parent, &new_len, TRUE, *e))
49- return NULL;
50+ if (! strncmp (str, *e, keylen)) {
51+ nih_unref (*e, *env);
52+
53+ /* shuffle up the remaining entries */
54+ memmove (e, e + 1, (char *)(*env + *len) - (char *)e);
55+
56+ (*len)--;
57+ }
58 }
59
60- *env = new_env;
61- *len = new_len;
62+ /* shrink amount of memory used */
63+ if (! nih_realloc (*env, parent, sizeof (**env) * (*len + 1)))
64+ return NULL;
65
66- return new_env;
67+ return *env;
68 }
69
70 /**
71
72=== modified file 'init/tests/test_environ.c'
73--- init/tests/test_environ.c 2013-10-24 13:33:05 +0000
74+++ init/tests/test_environ.c 2014-07-16 15:55:55 +0000
75@@ -618,8 +618,10 @@
76 void
77 test_remove (void)
78 {
79- char **env = NULL, **ret;
80- size_t len = 0;
81+ char **env = NULL;
82+ char *removed = NULL;
83+ char **ret = NULL;
84+ size_t len = 0;
85
86 TEST_FUNCTION ("environ_remove");
87
88@@ -688,6 +690,9 @@
89 TEST_ALLOC_PARENT (env[0], env);
90 TEST_ALLOC_SIZE (env[0], 8);
91 TEST_EQ_STR (env[0], "FOO=BAR");
92+ removed = env[0];
93+ TEST_FREE_TAG (removed);
94+
95 TEST_EQ_P (env[1], NULL);
96 }
97
98@@ -695,17 +700,6 @@
99
100 if (test_alloc_failed) {
101 TEST_EQ_P (ret, NULL);
102-
103- TEST_EQ (len, 1);
104-
105- TEST_ALLOC_PARENT (env[0], env);
106- TEST_ALLOC_SIZE (env[0], 8);
107-
108- TEST_NE_P (env[0], NULL);
109- TEST_EQ_STR (env[0], "FOO=BAR");
110-
111- TEST_EQ_P (env[1], NULL);
112-
113 nih_free (env);
114 continue;
115 }
116@@ -713,6 +707,7 @@
117 TEST_NE_P (ret, NULL);
118 TEST_EQ (len, 0);
119 TEST_EQ_P (env[0], NULL);
120+ TEST_FREE (removed);
121
122 nih_free (env);
123 }
124@@ -730,6 +725,9 @@
125 TEST_ALLOC_PARENT (env[0], env);
126 TEST_ALLOC_SIZE (env[0], 8);
127 TEST_EQ_STR (env[0], "FOO=BAR");
128+ removed = env[0];
129+ TEST_FREE_TAG (removed);
130+
131 TEST_EQ_P (env[1], NULL);
132 }
133
134@@ -737,17 +735,6 @@
135
136 if (test_alloc_failed) {
137 TEST_EQ_P (ret, NULL);
138-
139- TEST_EQ (len, 1);
140-
141- TEST_ALLOC_PARENT (env[0], env);
142- TEST_ALLOC_SIZE (env[0], 8);
143-
144- TEST_NE_P (env[0], NULL);
145- TEST_EQ_STR (env[0], "FOO=BAR");
146-
147- TEST_EQ_P (env[1], NULL);
148-
149 nih_free (env);
150 continue;
151 }
152@@ -755,6 +742,7 @@
153 TEST_NE_P (ret, NULL);
154 TEST_EQ (len, 0);
155 TEST_EQ_P (env[0], NULL);
156+ TEST_FREE (removed);
157
158 nih_free (env);
159 }
160@@ -781,6 +769,8 @@
161 TEST_ALLOC_PARENT (env[0], env);
162 TEST_ALLOC_SIZE (env[0], 8);
163 TEST_EQ_STR (env[0], "FOO=BAR");
164+ removed = env[0];
165+ TEST_FREE_TAG (removed);
166
167 TEST_ALLOC_PARENT (env[1], env);
168 TEST_ALLOC_SIZE (env[1], 8);
169@@ -794,18 +784,6 @@
170
171 if (test_alloc_failed) {
172 TEST_EQ_P (ret, NULL);
173-
174- TEST_EQ (len, 2);
175- TEST_ALLOC_PARENT (env[0], env);
176- TEST_ALLOC_SIZE (env[0], 8);
177- TEST_EQ_STR (env[0], "FOO=BAR");
178-
179- TEST_ALLOC_PARENT (env[1], env);
180- TEST_ALLOC_SIZE (env[1], 8);
181- TEST_EQ_STR (env[1], "BAZ=QUX");
182-
183- TEST_EQ_P (env[2], NULL);
184-
185 nih_free (env);
186 continue;
187 }
188@@ -816,6 +794,7 @@
189 TEST_ALLOC_PARENT (env[0], env);
190 TEST_ALLOC_SIZE (env[0], 8);
191 TEST_EQ_STR (env[0], "BAZ=QUX");
192+ TEST_FREE (removed);
193
194 TEST_EQ_P (env[1], NULL);
195
196@@ -852,6 +831,8 @@
197 TEST_ALLOC_PARENT (env[0], env);
198 TEST_ALLOC_SIZE (env[0], 8);
199 TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
200+ removed = env[0];
201+ TEST_FREE_TAG (removed);
202
203 TEST_ALLOC_PARENT (env[1], env);
204 TEST_ALLOC_SIZE (env[1], 8);
205@@ -865,18 +846,6 @@
206
207 if (test_alloc_failed) {
208 TEST_EQ_P (ret, NULL);
209-
210- TEST_EQ (len, 2);
211- TEST_ALLOC_PARENT (env[0], env);
212- TEST_ALLOC_SIZE (env[0], 8);
213- TEST_EQ_STR (env[0], "UPSTART_TEST_VARIABLE=foo");
214-
215- TEST_ALLOC_PARENT (env[1], env);
216- TEST_ALLOC_SIZE (env[1], 8);
217- TEST_EQ_STR (env[1], "BAZ=QUX");
218-
219- TEST_EQ_P (env[2], NULL);
220-
221 nih_free (env);
222 continue;
223 }
224@@ -887,6 +856,7 @@
225 TEST_ALLOC_PARENT (env[0], env);
226 TEST_ALLOC_SIZE (env[0], 8);
227 TEST_EQ_STR (env[0], "BAZ=QUX");
228+ TEST_FREE (removed);
229
230 TEST_EQ_P (env[1], NULL);
231
232@@ -921,6 +891,8 @@
233 TEST_ALLOC_PARENT (env[1], env);
234 TEST_ALLOC_SIZE (env[1], 8);
235 TEST_EQ_STR (env[1], "BAZ=QUX");
236+ removed = env[1];
237+ TEST_FREE_TAG (removed);
238
239 TEST_EQ_P (env[2], NULL);
240 }
241@@ -930,18 +902,6 @@
242
243 if (test_alloc_failed) {
244 TEST_EQ_P (ret, NULL);
245-
246- TEST_EQ (len, 2);
247- TEST_ALLOC_PARENT (env[0], env);
248- TEST_ALLOC_SIZE (env[0], 8);
249- TEST_EQ_STR (env[0], "FOO=BAR");
250-
251- TEST_ALLOC_PARENT (env[1], env);
252- TEST_ALLOC_SIZE (env[1], 8);
253- TEST_EQ_STR (env[1], "BAZ=QUX");
254-
255- TEST_EQ_P (env[2], NULL);
256-
257 nih_free (env);
258 continue;
259 }
260@@ -954,6 +914,7 @@
261 TEST_EQ_STR (env[0], "FOO=BAR");
262
263 TEST_EQ_P (env[1], NULL);
264+ TEST_FREE (removed);
265
266 nih_free (env);
267 }
268@@ -991,6 +952,9 @@
269 /* Should have been expanded */
270 TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
271
272+ removed = env[1];
273+ TEST_FREE_TAG (removed);
274+
275 TEST_EQ_P (env[2], NULL);
276 }
277
278@@ -999,18 +963,6 @@
279
280 if (test_alloc_failed) {
281 TEST_EQ_P (ret, NULL);
282-
283- TEST_EQ (len, 2);
284- TEST_ALLOC_PARENT (env[0], env);
285- TEST_ALLOC_SIZE (env[0], 8);
286- TEST_EQ_STR (env[0], "FOO=BAR");
287-
288- TEST_ALLOC_PARENT (env[1], env);
289- TEST_ALLOC_SIZE (env[1], 8);
290- TEST_EQ_STR (env[1], "UPSTART_TEST_VARIABLE=foo");
291-
292- TEST_EQ_P (env[2], NULL);
293-
294 nih_free (env);
295 continue;
296 }
297@@ -1023,6 +975,7 @@
298 TEST_EQ_STR (env[0], "FOO=BAR");
299
300 TEST_EQ_P (env[1], NULL);
301+ TEST_FREE (removed);
302
303 nih_free (env);
304 }

Subscribers

People subscribed via source and target branches