From 2fa292c0a614f955c7c8f0de62d3931b1fcd4dc8 Mon Sep 17 00:00:00 2001 From: ygd58 Date: Sat, 1 Aug 2026 13:52:27 +0000 Subject: [PATCH 1/2] fix: defer stdin temp-file unlink until fd 0 is restored (Windows) _inject_message_to_fd0() created a temp file, dup2()'d it onto fd 0, then immediately unlinked the path while fd 0 still held it open. This works on POSIX (unlinking an open file just removes the directory entry; the data stays available via the open fd) but raises PermissionError on Windows, which locks files with open handles even via a duplicated fd: PermissionError: [WinError 32] The process cannot access the file because it is being used by another process Part 3 of issue #99 (Windows: genlayer-test direct-mode completely broken). Bugs 1 and 2 in that issue (Buffer import on Python 3.10, SDK asset availability) live in genlayer-py and genvm releases respectively, not in this repo. Fix: store the temp path on the VM context instead of unlinking immediately. VMContext._cleanup_after_deactivate() already restores fd 0 (dup2 back + close) when the VM context deactivates; it now also unlinks the deferred temp file right after, once fd 0's duplicate has actually been closed. Same try/except OSError guard as the existing stdin restore, so cleanup can't raise even if the file is already gone. No behavior change on POSIX beyond deleting the file slightly later (same VM-context lifecycle, still cleaned up every time). Added tests/gltest_direct/test_stdin_injection.py: verifies the temp file is not deleted while still referenced, that cleanup removes it, that cleanup is a no-op when nothing was injected, and that cleanup tolerates the file already being gone. All 4 fail against the pre-fix code and pass with the fix. --- gltest/direct/loader.py | 8 +- gltest/direct/vm.py | 10 ++ tests/gltest_direct/test_stdin_injection.py | 105 ++++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 tests/gltest_direct/test_stdin_injection.py diff --git a/gltest/direct/loader.py b/gltest/direct/loader.py index 09f8c7f..09597c0 100644 --- a/gltest/direct/loader.py +++ b/gltest/direct/loader.py @@ -321,7 +321,13 @@ def _inject_message_to_fd0(vm: "VMContext") -> None: os.dup2(fd, 0) finally: os.close(fd) - os.unlink(path) + + # Defer unlinking 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. + vm._stdin_temp_path = path def _load_module(contract_path: Path) -> Any: diff --git a/gltest/direct/vm.py b/gltest/direct/vm.py index a88f5af..74c39b4 100644 --- a/gltest/direct/vm.py +++ b/gltest/direct/vm.py @@ -507,6 +507,16 @@ 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 OSError: + pass + 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..e71dd06 --- /dev/null +++ b/tests/gltest_direct/test_stdin_injection.py @@ -0,0 +1,105 @@ +"""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) + 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) From ab95130e76ecc7147e2cd7276ce8e3e62727c600 Mon Sep 17 00:00:00 2001 From: ygd58 Date: Thu, 3 Sep 2026 13:31:32 +0000 Subject: [PATCH 2/2] fix: address CodeRabbit review findings on #104 1. loader.py: record vm._stdin_temp_path immediately after mkstemp() succeeds, before os.write/os.dup/os.dup2 (any of which can raise). Previously a failure in that window would leak the temp file with nothing recorded to find it for cleanup. 2. vm.py: _cleanup_after_deactivate only clears _stdin_temp_path when the unlink actually succeeds or the file's confirmed already gone (FileNotFoundError). Previously any OSError (e.g. Windows PermissionError if the fd 0 restore itself failed and the file's still locked) silently discarded the path, permanently losing the ability to retry cleanup on a later call. 3. test_stdin_injection.py: test_cleanup_tolerates_missing_temp_file restores fd 0 before manually unlinking to simulate external removal. It previously unlinked while fd 0 still referenced the file - exactly the "unlink while still open" case this whole fix exists to prevent, so the regression test itself would have failed with PermissionError on the Windows this fix targets. All three per CodeRabbit's review on #104. Full stdin-injection suite: 4/4 passed. Full gltest_direct suite: 39 passed (same 11 pre-existing, unrelated environment failures as before - SDK version mismatch in this sandbox, present identically before this change). --- gltest/direct/loader.py | 16 ++++++++++------ gltest/direct/vm.py | 11 ++++++++++- tests/gltest_direct/test_stdin_injection.py | 8 ++++++++ 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/gltest/direct/loader.py b/gltest/direct/loader.py index 09597c0..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 @@ -322,12 +326,12 @@ def _inject_message_to_fd0(vm: "VMContext") -> None: finally: os.close(fd) - # Defer unlinking 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. - vm._stdin_temp_path = 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 74c39b4..0cd2734 100644 --- a/gltest/direct/vm.py +++ b/gltest/direct/vm.py @@ -513,9 +513,18 @@ def _cleanup_after_deactivate(self) -> 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 - self._stdin_temp_path = None + 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 index e71dd06..e79978c 100644 --- a/tests/gltest_direct/test_stdin_injection.py +++ b/tests/gltest_direct/test_stdin_injection.py @@ -95,6 +95,14 @@ def test_cleanup_tolerates_missing_temp_file(self, fake_sdk): 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