#11767: use context manager to close file in __getitem__ to prevent FD leak

All of the other methods in mailbox that create message objects take care to
close the file descriptors they use, so it seems to make sense to have
__getitem__ do so as well.

Patch by Filip Gruszczyński.

diff --git a/Lib/mailbox.py b/Lib/mailbox.py
index 0e4f99bcaf5a1d555906869fb4495782f9f99253..b96b270d68730dd9db2ab148d63a9e6857d2726c 100644 (file)
--- a/Lib/mailbox.py
+++ b/Lib/mailbox.py
@@ -20,6 +20,7 @@ import email
 import email.message
 import email.generator
 import io
+import contextlib
 try:
     if sys.platform == 'os2emx':
         # OS/2 EMX fcntl() not adequate
@@ -76,7 +77,8 @@ class Mailbox:
         if not self._factory:
             return self.get_message(key)
         else:
-            return self._factory(self.get_file(key))
+            with contextlib.closing(self.get_file(key)) as file:
+                return self._factory(file)
 
     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 8dc73262dd60b0ff24a7125542955978d2aac90e..10317c3bb913f8c4efd2a7300958c437f7ccbbdc 100644 (file)
--- a/Lib/test/test_mailbox.py
+++ b/Lib/test/test_mailbox.py
@@ -1201,6 +1201,37 @@ class TestBabyl(TestMailbox):
         self.assertEqual(set(self._box.get_labels()), set(['blah']))
 
 
+class FakeFileLikeObject:
+
+    def __init__(self):
+        self.closed = False
+
+    def close(self):
+        self.closed = True
+
+
+class FakeMailBox(mailbox.Mailbox):
+
+    def __init__(self):
+        mailbox.Mailbox.__init__(self, '', lambda file: None)
+        self.files = [FakeFileLikeObject() for i in range(10)]
+
+    def get_file(self, key):
+        return self.files[key]
+
+
+class TestFakeMailBox(unittest.TestCase):
+
+    def test_closing_fd(self):
+        box = FakeMailBox()
+        for i in range(10):
+            self.assertFalse(box.files[i].closed)
+        for i in range(10):
+            box[i]
+        for i in range(10):
+            self.assertTrue(box.files[i].closed)
+
+
 class TestMessage(TestBase):
 
     _factory = mailbox.Message      # Overridden by subclasses to reuse tests
@@ -2113,7 +2144,7 @@ def test_main():
              TestBabyl, TestMessage, TestMaildirMessage, TestMboxMessage,
              TestMHMessage, TestBabylMessage, TestMMDFMessage,
              TestMessageConversion, TestProxyFile, TestPartialFile,
-             MaildirTestCase)
+             MaildirTestCase, TestFakeMailBox)
     support.run_unittest(*tests)
     support.reap_children()
 

diff --git a/Misc/NEWS b/Misc/NEWS
index 6cc0f03637d8ca026c24f4b2fc55ce5efcd3ea1b..ec8b72ec32b6703a439448558a7cc7b7e3b7b2d3 100644 (file)
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -25,6 +25,8 @@ Core and Builtins
 Library
 -------
 
+- Issue #11767: Correct file descriptor leak in mailbox's __getitem__ method.
+
 - Issue #12133: AbstractHTTPHandler.do_open() of urllib.request closes the HTTP
   connection if its getresponse() method fails with a socket error. Patch
   written by Ezio Melotti.