Skip to content

Commit d97dbce

Browse files
committed
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.
1 parent 9cbd578 commit d97dbce

3 files changed

Lines changed: 83 additions & 3 deletions

File tree

Lib/mailbox.py

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,19 +1069,44 @@ def __setitem__(self, key, message):
10691069
raise KeyError('No message with key: %s' % key)
10701070
else:
10711071
raise
1072+
file_closed = False
10721073
try:
10731074
if self._locked:
10741075
_lock_file(f)
10751076
try:
1076-
os.close(os.open(path, os.O_WRONLY | os.O_TRUNC))
1077-
self._dump_message(message, f)
1077+
new_file = _create_temporary(path)
1078+
try:
1079+
self._dump_message(message, new_file)
1080+
_sync_close(new_file)
1081+
info = os.fstat(f.fileno())
1082+
try:
1083+
os.chown(new_file.name, info.st_uid, info.st_gid)
1084+
except (AttributeError, OSError):
1085+
pass
1086+
os.chmod(new_file.name, info.st_mode)
1087+
if os.name == 'nt':
1088+
# Windows cannot replace an open file.
1089+
f.close()
1090+
file_closed = True
1091+
os.replace(new_file.name, path)
1092+
except BaseException:
1093+
try:
1094+
new_file.close()
1095+
except OSError:
1096+
pass
1097+
try:
1098+
os.remove(new_file.name)
1099+
except OSError:
1100+
pass
1101+
raise
10781102
if isinstance(message, MHMessage):
10791103
self._dump_sequences(message, key)
10801104
finally:
10811105
if self._locked:
10821106
_unlock_file(f)
10831107
finally:
1084-
_sync_close(f)
1108+
if not file_closed:
1109+
f.close()
10851110

10861111
def get_message(self, key):
10871112
"""Return a Message representation or raise a KeyError."""

Lib/test/test_mailbox.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import os
2+
import stat
23
import sys
34
import time
45
import socket
@@ -1338,6 +1339,57 @@ class TestMH(TestMailbox, unittest.TestCase):
13381339
def assertMailboxEmpty(self):
13391340
self.assertEqual(os.listdir(self._path), ['.mh_sequences'])
13401341

1342+
def test_set_item_nonascii_string_raises_without_modifying_message(self):
1343+
key = self._box.add(self._template % 'original')
1344+
original = self._box.get_bytes(key)
1345+
with self.assertRaisesRegex(ValueError, "ASCII-only"):
1346+
self._box[key] = self._nonascii_msg
1347+
self.assertEqual(self._box.get_bytes(key), original)
1348+
self._box.close()
1349+
self._box = self._factory(self._path)
1350+
self.assertEqual(self._box.get_bytes(key), original)
1351+
1352+
def test_set_item_read_error_does_not_modify_message(self):
1353+
class CustomError(Exception):
1354+
pass
1355+
1356+
class FaultyMessage:
1357+
def __init__(self):
1358+
self.first_read = True
1359+
1360+
def read(self):
1361+
raise AssertionError
1362+
1363+
def readline(self):
1364+
if self.first_read:
1365+
self.first_read = False
1366+
return b'Subject: replacement\n'
1367+
raise CustomError
1368+
1369+
key = self._box.add(self._template % 'original')
1370+
original = self._box.get_bytes(key)
1371+
original_files = set(os.listdir(self._path))
1372+
with self.assertRaises(CustomError):
1373+
self._box[key] = FaultyMessage()
1374+
self.assertEqual(self._box.get_bytes(key), original)
1375+
self.assertEqual(set(os.listdir(self._path)), original_files)
1376+
self._box.close()
1377+
self._box = self._factory(self._path)
1378+
self.assertEqual(self._box.get_bytes(key), original)
1379+
1380+
@unittest.skipUnless(hasattr(os, 'chown'), 'requires os.chown')
1381+
def test_set_item_preserves_mode(self):
1382+
key = self._box.add(self._template % 'original')
1383+
path = os.path.join(self._path, str(key))
1384+
mode = os.stat(path).st_mode | stat.S_ISUID
1385+
os.chmod(path, mode)
1386+
if os.stat(path).st_mode != mode:
1387+
self.skipTest('filesystem does not support set-user-ID mode')
1388+
1389+
self._box[key] = self._template % 'replacement'
1390+
1391+
self.assertEqual(os.stat(path).st_mode, mode)
1392+
13411393
def test_list_folders(self):
13421394
# List folders
13431395
self._box.add_folder('one')
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Prevent :class:`mailbox.MH` message replacement from truncating or partially
2+
overwriting the original message when serializing the replacement raises an
3+
exception.

0 commit comments

Comments
 (0)