fix(file-ops): stop read_file blocking forever on non-regular files - #82075
fix(file-ops): stop read_file blocking forever on non-regular files#82075Drexuxux wants to merge 1 commit into
Conversation
The size probe every read path starts with — `wc -c < path` — opens the path. On a FIFO with no writer, a socket, or a character device that never reaches EOF, that read never returns, and read_file/read_file_raw/ read_file_bytes all pass no timeout to _exec. The turn wedges until the process is killed. The device blocklist in tools/file_tools.py cannot close this: it matches literal /dev/* names, so it can only ever cover paths someone thought to enumerate. A FIFO is a file type and can sit at any path. Gate the probe behind `[ -f ]`, which stats instead of opening, and report a path that exists but is not a regular file as such. A missing path keeps its existing not-found handling.
|
This was generated by AI during triage. Summary: Problems:
Solution:
Checked against |
|
Salvaged and merged in #83008 — your sentinel mechanism landed verbatim with your authorship preserved (e0b5005): the [ -f ] check folded into the existing wc -c probe, covering read_file, read_file_raw, and read_file_bytes on every backend with zero added round-trips. Excellent root-cause writeup — the 'a FIFO is a file type, not a name; no blocklist can enumerate it' framing is exactly right, and it now complements the host-side stat guard from #82792. Only additions on top were test-mock adaptations for the changed first-command shape. Thank you! |
What
Every read path in
tools/file_operations.pystarts with the same size probe:wcopens the path. On a FIFO with no writer, a socket, or a character device that never reaches EOF, that read never returns — andread_file,read_file_rawandread_file_bytesall call_execwithout a timeout. There is no deadline anywhere in the call path, so the turn wedges until the process is killed.The guard that exists for this today is a list of names. e3f8347 (2026-03-31, feat(file_tools): harden read_file with size guard, dedup, and device blocking) introduced
_BLOCKED_DEVICE_PATHSwith the/dev/*entries, and the list has been extended by name ever since — most recently 868fa95 (2026-07-01, fix(security): block /proc/*/auxv and /proc/*/pagemap read leaks). Each extension adds paths someone thought to enumerate, which is the right shape for/devand/procaliases.It cannot reach this case. A FIFO is a file type, not a name: it can sit at any path in a workspace, created by a build system, a daemon, or a test fixture. No blocklist can enumerate it. Measured against a FIFO with no writer,
read_file_toolwas still running after 20 s with nothing in the call path able to interrupt it.The fix
Gate the probe behind
[ -f ], which is a stat rather than an open, and route all three readers through one helper:[ -f ]answers exactly the question the size probe needs — regular file, symlinks followed — without touching the contents. A path that exists but is not a regular file reports the sentinel so the caller can say so instead of claiming the file is missing; a genuinely absent path still exits non-zero and keeps its existing not-found handling, including the similar-file suggestions.This is independent of the name blocklist and does not change it. The two compose: the blocklist keeps rejecting known device aliases before any I/O, and this bounds everything it cannot enumerate.
Tests and results
New
TestNonRegularFileReadsintests/tools/test_file_read_guards.py. Each read runs on a worker thread with a wall clock, so a blocking call fails as an assertion instead of hanging the suite:not a regular fileinstead of blocking; skips whereos.mkfifois unavailableAll three readers were exercised directly against a FIFO and against a regular file: each returned a bounded error for the FIFO in under a second, and read the regular file unchanged.
tests/tools/test_file_operations.py::TestShellFileOpsHelpers— 8 passed.test_read_file_uses_bash_safe_windows_pathspinned the probe's exact command string, so it moves to the new one; what it protects — that every command the read path issues carries the bash-safe converted path — is unchanged.