From bcca8aefabac1998e97afe24794d12a6d756cc56 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 23 Oct 2026 16:12:37 -0400 Subject: [PATCH] fix(foundation): close the input when the Extractor constructor raises (#2100) Only a KeyboardInterrupt reached _cleanup, so a FormatError out of __init__ left an input the Extractor opened open until collection, which Windows will not delete. The magic read and run() now release an owned input and detach its finaliser on any exception; a caller's stream stays open. _cleanup itself is not called: an unknown format raises before any engine exists. --- pcapkit/foundation/extraction.py | 30 +++-- ...extraction_constructor_raises_2100_unit.py | 111 ++++++++++++++++++ 2 files changed, 131 insertions(+), 10 deletions(-) create mode 100644 tests/foundation/test_extraction_constructor_raises_2100_unit.py diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 8b18b5b182..ca5cc4a713 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -1507,16 +1507,26 @@ def __init__(self, # NOTE: we use peek() to read the magic number, as the file pointer # will not be moved after reading; however, the returned bytes object # may not be exactly 4 bytes, so we use [:4] to get the first 4 bytes - self._magic = self._ifile.peek(4)[:4] - if len(self._magic) < 4: - # NOTE: ``peek`` makes at most one read of the stream, which a pipe or a - # raw stream may answer with fewer than four octets, so a short peek is - # not a short file. Read them instead, and seek back (#1586). - position = self._ifile.tell() - self._magic = self._ifile.read(4) - self._ifile.seek(position, os.SEEK_SET) - - self.run() # start extraction + try: + self._magic = self._ifile.peek(4)[:4] + if len(self._magic) < 4: + # NOTE: ``peek`` makes at most one read of the stream, which a pipe or a + # raw stream may answer with fewer than four octets, so a short peek is + # not a short file. Read them instead, and seek back (#1586). + position = self._ifile.tell() + self._magic = self._ifile.read(4) + self._ifile.seek(position, os.SEEK_SET) + + self.run() # start extraction + except BaseException: + # NOTE: a constructor that raises leaves the caller nothing to close, + # and Windows cannot delete a file still open (#2100). Only the input + # is released: ``_cleanup`` would need an engine, which an unknown + # format never gets, and would finish a trace that never ran. + if self._owns_input(): + self._ifile.close() + self._detach_finalizer() + raise def __iter__(self) -> 'Extractor': """Iterate and parse PCAP frame. diff --git a/tests/foundation/test_extraction_constructor_raises_2100_unit.py b/tests/foundation/test_extraction_constructor_raises_2100_unit.py new file mode 100644 index 0000000000..12cb15c5d2 --- /dev/null +++ b/tests/foundation/test_extraction_constructor_raises_2100_unit.py @@ -0,0 +1,111 @@ +# -*- coding: utf-8 -*- +"""An ``Extractor`` whose constructor raises has closed the input it opened (#2100). + +Only a ``KeyboardInterrupt`` reached ``_cleanup``, so a ``FormatError`` out of +``__init__`` left the file open until collection, and the exception holds the +frame that keeps it alive. Windows then refuses to delete the file +(``WinError 32``); POSIX hides it, so the handles are recorded here instead. + +""" + +from __future__ import annotations + +import builtins +import importlib.util +import io +import pathlib +import struct +import tempfile +import unittest +import warnings +from unittest import mock + +from tests._support import reimport_once_per_class + +#: Test tier (#1958): exercises finalisation and stream ownership. +__tier__ = 'ALL' + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: A PCAP-NG section header block. +SHB = struct.pack(' None: + super().__init__() + self._data = io.BytesIO(data) + + def readable(self) -> bool: + return True + + def readinto(self, buffer: 'bytearray | memoryview') -> int: # type: ignore[override] + return self._data.readinto(buffer) + + +@unittest.skipUnless(HAS_RUNTIME, 'pcapkit runtime dependencies are not installed') +class TestConstructorRaises(unittest.TestCase): + """The input is released as the constructor raises, by the ownership rule (#610).""" + + def setUp(self) -> None: + reimport_once_per_class(self) + tmp = tempfile.TemporaryDirectory() # pylint: disable=consider-using-with + self.addCleanup(tmp.cleanup) + self.tmp_path = pathlib.Path(tmp.name) + + def extract(self, fin: 'object', **kwargs: 'object') -> 'BaseException': + import pcapkit # pylint: disable=import-outside-toplevel + from pcapkit.utilities.exceptions import FormatError # pylint: disable=import-outside-toplevel + + with warnings.catch_warnings(), self.assertRaises(FormatError) as caught: + warnings.simplefilter('ignore') + pcapkit.extract(fin=fin, nofile=True, **kwargs) + return caught.exception + + def opened_by_path(self, data: 'bytes') -> 'tuple[list[io.BufferedReader], BaseException]': + """Extract ``data`` by path, and return the handles the extractor opened.""" + import pcapkit.foundation.extraction as extraction # pylint: disable=import-outside-toplevel + + path = self.tmp_path / 'capture.pcapng' + path.write_bytes(data) + handles = [] # type: list[io.BufferedReader] + + def recording_open(*args: 'object', **kwargs: 'object') -> 'io.BufferedReader': + handle = builtins.open(*args, **kwargs) # type: ignore[call-overload] + handles.append(handle) + return handle + + with mock.patch.object(extraction, 'open', recording_open, create=True): + error = self.extract(str(path)) + self.assertEqual(len(handles), 1) + return handles, error + + def test_a_format_error_while_reading_frames_closes_the_input(self) -> None: + handles, error = self.opened_by_path(SHB + EPB) + self.assertIn('interface', str(error)) + self.assertTrue(handles[0].closed) + + def test_an_unknown_format_closes_the_input_and_keeps_its_error(self) -> None: + handles, error = self.opened_by_path(b'\x00' * 64) + self.assertIn('unknown file format', str(error)) + self.assertTrue(handles[0].closed) + + def test_a_stream_the_caller_gave_is_left_open(self) -> None: + path = self.tmp_path / 'capture.pcapng' + path.write_bytes(SHB + EPB) + with open(path, 'rb') as file: + self.extract(file) + self.assertFalse(file.closed) + + stream = Unseekable(SHB + EPB) + self.extract(stream, buffer_save=True) + self.assertFalse(stream.closed) + + +if __name__ == '__main__': + unittest.main()