From d97dbcea85274baef1ac0572a77a85dc9d9b2b57 Mon Sep 17 00:00:00 2001 From: lipengyu Date: Mon, 24 Aug 2026 19:44:19 +0800 Subject: [PATCH 1/2] gh-156312: Prevent mailbox.MH replacement failures from corrupting messages Make `mailbox.MH.__setitem__()` failure-atomic by writing replacements to a temporary file before replacing the original. Failed serialization now leaves the existing message unchanged. --- Lib/mailbox.py | 31 +++++++++-- Lib/test/test_mailbox.py | 52 +++++++++++++++++++ ...-08-24-19-37-56.gh-issue-156312.tF9p9m.rst | 3 ++ 3 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-08-24-19-37-56.gh-issue-156312.tF9p9m.rst diff --git a/Lib/mailbox.py b/Lib/mailbox.py index 99426220154360b..bda86c531fb9ba2 100644 --- a/Lib/mailbox.py +++ b/Lib/mailbox.py @@ -1069,19 +1069,44 @@ def __setitem__(self, key, message): raise KeyError('No message with key: %s' % key) else: raise + file_closed = False try: if self._locked: _lock_file(f) try: - os.close(os.open(path, os.O_WRONLY | os.O_TRUNC)) - self._dump_message(message, f) + new_file = _create_temporary(path) + try: + self._dump_message(message, new_file) + _sync_close(new_file) + info = os.fstat(f.fileno()) + try: + os.chown(new_file.name, info.st_uid, info.st_gid) + except (AttributeError, OSError): + pass + os.chmod(new_file.name, info.st_mode) + if os.name == 'nt': + # Windows cannot replace an open file. + f.close() + file_closed = True + os.replace(new_file.name, path) + except BaseException: + try: + new_file.close() + except OSError: + pass + try: + os.remove(new_file.name) + except OSError: + pass + raise if isinstance(message, MHMessage): self._dump_sequences(message, key) finally: if self._locked: _unlock_file(f) finally: - _sync_close(f) + if not file_closed: + f.close() def get_message(self, key): """Return a Message representation or raise a KeyError.""" diff --git a/Lib/test/test_mailbox.py b/Lib/test/test_mailbox.py index 019c699bff55c42..340c725a9e03d81 100644 --- a/Lib/test/test_mailbox.py +++ b/Lib/test/test_mailbox.py @@ -1,4 +1,5 @@ import os +import stat import sys import time import socket @@ -1338,6 +1339,57 @@ class TestMH(TestMailbox, unittest.TestCase): def assertMailboxEmpty(self): self.assertEqual(os.listdir(self._path), ['.mh_sequences']) + def test_set_item_nonascii_string_raises_without_modifying_message(self): + key = self._box.add(self._template % 'original') + original = self._box.get_bytes(key) + with self.assertRaisesRegex(ValueError, "ASCII-only"): + self._box[key] = self._nonascii_msg + self.assertEqual(self._box.get_bytes(key), original) + self._box.close() + self._box = self._factory(self._path) + self.assertEqual(self._box.get_bytes(key), original) + + def test_set_item_read_error_does_not_modify_message(self): + class CustomError(Exception): + pass + + class FaultyMessage: + def __init__(self): + self.first_read = True + + def read(self): + raise AssertionError + + def readline(self): + if self.first_read: + self.first_read = False + return b'Subject: replacement\n' + raise CustomError + + key = self._box.add(self._template % 'original') + original = self._box.get_bytes(key) + original_files = set(os.listdir(self._path)) + with self.assertRaises(CustomError): + self._box[key] = FaultyMessage() + self.assertEqual(self._box.get_bytes(key), original) + self.assertEqual(set(os.listdir(self._path)), original_files) + self._box.close() + self._box = self._factory(self._path) + self.assertEqual(self._box.get_bytes(key), original) + + @unittest.skipUnless(hasattr(os, 'chown'), 'requires os.chown') + def test_set_item_preserves_mode(self): + key = self._box.add(self._template % 'original') + path = os.path.join(self._path, str(key)) + mode = os.stat(path).st_mode | stat.S_ISUID + os.chmod(path, mode) + if os.stat(path).st_mode != mode: + self.skipTest('filesystem does not support set-user-ID mode') + + self._box[key] = self._template % 'replacement' + + self.assertEqual(os.stat(path).st_mode, mode) + def test_list_folders(self): # List folders self._box.add_folder('one') diff --git a/Misc/NEWS.d/next/Library/2026-08-24-19-37-56.gh-issue-156312.tF9p9m.rst b/Misc/NEWS.d/next/Library/2026-08-24-19-37-56.gh-issue-156312.tF9p9m.rst new file mode 100644 index 000000000000000..9fcfc2c6a0a8d6f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-08-24-19-37-56.gh-issue-156312.tF9p9m.rst @@ -0,0 +1,3 @@ +Prevent :class:`mailbox.MH` message replacement from truncating or partially +overwriting the original message when serializing the replacement raises an +exception. From 032bab46ac430f61e48daf98e06dca2484bfcb00 Mon Sep 17 00:00:00 2001 From: lipengyu Date: Tue, 8 Sep 2026 20:25:49 +0800 Subject: [PATCH 2/2] update Serialize the replacement to a temporary file before overwriting the existing MH message, so invalid input or input read errors leave the original message unchanged. Co-Authored-By: lipengyu --- Lib/mailbox.py | 34 +++++++--------------------------- Lib/test/test_mailbox.py | 21 ++++++++++++++++----- 2 files changed, 23 insertions(+), 32 deletions(-) diff --git a/Lib/mailbox.py b/Lib/mailbox.py index bda86c531fb9ba2..510330fcdff54b6 100644 --- a/Lib/mailbox.py +++ b/Lib/mailbox.py @@ -18,6 +18,8 @@ import email.generator import io import contextlib +import shutil +import tempfile from types import GenericAlias try: import fcntl @@ -1069,44 +1071,22 @@ def __setitem__(self, key, message): raise KeyError('No message with key: %s' % key) else: raise - file_closed = False try: if self._locked: _lock_file(f) try: - new_file = _create_temporary(path) - try: + with tempfile.TemporaryFile(mode='w+b') as new_file: self._dump_message(message, new_file) - _sync_close(new_file) - info = os.fstat(f.fileno()) - try: - os.chown(new_file.name, info.st_uid, info.st_gid) - except (AttributeError, OSError): - pass - os.chmod(new_file.name, info.st_mode) - if os.name == 'nt': - # Windows cannot replace an open file. - f.close() - file_closed = True - os.replace(new_file.name, path) - except BaseException: - try: - new_file.close() - except OSError: - pass - try: - os.remove(new_file.name) - except OSError: - pass - raise + new_file.seek(0) + os.close(os.open(path, os.O_WRONLY | os.O_TRUNC)) + shutil.copyfileobj(new_file, f) if isinstance(message, MHMessage): self._dump_sequences(message, key) finally: if self._locked: _unlock_file(f) finally: - if not file_closed: - f.close() + _sync_close(f) def get_message(self, key): """Return a Message representation or raise a KeyError.""" diff --git a/Lib/test/test_mailbox.py b/Lib/test/test_mailbox.py index 340c725a9e03d81..c864a08d695540a 100644 --- a/Lib/test/test_mailbox.py +++ b/Lib/test/test_mailbox.py @@ -1377,18 +1377,29 @@ def readline(self): self._box = self._factory(self._path) self.assertEqual(self._box.get_bytes(key), original) - @unittest.skipUnless(hasattr(os, 'chown'), 'requires os.chown') + @unittest.skipUnless(os.name == 'posix', 'requires POSIX permissions') def test_set_item_preserves_mode(self): key = self._box.add(self._template % 'original') path = os.path.join(self._path, str(key)) - mode = os.stat(path).st_mode | stat.S_ISUID + mode = 0o640 os.chmod(path, mode) - if os.stat(path).st_mode != mode: - self.skipTest('filesystem does not support set-user-ID mode') + if stat.S_IMODE(os.stat(path).st_mode) != mode: + self.skipTest('filesystem does not support POSIX permissions') self._box[key] = self._template % 'replacement' - self.assertEqual(os.stat(path).st_mode, mode) + self.assertEqual(stat.S_IMODE(os.stat(path).st_mode), mode) + + def test_set_item_with_open_file(self): + key = self._box.add(self._template % 'original') + replacement = self._template % 'replacement' + self._box.lock() + try: + with self._box.get_file(key): + self._box[key] = replacement + self.assertEqual(self._box.get_bytes(key), replacement.encode('ascii')) + finally: + self._box.unlock() def test_list_folders(self): # List folders