diff --git a/gltest/direct/loader.py b/gltest/direct/loader.py index 09f8c7f..8b752ea 100644 --- a/gltest/direct/loader.py +++ b/gltest/direct/loader.py @@ -309,6 +309,10 @@ def _inject_message_to_fd0(vm: "VMContext") -> None: # Create a temp file with the encoded message fd, path = tempfile.mkstemp() + # Record the path on vm immediately, before anything below can raise — + # otherwise a failure between mkstemp() and here would leak the file + # with nothing recorded to find it for cleanup later. + vm._stdin_temp_path = path try: os.write(fd, encoded) os.lseek(fd, 0, os.SEEK_SET) # Reset to beginning @@ -321,7 +325,13 @@ def _inject_message_to_fd0(vm: "VMContext") -> None: os.dup2(fd, 0) finally: os.close(fd) - os.unlink(path) + + # Actual extraction/unlinking of vm._stdin_temp_path is deferred until + # fd 0's duplicate of this file is closed (in _cleanup_after_deactivate, + # after stdin is restored). Unlinking here while fd 0 still references + # the file works on POSIX (the directory entry is removed but the data + # stays available via the open fd) but raises PermissionError on + # Windows, which locks files with open handles. def _load_module(contract_path: Path) -> Any: diff --git a/gltest/direct/vm.py b/gltest/direct/vm.py index a88f5af..0cd2734 100644 --- a/gltest/direct/vm.py +++ b/gltest/direct/vm.py @@ -507,6 +507,25 @@ def _cleanup_after_deactivate(self) -> None: pass self._original_stdin_fd = None + # Clean up the temp file used to inject stdin, now that fd 0's + # duplicate of it has been closed above (see _inject_message_to_fd0). + stdin_temp_path = getattr(self, '_stdin_temp_path', None) + if stdin_temp_path is not None: + try: + _os.unlink(stdin_temp_path) + except FileNotFoundError: + # already gone (e.g. removed by something else) — fine to + # forget it. + self._stdin_temp_path = None + except OSError: + # e.g. PermissionError on Windows if the fd 0 restore above + # itself failed and the file is still locked. Leave the + # path recorded so a later cleanup call can retry, instead + # of silently losing track of a file we can never clean up. + pass + else: + self._stdin_temp_path = None + # Collect SDK root paths before removing them from sys.path sdk_roots = [p for p in sys.path if 'gltest-direct' in p] diff --git a/tests/gltest_direct/test_stdin_injection.py b/tests/gltest_direct/test_stdin_injection.py new file mode 100644 index 0000000..e79978c --- /dev/null +++ b/tests/gltest_direct/test_stdin_injection.py @@ -0,0 +1,113 @@ +"""Regression tests for the stdin-injection temp file lifecycle. + +See issue #99 (bug 3): unlinking the temp file used to inject the message +context into fd 0 immediately after os.dup2() works on POSIX (the directory +entry is removed but the data stays available via the still-open fd) but +raises PermissionError on Windows, which locks files with open handles. + +The fix defers the unlink until fd 0's duplicate has actually been closed +in VMContext._cleanup_after_deactivate. +""" + +import os + +import pytest + +from gltest.direct import loader +from gltest.direct.vm import VMContext + + +class _FakeAddress: + def __init__(self, data): + self.data = data + + +class _FakeCalldata: + """Minimal calldata stand-in: encode() just needs to produce bytes.""" + + @staticmethod + def encode(_message_data): + return b"fake-encoded-message" + + +@pytest.fixture +def fake_sdk(monkeypatch): + """Avoid requiring a real downloaded SDK for this unit test.""" + monkeypatch.setattr(loader, "import_calldata", lambda: _FakeCalldata()) + monkeypatch.setattr(loader, "import_address", lambda: _FakeAddress) + + +class TestInjectMessageToFd0: + def test_temp_file_is_not_deleted_immediately(self, fake_sdk): + """Regression: the temp file must still exist right after + _inject_message_to_fd0 returns, because fd 0 still references it. + Deleting it immediately is what breaks on Windows.""" + vm = VMContext() + original_stdin = os.dup(0) + try: + loader._inject_message_to_fd0(vm) + + temp_path = getattr(vm, "_stdin_temp_path", None) + assert temp_path is not None, "temp path must be recorded on vm for deferred cleanup" + assert os.path.exists(temp_path), "temp file must not be unlinked while fd 0 still holds it open" + finally: + # Restore real stdin and clean up manually (mirrors _cleanup_after_deactivate) + os.dup2(original_stdin, 0) + os.close(original_stdin) + stdin_fd = getattr(vm, "_original_stdin_fd", None) + if stdin_fd is not None: + os.close(stdin_fd) + temp_path = getattr(vm, "_stdin_temp_path", None) + if temp_path is not None: + os.unlink(temp_path) + + def test_cleanup_after_deactivate_removes_temp_file(self, fake_sdk): + """After the VM context restores stdin, the deferred temp file + must be cleaned up (and cleanup must not raise, mirroring the + try/except OSError guard needed for Windows).""" + vm = VMContext() + original_stdin = os.dup(0) + try: + loader._inject_message_to_fd0(vm) + temp_path = vm._stdin_temp_path + assert os.path.exists(temp_path) + + vm._cleanup_after_deactivate() + + assert not os.path.exists(temp_path), "temp file must be removed after cleanup" + assert vm._stdin_temp_path is None + assert vm._original_stdin_fd is None + finally: + os.dup2(original_stdin, 0) + os.close(original_stdin) + + def test_cleanup_is_safe_when_nothing_was_injected(self): + """Calling cleanup on a VM that never replaced stdin (e.g. ImportError + early-return in _inject_message_to_fd0) must not raise.""" + vm = VMContext() + vm._cleanup_after_deactivate() # should not raise + + def test_cleanup_tolerates_missing_temp_file(self, fake_sdk): + """If the temp file was already removed some other way, cleanup + must swallow the OSError instead of propagating it (same guard + already used for the stdin dup2/close above it).""" + vm = VMContext() + original_stdin = os.dup(0) + try: + loader._inject_message_to_fd0(vm) + # Restore fd 0 before manually unlinking below — otherwise this + # unlink is exactly the "unlink while fd 0 still references the + # file" case this whole fix exists to avoid, and would itself + # raise PermissionError on Windows. vm._original_stdin_fd is + # left as-is so _cleanup_after_deactivate's own restore path is + # still exercised below (redundant dup2 onto the same target, + # then close — harmless). + os.dup2(original_stdin, 0) + os.unlink(vm._stdin_temp_path) # simulate external removal + + vm._cleanup_after_deactivate() # should not raise despite missing file + + assert vm._stdin_temp_path is None + finally: + os.dup2(original_stdin, 0) + os.close(original_stdin)