fix(#4410): pre-check workspace leaf type via lstat + close fd on non-regular (Codex gate v2)

Codex re-gate found two issues in the uncapped-read fix: (1) open_anchored_fd
opens blocking O_RDONLY, which HANGS on a workspace FIFO/special file swapped in
at a regular-file path; (2) the non-regular fstat branch returned without
closing the fd (leak). Fix: lstat the symlink-resolved target BEFORE any open to
skip non-regular leaves (no blocking open on a FIFO), and close the fd on the
non-regular fstat branch. Adds a threaded FIFO-no-hang regression test.
This commit is contained in:
nesquena-hermes committed 2026-06-18 19:22:29 +00:00
1 parent 94909590ad
commit 9a54fe9aeb
2 files changed
+47 -2

No files matched your search

+19 -2
View File
@@ -169,22 +169,39 @@ def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None:
target = safe_resolve_ws(workspace_root, rel_path)
except (ValueError, OSError):
return None
# Pre-check the leaf type WITHOUT opening it: open_anchored_fd opens with
# blocking O_RDONLY, which would HANG on a FIFO/special file swapped in at a
# regular-file path. lstat on the already-symlink-resolved target tells us the
# type without an open, so we never block on a non-regular leaf. (The old
# Path.is_file() guard likewise never opened a FIFO.)
try:
if not stat.S_ISREG(os.lstat(target).st_mode):
return None
except OSError:
return None
try:
fd = open_anchored_fd(workspace_root, target, want_dir=False)
except (FileNotFoundError, ValueError, OSError):
return None
# From here the fd is owned; make sure every path closes it exactly once.
try:
st = os.fstat(fd)
if not stat.S_ISREG(st.st_mode):
os.close(fd)
return None
with os.fdopen(fd, "rb") as fh:
return fh.read().decode(errors="replace")
fh = os.fdopen(fd, "rb")
except OSError:
try:
os.close(fd)
except OSError:
pass
return None
# os.fdopen succeeded → the file object owns the fd and will close it.
try:
with fh:
return fh.read().decode(errors="replace")
except OSError:
return None
# ── Public API functions (called from routes.py) ────────────────────────────
@@ -125,3 +125,31 @@ def test_checkpoint_diff_renders_large_workspace_file_as_modified_not_deleted(tm
"(the capped read_file_content path regressed this): got "
f"{statuses.get('big.txt')!r}"
)
def test_checkpoint_diff_skips_workspace_fifo_without_hanging(tmp_path, monkeypatch):
"""Regression (Codex gate): a workspace FIFO/special file at a checkpoint-
tracked regular-file path must NOT hang the diff (open_anchored_fd opens
blocking O_RDONLY, which blocks forever on a FIFO with no writer) and must
not leak an fd — the leaf type is pre-checked via lstat before any open.
"""
import threading
workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
_commit_checkpoint_file(ckpt_dir, "pipe.txt", "checkpoint content\n")
# Replace the workspace-side file with a FIFO (no writer → blocking open hangs).
os.mkfifo(workspace / "pipe.txt")
result_box = {}
def _run():
result_box["r"] = rollback.get_checkpoint_diff(str(workspace), checkpoint)
t = threading.Thread(target=_run, daemon=True)
t.start()
t.join(timeout=10)
assert not t.is_alive(), "get_checkpoint_diff hung on a workspace FIFO"
# FIFO workspace side is treated as absent → checkpoint file renders as deleted,
# never blocks and never leaks the secret/hangs.
statuses = {f["file"]: f.get("status") for f in result_box["r"]["files_changed"]}
assert statuses.get("pipe.txt") == "deleted"