From 374629798e9db454dd8f27b16b7b5e8b81d85470 Mon Sep 17 00:00:00 2001 From: CF Bolz-Tereick Date: Sat, 9 Nov 2024 16:07:57 +0100 Subject: [PATCH 1/6] check that the pyc file is fully written in _write_atomic --- Lib/importlib/_bootstrap_external.py | 4 +++- Lib/test/test_importlib/test_util.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 1b76328429f63a..6e26ad69970548 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -209,7 +209,9 @@ def _write_atomic(path, data, mode=0o666): # We first write data to a temporary file, and then use os.replace() to # perform an atomic rename. with _io.FileIO(fd, 'wb') as file: - file.write(data) + bytes_written = file.write(data) + if bytes_written != len(data): + raise OSError("os.write didn't write the full pyc file") _os.replace(path_tmp, path) except OSError: try: diff --git a/Lib/test/test_importlib/test_util.py b/Lib/test/test_importlib/test_util.py index 668042782bdc5f..799df5c8ca3fba 100644 --- a/Lib/test/test_importlib/test_util.py +++ b/Lib/test/test_importlib/test_util.py @@ -775,5 +775,33 @@ def test_complete_multi_phase_init_module(self): self.run_with_own_gil(script) +class MiscTests(unittest.TestCase): + def test_atomic_write_should_notice_incomplete_writes(self): + from importlib import _bootstrap_external + from test.support import os_helper + import _pyio + import os + + oldwrite = os.write + seen_write = False + + # emulate an os.write that only writes partial data + def write(fd, data): + nonlocal seen_write + seen_write = True + return oldwrite(fd, data[:100]) + + # need to patch _io to be _pyio, so that io.FileIO is affected by the + # os.write patch + with (unittest.mock.patch('importlib._bootstrap_external._io', _pyio), + unittest.mock.patch('os.write', write)): + with self.assertRaises(OSError): + _bootstrap_external._write_atomic(os_helper.TESTFN, b'x' * 10000) + assert seen_write + + with self.assertRaises(OSError): + os.stat(os_helper.TESTFN) # did not get written + + if __name__ == '__main__': unittest.main() From c87d9bb7cb4a265b8b21246dfb01ef36b0dca04e Mon Sep 17 00:00:00 2001 From: CF Bolz-Tereick Date: Sat, 9 Nov 2024 16:10:28 +0100 Subject: [PATCH 2/6] add news --- .../2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst new file mode 100644 index 00000000000000..66593d28d19ba5 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst @@ -0,0 +1,3 @@ +Fix importlib to not write an incomplete pyc files when a ulimit or some +other operating system mechanism is preventing the write to go through +fully. From 2c07d2b77f08bb4b68bedf1173cf3f8050fdc52f Mon Sep 17 00:00:00 2001 From: CF Bolz-Tereick Date: Sat, 9 Nov 2024 18:59:23 +0100 Subject: [PATCH 3/6] Update Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst Co-authored-by: Kirill Podoprigora --- .../2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst index 66593d28d19ba5..9c0072304ded63 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-11-09-16-10-22.gh-issue-126066.9zs4m4.rst @@ -1,3 +1,3 @@ -Fix importlib to not write an incomplete pyc files when a ulimit or some +Fix :mod:`importlib` to not write an incomplete .pyc files when a ulimit or some other operating system mechanism is preventing the write to go through fully. From 50f21680a120eb993c51566bd87f4e0d7c9c1b46 Mon Sep 17 00:00:00 2001 From: CF Bolz-Tereick Date: Wed, 13 Nov 2024 10:37:11 +0100 Subject: [PATCH 4/6] add parens to error, add comment why I'm using an OSError --- Lib/importlib/_bootstrap_external.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 6e26ad69970548..153ea22203d596 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -211,7 +211,9 @@ def _write_atomic(path, data, mode=0o666): with _io.FileIO(fd, 'wb') as file: bytes_written = file.write(data) if bytes_written != len(data): - raise OSError("os.write didn't write the full pyc file") + # Raise an OSError so the except below cleans up the partially + # written file. + raise OSError("os.write() didn't write the full pyc file") _os.replace(path_tmp, path) except OSError: try: From bffdc028f4ee2e9fbdc24c7e48d798e53b98ad63 Mon Sep 17 00:00:00 2001 From: CF Bolz-Tereick Date: Wed, 13 Nov 2024 12:58:58 +0100 Subject: [PATCH 5/6] improve test: - use swap_attr instead of mock - nicer comments - remove magic constant --- Lib/test/test_importlib/test_util.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_importlib/test_util.py b/Lib/test/test_importlib/test_util.py index 799df5c8ca3fba..0bdd1b4b82e544 100644 --- a/Lib/test/test_importlib/test_util.py +++ b/Lib/test/test_importlib/test_util.py @@ -6,12 +6,14 @@ importlib_util = util.import_importlib('importlib.util') import importlib.util +from importlib import _bootstrap_external import os import pathlib import re import string import sys from test import support +from test.support import os_helper import textwrap import types import unittest @@ -777,30 +779,32 @@ def test_complete_multi_phase_init_module(self): class MiscTests(unittest.TestCase): def test_atomic_write_should_notice_incomplete_writes(self): - from importlib import _bootstrap_external - from test.support import os_helper import _pyio - import os oldwrite = os.write seen_write = False - # emulate an os.write that only writes partial data + truncate_at_length = 100 + + # Emulate an os.write that only writes partial data. def write(fd, data): nonlocal seen_write seen_write = True - return oldwrite(fd, data[:100]) + return oldwrite(fd, data[:truncate_at_length]) - # need to patch _io to be _pyio, so that io.FileIO is affected by the - # os.write patch - with (unittest.mock.patch('importlib._bootstrap_external._io', _pyio), - unittest.mock.patch('os.write', write)): + # Need to patch _io to be _pyio, so that io.FileIO is affected by the + # os.write patch. + with (support.swap_attr(_bootstrap_external, '_io', _pyio), + support.swap_attr(os, 'write', write)): with self.assertRaises(OSError): - _bootstrap_external._write_atomic(os_helper.TESTFN, b'x' * 10000) + # Make sure we write something longer than the point where we + # truncate. + content = b'x' * (truncate_at_length * 2) + _bootstrap_external._write_atomic(os_helper.TESTFN, content) assert seen_write with self.assertRaises(OSError): - os.stat(os_helper.TESTFN) # did not get written + os.stat(support.os_helper.TESTFN) # Check that the file did not get written. if __name__ == '__main__': From 15b23b57d0d758d4f7ccc93dc54f6320c4d5044e Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Wed, 13 Nov 2024 13:05:33 -0800 Subject: [PATCH 6/6] Tweak a comment --- Lib/importlib/_bootstrap_external.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/importlib/_bootstrap_external.py b/Lib/importlib/_bootstrap_external.py index 153ea22203d596..fa36159711846f 100644 --- a/Lib/importlib/_bootstrap_external.py +++ b/Lib/importlib/_bootstrap_external.py @@ -211,7 +211,7 @@ def _write_atomic(path, data, mode=0o666): with _io.FileIO(fd, 'wb') as file: bytes_written = file.write(data) if bytes_written != len(data): - # Raise an OSError so the except below cleans up the partially + # Raise an OSError so the 'except' below cleans up the partially # written file. raise OSError("os.write() didn't write the full pyc file") _os.replace(path_tmp, path)