Merge lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file into lp:ubuntu/vivid/ubuntu-core-upgrader

Proposed by James Hunt
Status: Merged
Merged at revision: 24
Proposed branch: lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file
Merge into: lp:ubuntu/vivid/ubuntu-core-upgrader
Diff against target: 406 lines (+157/-56)
5 files modified
debian/changelog (+9/-0)
functional/test_upgrader.py (+78/-20)
ubuntucoreupgrader/tests/test_upgrader.py (+46/-10)
ubuntucoreupgrader/tests/utils.py (+6/-3)
ubuntucoreupgrader/upgrader.py (+18/-23)
To merge this branch: bzr merge lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/handle-invalid-removed-file
Reviewer Review Type Date Requested Status
Michael Vogt Needs Information
Review via email: mp+254372@code.launchpad.net

Description of the change

* ubuntucoreupgrader/upgrader.py: Tolerate an invalid 'removed' file
  to avoid the upgrade failing attempting to remove '/writable/cache'
  (see LP: #1437225).

To post a comment you must log in.
Michael Vogt (mvo) wrote :

Thanks for adding this bugfix. I think we should also include a regression test.

James Hunt (jamesodhunt) wrote :

Tests added.

Michael Vogt (mvo) wrote :

Thanks for adding the test. It runs fine.

One thing that is a odd is that if I revert the change in r23 (the bugfix for the empty file) and run the tests they still succeed. Is this intended?

James Hunt (jamesodhunt) wrote :

Branch updated. Phew...

Michael Vogt (mvo) wrote :

Thanks, I put some suggestions inline.

26. By James Hunt on 2015-04-08

* Review changes.

James Hunt (jamesodhunt) wrote :

Thanks for reviewing - branch updated.

Michael Vogt (mvo) wrote :

Thanks, looks good, one question about the strip() below seems to have slipped through, would be nice to get this clarified in some way. Otherwise fine and ready to go.

review: Needs Information
Michael Vogt (mvo) wrote :

I merged/resolved-conflicts/uploaded as per Steve.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2015-03-13 08:14:06 +0000
3+++ debian/changelog 2015-04-08 14:54:30 +0000
4@@ -1,3 +1,12 @@
5+ubuntu-core-upgrader (0.7.8) UNRELEASED; urgency=medium
6+
7+ * ubuntucoreupgrader/upgrader.py: Tolerate an invalid 'removed' file
8+ to avoid the upgrade failing attempting to remove '/writable/cache'
9+ (see LP: #1437225).
10+ * functional/test_upgrader.py: Add tests for an invalid removed file.
11+
12+ -- James Hunt <james.hunt@ubuntu.com> Fri, 27 Mar 2015 09:33:15 +0000
13+
14 ubuntu-core-upgrader (0.7.7) vivid; urgency=low
15
16 * fix entry point for s-i 3.0
17
18=== modified file 'functional/test_upgrader.py'
19--- functional/test_upgrader.py 2015-03-04 16:27:30 +0000
20+++ functional/test_upgrader.py 2015-04-08 14:54:30 +0000
21@@ -25,7 +25,6 @@
22 import os
23 import logging
24 import unittest
25-import shutil
26
27 from ubuntucoreupgrader.upgrader import (
28 Upgrader,
29@@ -37,7 +36,6 @@
30 sys.path.append(base_dir)
31
32 from ubuntucoreupgrader.tests.utils import (
33- make_tmp_dir,
34 append_file,
35 TEST_DIR_MODE,
36 create_file,
37@@ -68,21 +66,11 @@
38 args.append(command_file)
39 commands = file_to_list(command_file)
40
41- cache_dir = make_tmp_dir()
42-
43- def mock_get_cache_dir():
44- cache_dir = update.tmp_dir
45- sys_dir = os.path.join(cache_dir, 'system')
46- os.makedirs(sys_dir, exist_ok=True)
47- return cache_dir
48-
49 upgrader = Upgrader(parse_args(args), commands, [])
50- upgrader.get_cache_dir = mock_get_cache_dir
51+ upgrader.cache_dir = root_dir
52 upgrader.MOUNTPOINT_CMD = "true"
53 upgrader.run()
54
55- shutil.rmtree(cache_dir)
56-
57
58 def create_device_file(path, type='c', major=-1, minor=-1):
59 '''
60@@ -216,7 +204,10 @@
61 self.assertTrue(os.path.exists(file_path))
62 self.assertTrue(os.path.isfile(file_path))
63
64- call_upgrader(cmd_file, self.victim_dir, self.update)
65+ # remove 'system' suffix that upgrader will add back on
66+ vdir = os.path.split(self.victim_dir)[0]
67+
68+ call_upgrader(cmd_file, vdir, self.update)
69
70 self.assertFalse(os.path.exists(file_path))
71
72@@ -240,7 +231,8 @@
73 self.assertTrue(os.path.exists(dir_path))
74 self.assertTrue(os.path.isdir(dir_path))
75
76- call_upgrader(cmd_file, self.victim_dir, self.update)
77+ vdir = os.path.split(self.victim_dir)[0]
78+ call_upgrader(cmd_file, vdir, self.update)
79
80 self.assertFalse(os.path.exists(dir_path))
81
82@@ -272,7 +264,8 @@
83 self.assertTrue(os.path.exists(link_file_path))
84 self.assertTrue(os.path.islink(link_file_path))
85
86- call_upgrader(cmd_file, self.victim_dir, self.update)
87+ vdir = os.path.split(self.victim_dir)[0]
88+ call_upgrader(cmd_file, vdir, self.update)
89
90 # original file should still be there
91 self.assertTrue(os.path.exists(src_file_path))
92@@ -310,7 +303,8 @@
93 self.assertTrue(os.path.exists(link_file_path))
94 self.assertTrue(os.path.islink(link_file_path))
95
96- call_upgrader(cmd_file, self.victim_dir, self.update)
97+ vdir = os.path.split(self.victim_dir)[0]
98+ call_upgrader(cmd_file, vdir, self.update)
99
100 # original directory should still be there
101 self.assertTrue(os.path.exists(src_dir_path))
102@@ -352,7 +346,8 @@
103
104 self.assertTrue(src_inode == link_inode)
105
106- call_upgrader(cmd_file, self.victim_dir, self.update)
107+ vdir = os.path.split(self.victim_dir)[0]
108+ call_upgrader(cmd_file, vdir, self.update)
109
110 # original file should still be there
111 self.assertTrue(os.path.exists(src_file_path))
112@@ -392,9 +387,11 @@
113 # device because it won't be :)
114 self.assertTrue(os.path.isfile(file_path))
115
116- call_upgrader(cmd_file, self.victim_dir, self.update)
117+ vdir = os.path.split(self.victim_dir)[0]
118+ call_upgrader(cmd_file, vdir, self.update)
119
120- self.assertFalse(os.path.exists(file_path))
121+ # upgrader doesn't currently remove device files
122+ self.assertTrue(os.path.exists(file_path))
123
124
125 class UpgraderFileAddTestCase(UbuntuCoreUpgraderTestCase):
126@@ -595,6 +592,67 @@
127 self.assertTrue(is_sym_link_broken(victim_link_file_path))
128
129
130+class UpgraderRemoveFileTests(UbuntuCoreUpgraderTestCase):
131+
132+ def common_removed_file_test(self, contents):
133+ '''
134+ Common code to test for an invalid removed file.
135+
136+ The contents parameter specifies the contents of the removed
137+ file.
138+ '''
139+ file = 'created-regular-file'
140+
141+ create_file(self.update.removed_file, contents)
142+
143+ file_path = os.path.join(self.update.system_dir, file)
144+ create_file(file_path, 'foo bar')
145+
146+ archive = self.update.create_archive(self.TARFILE)
147+ self.assertTrue(os.path.exists(archive))
148+
149+ cmd_file = os.path.join(self.update.tmp_dir, CMD_FILE)
150+ make_command_file(cmd_file, [self.TARFILE])
151+
152+ vdir = os.path.split(self.victim_dir)[0]
153+
154+ file_path = os.path.join(vdir, file)
155+ self.assertFalse(os.path.exists(file_path))
156+
157+ # XXX: There is an implicit test here since if the upgrader
158+ # fails (as documented on LP: #1437225), this test will also
159+ # fail.
160+ call_upgrader(cmd_file, vdir, self.update)
161+
162+ self.assertTrue(os.path.exists(vdir))
163+ self.assertTrue(os.path.exists(self.victim_dir))
164+ self.assertTrue(os.path.exists(file_path))
165+
166+ # ensure the empty removed file hasn't removed the directory the
167+ # unpack applies to.
168+ self.assertTrue(self.victim_dir)
169+
170+ def test_removed_file_empty(self):
171+ '''
172+ Ensure the upgrader ignores an empty 'removed' file.
173+ '''
174+ self.common_removed_file_test('')
175+
176+ def test_removed_file_space(self):
177+ '''
178+ Ensure the upgrader handles a 'removed' file containing just a
179+ space.
180+ '''
181+ self.common_removed_file_test(' ')
182+
183+ def test_removed_file_nl(self):
184+ '''
185+ Ensure the upgrader handles a 'removed' file containing just a
186+ newline
187+ '''
188+ self.common_removed_file_test('\n')
189+
190+
191 def main():
192 kwargs = {}
193 format = \
194
195=== modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
196--- ubuntucoreupgrader/tests/test_upgrader.py 2015-03-04 16:50:08 +0000
197+++ ubuntucoreupgrader/tests/test_upgrader.py 2015-04-08 14:54:30 +0000
198@@ -196,15 +196,6 @@
199
200 class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):
201
202- def mock_get_cache_dir(self):
203- '''
204- Mock to allow the tests to control the cache directory location.
205- '''
206- assert(self.cache_dir)
207- self.sys_dir = os.path.join(self.cache_dir, 'system')
208- os.makedirs(self.sys_dir, exist_ok=True)
209- return self.cache_dir
210-
211 def test_create_object(self):
212 root_dir = make_tmp_dir()
213
214@@ -233,7 +224,6 @@
215 options.root_dir = root_dir
216
217 upgrader = Upgrader(options, commands, [])
218- upgrader.get_cache_dir = self.mock_get_cache_dir
219 upgrader.MOUNTPOINT_CMD = "true"
220 upgrader.run()
221
222@@ -242,6 +232,52 @@
223
224 shutil.rmtree(root_dir)
225
226+ def test_empty_removed_file(self):
227+ root_dir = make_tmp_dir()
228+
229+ file = 'created-regular-file'
230+
231+ file_path = os.path.join(self.update.system_dir, file)
232+ create_file(file_path, 'foo bar')
233+
234+ self.cache_dir = self.update.tmp_dir
235+
236+ removed_file = self.update.removed_file
237+ # Create an empty removed file
238+ create_file(removed_file, '')
239+
240+ archive = self.update.create_archive(self.TARFILE)
241+ self.assertTrue(os.path.exists(archive))
242+
243+ dest = os.path.join(self.cache_dir, os.path.basename(archive))
244+ touch_file('{}.asc'.format(dest))
245+
246+ commands = make_commands([self.TARFILE])
247+
248+ options = make_default_options()
249+
250+ # XXX: doesn't actually exist, but the option must be set since
251+ # the upgrader looks for the update archives in the directory
252+ # where this file is claimed to be.
253+ options.cmdfile = os.path.join(self.cache_dir, 'ubuntu_command')
254+
255+ options.root_dir = root_dir
256+
257+ upgrader = Upgrader(options, commands, [])
258+ upgrader.cache_dir = self.cache_dir
259+ upgrader.MOUNTPOINT_CMD = "true"
260+
261+ si_file = self.TARFILE
262+ si_signature = "{}.asc".format(self.TARFILE)
263+ upgrader._cmd_update([si_file, si_signature])
264+
265+ # Ensure that the upgrader has not attempted to remove the
266+ # cache_dir when the 'removed' file is empty.
267+ #
268+ # (Regression test for LP: #1437225).
269+ self.assertTrue(os.path.exists(upgrader.cache_dir))
270+
271+ shutil.rmtree(self.cache_dir)
272
273 if __name__ == "__main__":
274 unittest.main()
275
276=== modified file 'ubuntucoreupgrader/tests/utils.py'
277--- ubuntucoreupgrader/tests/utils.py 2015-03-04 16:50:08 +0000
278+++ ubuntucoreupgrader/tests/utils.py 2015-04-08 14:54:30 +0000
279@@ -119,7 +119,8 @@
280 '''
281 # all files listed in the removed list must be system files
282 final = list(map(lambda a:
283- '{}{}'.format(self.TEST_SYSTEM_DIR, a), removed_files))
284+ os.path.normpath('{}{}'.format(self.TEST_SYSTEM_DIR,
285+ a)), removed_files))
286
287 contents = "".join(final)
288 append_file(self.removed_file, contents)
289@@ -194,7 +195,7 @@
290
291 This creates 2 temporary directories:
292
293- - self.system dir: Used as to generate an update archive from.
294+ - self.system_dir: Used to generate an update archive from.
295
296 - self.tmp_dir: Used to write the generated archive file to. The
297 intention is that this directory should also be used to hold
298@@ -244,7 +245,9 @@
299
300 # The directory which will have the update archive applied to
301 # it.
302- self.victim_dir = make_tmp_dir(tag='victim')
303+ self.victim_dir_base = make_tmp_dir(tag='victim')
304+ self.victim_dir = os.path.join(self.victim_dir_base, 'system')
305+ os.makedirs(self.victim_dir, mode=0o750)
306
307 def tearDown(self):
308 '''
309
310=== modified file 'ubuntucoreupgrader/upgrader.py'
311--- ubuntucoreupgrader/upgrader.py 2015-03-05 13:24:40 +0000
312+++ ubuntucoreupgrader/upgrader.py 2015-04-08 14:54:30 +0000
313@@ -472,29 +472,26 @@
314 # Note: Only used by UPGRADE_IN_PLACE.
315 self.other_is_empty = False
316
317+ # cache_dir is used as a scratch pad, for downloading new images
318+ # to and bind mounting the rootfs.
319+ if self.options.tmpdir:
320+ self.cache_dir = self.options.tmpdir
321+ else:
322+ self.cache_dir = self.DEFAULT_CACHE_DIR
323+
324 def update_timestamp(self):
325 '''
326 Update the timestamp file to record the time the last upgrade
327 completed successfully.
328 '''
329- file = os.path.join(self.get_cache_dir(), self.TIMESTAMP_FILE)
330+ file = os.path.join(self.cache_dir, self.TIMESTAMP_FILE)
331 open(file, 'w').close()
332
333- def get_cache_dir(self):
334- '''
335- Returns the full path to the cache directory, which is used as a
336- scratch pad, for downloading new images to and bind mounting the
337- rootfs.
338- '''
339- return self.options.tmpdir \
340- if self.options.tmpdir \
341- else self.DEFAULT_CACHE_DIR
342-
343 def get_mount_target(self):
344 '''
345 Get the full path to the mount target directory.
346 '''
347- return os.path.join(self.get_cache_dir(), self.MOUNT_TARGET)
348+ return os.path.join(self.cache_dir, self.MOUNT_TARGET)
349
350 def prepare(self):
351 '''
352@@ -740,13 +737,17 @@
353 to_remove = []
354
355 if not self.full_image:
356-
357 if found_removed_file:
358 log.debug('processing {} file'
359 .format(self.removed_file))
360
361 # process backwards to work around bug LP:#1381134.
362 for remove in sorted(to_remove, reverse=True):
363+
364+ if not remove:
365+ # handle invalid removed file entry (see LP:#1437225)
366+ continue
367+
368 remove = remove.strip()
369
370 # don't remove devices
371@@ -762,13 +763,7 @@
372 if '../' in remove:
373 continue
374
375- if self.options.root_dir == '/':
376- final = os.path.join(self.get_cache_dir(), remove)
377- else:
378- # os.path.join() refuses to work if the file
379- # begins with a slash.
380- base = remove_prefix(remove)
381- final = '{}{}'.format(self.options.root_dir, base)
382+ final = os.path.join(self.cache_dir, remove)
383
384 if not os.path.exists(final):
385 # This scenario can only mean there is a bug
386@@ -804,9 +799,9 @@
387 from unittest.mock import patch
388 with patch("grp.getgrnam") as m:
389 m.side_effect = KeyError()
390- tar.extractall(path=self.get_cache_dir(),
391+ tar.extractall(path=self.cache_dir,
392 members=tar_generator(
393- tar, self.get_cache_dir(),
394+ tar, self.cache_dir,
395 self.removed_file,
396 self.options.root_dir))
397 tar.close()
398@@ -821,7 +816,7 @@
399 '''
400 target = self.get_mount_target()
401 bindmount_rootfs_dir = tempfile.mkdtemp(prefix=script_name,
402- dir=self.get_cache_dir())
403+ dir=self.cache_dir)
404 bind_mount("/", bindmount_rootfs_dir)
405
406 cwd = os.getcwd()

Subscribers

People subscribed via source and target branches

to all changes: