diff --git a/CHANGELOG.md b/CHANGELOG.md index bc8f6a76..61380555 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ - `find_unknown_base_reverse_dns.py`'s missing-file checks for `base_reverse_dns_map.csv` and the `known_unknown`/PSL-override lists printed a clean error message but fell through into an unhandled `FileNotFoundError` traceback instead of exiting. +### Bug fixes + +- **`parse_report_file()` now closes the file handle it opens itself for a path input if reading it raises.** When `input_` is a path, the function opened the file, read it, and closed it with no exception handling in between; an exception raised by `read()` (e.g. an `OSError` from the underlying storage) skipped the close, so the descriptor was left to be released only when Python's garbage collector eventually finalized the object — CPython's `io.IOBase.__del__` closes an unclosed file on finalization () — rather than being closed deterministically. This is the pattern CodeQL's `py/file-not-closed` query flags, found in a local code-quality scan. The path branch now opens the file with a `with` block, so the handle is closed on both the success and exception paths. A file-like object or bytes buffer supplied by the caller is unaffected: as before, it is closed only after a successful read, and left open if `read()` raises. + ## 11.0.1 ### Security diff --git a/parsedmarc/__init__.py b/parsedmarc/__init__.py index a1317c15..ac1c8e04 100644 --- a/parsedmarc/__init__.py +++ b/parsedmarc/__init__.py @@ -2319,14 +2319,20 @@ def parse_report_file( if isinstance(input_, (str, os.PathLike)): file_path = os.fspath(input_) logger.debug(f"Parsing {file_path}") - file_object = open(file_path, "rb") - elif isinstance(input_, (bytes, bytearray, memoryview)): - file_object = BytesIO(bytes(input_)) + # A path is a handle we opened ourselves, so it must be closed on + # both the success and exception paths. + with open(file_path, "rb") as file_object: + content = file_object.read() else: - file_object = input_ - - content = file_object.read() - file_object.close() + if isinstance(input_, (bytes, bytearray, memoryview)): + file_object = BytesIO(bytes(input_)) + else: + # A caller-supplied file-like object is only closed on success, + # matching long-standing behavior; it is left open if read() + # raises. + file_object = input_ + content = file_object.read() + file_object.close() if content.startswith(MAGIC_ZIP) or content.startswith(MAGIC_GZIP): content = extract_report(content) diff --git a/tests/test_init.py b/tests/test_init.py index bf60e8af..233e7789 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -2754,6 +2754,83 @@ class TestParseReportFile(unittest.TestCase): finally: logger.setLevel(previous) + def testParseReportFileClosesHandleOnReadError(self): + """A file opened internally by parse_report_file is closed even + when reading it raises (the pattern CodeQL's py/file-not-closed + query flags, found in a local code-quality scan). + + parse_report_file's path branch does `open(file_path, "rb")` and + then `.read()`; before the fix, `.close()` sat on the line after + `.read()` with no try/finally (and no `with`), so a read()-time + exception skipped close() and leaked the descriptor. Patch + builtins.open (the SDK boundary) to return a handle whose read() + raises, and assert the handle was still closed. The fake handle + implements the context-manager protocol itself (mirroring a real + file object's __exit__ calling close()), rather than relying on + MagicMock's default __enter__/__exit__, which would return a + different mock object from __enter__ and never call close(). + """ + + class _RaisingHandle: + def __init__(self): + self.closed = False + + def read(self): + raise OSError("boom") + + def close(self): + self.closed = True + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + self.close() + return False + + fake_handle = _RaisingHandle() + + with patch("builtins.open", return_value=fake_handle) as mock_open: + with self.assertRaises(OSError): + parsedmarc.parse_report_file("some/path.xml", offline=True) + + mock_open.assert_called_once_with("some/path.xml", "rb") + self.assertTrue(fake_handle.closed) + + def testParseReportFileLeavesCallerHandleOpenOnReadError(self): + """A caller-supplied file-like object is left open (not closed) + when reading it raises, preserving parse_report_file's + long-standing contract for handles it did not open itself. + + Only a path input (opened internally by parse_report_file) is + closed on the exception path; a handle the caller passed in is + closed on success only, exactly as before the fix for the + internally-opened-path leak. + + A plain class is used instead of MagicMock because MagicMock + auto-implements ``__fspath__`` (supported since Python 3.8's + unittest.mock), which would make ``isinstance(fake, os.PathLike)`` + true and route the object through the path branch instead of the + caller-supplied-object branch this test targets. + """ + + class _RaisingCallerHandle: + def __init__(self): + self.close_called = False + + def read(self): + raise OSError("boom") + + def close(self): + self.close_called = True + + fake_handle = _RaisingCallerHandle() + + with self.assertRaises(OSError): + parsedmarc.parse_report_file(cast(BinaryIO, fake_handle), offline=True) + + self.assertFalse(fake_handle.close_called) + class TestParseReportEmail(unittest.TestCase): """Tests for parse_report_email edge cases""" @@ -3699,7 +3776,11 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase): self._inbox = mailbox.Maildir(self._maildir, create=True) def _deliver(self, source): - raw = open(source, "rb").read() if isinstance(source, str) else source + if isinstance(source, str): + with open(source, "rb") as f: + raw = f.read() + else: + raw = source self._inbox.add(mailbox.MaildirMessage(raw)) self._inbox.flush()