Release Upgrader

Merge ~mruffell/ubuntu-release-upgrader:lp1982534 into ubuntu-release-upgrader:ubuntu/focal

  1. Git
  2. lp:~mruffell/ubuntu-release-upgrader
  3. lp1982534
  4. Merge into ubuntu/focal
Proposed by Matthew Ruffell
Status: Merged
Merged at revision: 5fafe12d3760861ed31ae265b6394a8c360f90e1
Proposed branch: ~mruffell/ubuntu-release-upgrader:lp1982534
Merge into: ubuntu-release-upgrader:ubuntu/focal
Diff against target: 105 lines (+15/-15)
2 files modified
DistUpgrade/DistUpgradeQuirks.py (+3/-3)
tests/test_quirks.py (+12/-12)
Reviewer Review Type Date Requested Status
Brian Murray Approve
Review via email: mp+435410@code.launchpad.net

Description of the change

Quirks: Change fips libgcrypt functions from subp.Popen to safer alternatives

Robie Basak pointed out during SRU review that subprocess.Popen()
requires the author to call wait() and check the return code manually
when it could be done in a more safe fashion with check_call().

Change _fipsLibgcryptDivert() to use subprocess.check_call().

Additionally, change the subprocess.Popen() for manual rm to
os.unlink(), again a safer way of removing the hmac file.

Change the tests to match new functions.

(LP: #1982534)

To post a comment you must log in.
Revision history for this message
Brian Murray (brian-murray) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
2index 33c0a57..33479f2 100644
3--- a/DistUpgrade/DistUpgradeQuirks.py
4+++ b/DistUpgrade/DistUpgradeQuirks.py
5@@ -1154,8 +1154,8 @@ class DistUpgradeQuirks(object):
6 # We do not use --rename due to still requiring the file
7 # while the upgrade completes. We will cleanup the old hmac
8 # file later in _fipsLibgcryptHmacCleanup()
9- subprocess.Popen(["dpkg-divert", "--add", "--divert",
10- new, old])
11+ subprocess.check_call(["dpkg-divert", "--add", "--divert",
12+ new, old])
13 except Exception as e:
14 logging.exception("Failed to add dpkg-divert for " +
15 old + "(%s)" % e)
16@@ -1200,6 +1200,6 @@ class DistUpgradeQuirks(object):
17 else:
18 logging.debug("Removing old hmac file from Bionic version")
19 try:
20- subprocess.Popen(["rm", old])
21+ os.unlink(old)
22 except Exception as e:
23 logging.exception("Failed to remove " + old + " (%s)" % e)
24diff --git a/tests/test_quirks.py b/tests/test_quirks.py
25index 43d9851..cf705cc 100644
26--- a/tests/test_quirks.py
27+++ b/tests/test_quirks.py
28@@ -501,8 +501,8 @@ class TestQuirks(unittest.TestCase):
29 self.assertTrue(any("Fixed libgcrypt20-hmac will be installed"
30 in s for s in cm.output))
31
32- @mock.patch('subprocess.Popen')
33- def test_fips_libgcrypt_broken(self, mocked_Popen):
34+ @mock.patch('subprocess.check_call')
35+ def test_fips_libgcrypt_broken(self, mocked_check_call):
36 mock_controller = mock.Mock()
37 mock_cache = dict([
38 ('libgcrypt20', make_mock_pkg(
39@@ -527,10 +527,10 @@ class TestQuirks(unittest.TestCase):
40 q._fipsLibgcryptDivert()
41 self.assertTrue(any("Broken libgcrypt20-hmac will be installed"
42 in s for s in cm.output))
43- self.assertTrue(mocked_Popen.called)
44+ self.assertTrue(mocked_check_call.called)
45
46- @mock.patch('subprocess.Popen', side_effect=Exception())
47- def test_fips_libgcrypt_broken_exception(self, mocked_Popen):
48+ @mock.patch('subprocess.check_call', side_effect=Exception())
49+ def test_fips_libgcrypt_broken_exception(self, mocked_check_call):
50 mock_controller = mock.Mock()
51 mock_cache = dict([
52 ('libgcrypt20', make_mock_pkg(
53@@ -557,7 +557,7 @@ class TestQuirks(unittest.TestCase):
54 in s for s in cm.output))
55 self.assertTrue(any("Failed to add dpkg-divert for"
56 in s for s in cm.output))
57- self.assertTrue(mocked_Popen.called)
58+ self.assertTrue(mocked_check_call.called)
59
60 def test_fips_libgcrypt_cleanup_not_installed(self):
61 mock_controller = mock.Mock()
62@@ -681,11 +681,11 @@ class TestQuirks(unittest.TestCase):
63 in s for s in cm.output))
64 self.assertTrue(mocked_exists.called)
65
66- @mock.patch('subprocess.Popen')
67+ @mock.patch('os.unlink')
68 @mock.patch('os.path.exists', side_effect=[True, True])
69 @mock.patch('os.path.islink', side_effect=[False])
70 def test_fips_libgcrypt_cleanup_broken_present(
71- self, mocked_islink, mocked_exists, mocked_Popen):
72+ self, mocked_islink, mocked_exists, mocked_unlink):
73 mock_controller = mock.Mock()
74 mock_cache = dict([
75 ('libgcrypt20', make_mock_pkg(
76@@ -712,7 +712,7 @@ class TestQuirks(unittest.TestCase):
77 self.assertTrue(any("Removing old hmac file from Bionic version"
78 in s for s in cm.output))
79 self.assertTrue(mocked_exists.called)
80- self.assertTrue(mocked_Popen.called)
81+ self.assertTrue(mocked_unlink.called)
82 self.assertTrue(mocked_islink.called)
83
84 @mock.patch('os.path.exists', side_effect=[True, True])
85@@ -751,11 +751,11 @@ class TestQuirks(unittest.TestCase):
86 self.assertTrue(mocked_islink.called)
87 self.assertTrue(mocked_realpath.called)
88
89- @mock.patch('subprocess.Popen', side_effect=Exception())
90+ @mock.patch('os.unlink', side_effect=Exception())
91 @mock.patch('os.path.exists', side_effect=[True, True])
92 @mock.patch('os.path.islink', side_effect=[False])
93 def test_fips_libgcrypt_cleanup_broken_present_exception(
94- self, mocked_islink, mocked_exists, mocked_Popen):
95+ self, mocked_islink, mocked_exists, mocked_unlink):
96 mock_controller = mock.Mock()
97 mock_cache = dict([
98 ('libgcrypt20', make_mock_pkg(
99@@ -784,7 +784,7 @@ class TestQuirks(unittest.TestCase):
100 self.assertTrue(any("Failed to remove"
101 in s for s in cm.output))
102 self.assertTrue(mocked_exists.called)
103- self.assertTrue(mocked_Popen.called)
104+ self.assertTrue(mocked_unlink.called)
105 self.assertTrue(mocked_islink.called)
106
107

Subscribers

People subscribed via source and target branches