From e0b50059857fa839d36f06bab0615512f3ae0d2d Mon Sep 17 00:00:00 2001 From: Drexuxux Date: Sun, 9 Aug 2026 02:29:05 +0300 Subject: [PATCH] fix(file-ops): stop read_file blocking forever on non-regular files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/tools/test_file_operations.py | 12 +++- tests/tools/test_file_read_guards.py | 86 ++++++++++++++++++++++++++++ tools/file_operations.py | 63 ++++++++++++++++---- 3 files changed, 148 insertions(+), 13 deletions(-) diff --git a/tests/tools/test_file_operations.py b/tests/tools/test_file_operations.py index 37b5b08430bce..d23eeac5a6b4f 100644 --- a/tests/tools/test_file_operations.py +++ b/tests/tools/test_file_operations.py @@ -303,7 +303,9 @@ class TestShellFileOpsHelpers: def side_effect(command, **kwargs): commands.append(command) - if command.startswith("wc -c"): + # The size probe gates `wc -c` behind `[ -f ]` so a FIFO or device + # cannot block the read; it still reports a plain byte count. + if command.startswith("if [ -f ") or command.startswith("wc -c"): return {"output": "5\n", "returncode": 0} if command.startswith("head -c") and "| base64" in command: import base64 as b64 @@ -321,7 +323,13 @@ class TestShellFileOpsHelpers: result = ops.read_file(r"C:\Users\alice\notes.txt") assert result.error is None - assert commands[0] == "wc -c < '/c/Users/alice/notes.txt' 2>/dev/null" + assert commands[0] == ( + "if [ -f '/c/Users/alice/notes.txt' ]; " + "then wc -c < '/c/Users/alice/notes.txt' 2>/dev/null; " + "elif [ -e '/c/Users/alice/notes.txt' ]; " + "then echo __hermes_not_regular__; " + "else exit 1; fi" + ) assert commands[1] == "head -c 1000 '/c/Users/alice/notes.txt' 2>/dev/null | base64" assert commands[2] == "sed -n '1,2000p' '/c/Users/alice/notes.txt'" assert commands[3] == "wc -l < '/c/Users/alice/notes.txt'" diff --git a/tests/tools/test_file_read_guards.py b/tests/tools/test_file_read_guards.py index 52eaf50ed213d..02a0f8e2a8c48 100644 --- a/tests/tools/test_file_read_guards.py +++ b/tests/tools/test_file_read_guards.py @@ -211,6 +211,92 @@ class TestDevicePathBlocking(unittest.TestCase): mock_ops.assert_not_called() +# --------------------------------------------------------------------------- +# Non-regular files (FIFOs, sockets, directories) +# --------------------------------------------------------------------------- + +class TestNonRegularFileReads(unittest.TestCase): + """Blocking paths the device blocklist structurally cannot cover. + + The blocklist matches literal ``/dev/*`` names. A FIFO is a file *type* + and can sit at any path, so no name list catches it. Reading one with no + writer blocks in the size probe, and the read helpers pass no timeout, so + the turn wedges until the process is killed. + + Each read runs on a worker thread with a wall clock: a thread still alive + at the deadline means the call blocked, which fails as an assertion + instead of hanging the suite. + """ + + DEADLINE_SECONDS = 20.0 + + def _read_within_deadline(self, path, task_id): + import threading + + box = {} + + def call(): + try: + box["raw"] = read_file_tool(path, task_id=task_id) + except BaseException as exc: # noqa: BLE001 + box["exc"] = exc + + worker = threading.Thread(target=call, daemon=True) + worker.start() + worker.join(self.DEADLINE_SECONDS) + self.assertFalse( + worker.is_alive(), + f"read_file_tool({path!r}) still running after " + f"{self.DEADLINE_SECONDS:.0f}s — the read blocked", + ) + if "exc" in box: + raise box["exc"] + return json.loads(box["raw"]) + + def test_read_file_tool_on_fifo_errors_instead_of_blocking(self): + if not hasattr(os, "mkfifo"): + self.skipTest("platform has no os.mkfifo") + with tempfile.TemporaryDirectory() as tmpdir: + fifo_path = os.path.join(tmpdir, "pipe") + try: + os.mkfifo(fifo_path) + except (OSError, NotImplementedError) as exc: + self.skipTest(f"mkfifo unavailable: {exc}") + + result = self._read_within_deadline(fifo_path, "fifo_read_test") + + self.assertIn("error", result) + self.assertIn("not a regular file", result["error"]) + + def test_read_file_tool_on_directory_errors_instead_of_blocking(self): + with tempfile.TemporaryDirectory() as tmpdir: + result = self._read_within_deadline(tmpdir, "dir_read_test") + + self.assertIn("error", result) + self.assertIn("not a regular file", result["error"]) + + def test_regular_file_still_reads(self): + """The guard must not cost ordinary reads their content.""" + with tempfile.TemporaryDirectory() as tmpdir: + target = os.path.join(tmpdir, "notes.txt") + with open(target, "w", encoding="utf-8") as handle: + handle.write("first line\nsecond line\n") + + result = self._read_within_deadline(target, "regular_read_test") + + self.assertNotIn("error", result) + self.assertIn("second line", result["content"]) + + def test_missing_file_still_reports_not_found(self): + """An absent path keeps the not-found wording, not the type error.""" + with tempfile.TemporaryDirectory() as tmpdir: + missing = os.path.join(tmpdir, "no-such-file.txt") + result = self._read_within_deadline(missing, "missing_read_test") + + self.assertIn("error", result) + self.assertNotIn("not a regular file", result["error"]) + + # --------------------------------------------------------------------------- # Character-count limits # --------------------------------------------------------------------------- diff --git a/tools/file_operations.py b/tools/file_operations.py index de47b89c3add3..2b6c2a8560509 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -732,6 +732,10 @@ DEFAULT_READ_LIMIT = 2000 DEFAULT_SEARCH_OFFSET = 0 DEFAULT_SEARCH_LIMIT = 50 +# Echoed by the size probe when the path exists but is not a regular file. +# `wc -c` prints only digits, so this can never collide with a real size. +NOT_REGULAR_SENTINEL = "__hermes_not_regular__" + def _coerce_int(value: Any, default: int) -> int: """Best-effort integer coercion for tool pagination inputs.""" @@ -1220,6 +1224,40 @@ class ShellFileOperations(FileOperations): # READ Implementation # ========================================================================= + def _size_probe_cmd(self, path: str) -> str: + """Byte size of a regular file, without opening one that never ends. + + ``wc -c < path`` opens the path. On a FIFO with no writer, a socket, + or a character device like /dev/zero that never reaches EOF, that + read blocks forever — and the read helpers pass no timeout to + :meth:`_exec`, so the turn wedges until the process is killed. The + device blocklist in ``tools/file_tools.py`` cannot cover this: it + matches literal ``/dev/*`` names, while a FIFO is a file *type* and + can sit at any path. + + ``[ -f ]`` is a stat, not an open — it answers exactly the question + the size probe needs (regular file, symlinks followed) without + touching the contents. Non-regular paths that exist report the + sentinel so callers can say so instead of claiming the file is + missing; a genuinely absent path still exits non-zero. + """ + arg = self._escape_shell_arg(path) + return ( + f"if [ -f {arg} ]; then wc -c < {arg} 2>/dev/null; " + f"elif [ -e {arg} ]; then echo {NOT_REGULAR_SENTINEL}; " + f"else exit 1; fi" + ) + + @staticmethod + def _not_regular_error(path: str) -> ReadResult: + """Error for a path that exists but would block if read.""" + return ReadResult( + error=( + f"Cannot read '{path}': not a regular file (directory, FIFO, " + "socket, or device). Reading it could block indefinitely." + ) + ) + def read_file(self, path: str, offset: int = 1, limit: int = 2000) -> ReadResult: """ Read a file with pagination, binary detection, and line numbers. @@ -1237,10 +1275,9 @@ class ShellFileOperations(FileOperations): offset, limit = normalize_read_pagination(offset, limit) - # Check if file exists and get size (wc -c is POSIX, works on Linux + macOS) - stat_cmd = f"wc -c < {self._escape_shell_arg(path)} 2>/dev/null" - stat_result = self._exec(stat_cmd) - + # Check if file exists and get size (POSIX, works on Linux + macOS) + stat_result = self._exec(self._size_probe_cmd(path)) + if stat_result.exit_code != 0: # File not found. Before failing, try unicode-equivalent # spellings — NFC/NFD, narrow no-break space, curly quotes @@ -1260,8 +1297,10 @@ class ShellFileOperations(FileOperations): return result # No equivalent spelling — suggest similar files return self._suggest_similar_files(path) - + stat_output = _strip_terminal_fence_leaks(stat_result.stdout) + if stat_output.strip() == NOT_REGULAR_SENTINEL: + return self._not_regular_error(path) try: file_size = int(stat_output.strip()) except ValueError: @@ -1473,11 +1512,12 @@ class ShellFileOperations(FileOperations): Uses cat so the full file is returned regardless of size. """ path = self._expand_path(path) - stat_cmd = f"wc -c < {self._escape_shell_arg(path)} 2>/dev/null" - stat_result = self._exec(stat_cmd) + stat_result = self._exec(self._size_probe_cmd(path)) if stat_result.exit_code != 0: return self._suggest_similar_files(path) stat_output = _strip_terminal_fence_leaks(stat_result.stdout) + if stat_output.strip() == NOT_REGULAR_SENTINEL: + return self._not_regular_error(path) try: file_size = int(stat_output.strip()) except ValueError: @@ -1514,13 +1554,14 @@ class ShellFileOperations(FileOperations): def read_file_bytes(self, path: str, max_bytes: Optional[int] = None) -> ReadResult: """Read binary-safe bytes from any shell-backed environment.""" path = self._expand_path(path) - stat_result = self._exec( - f"wc -c < {self._escape_shell_arg(path)} 2>/dev/null" - ) + stat_result = self._exec(self._size_probe_cmd(path)) if stat_result.exit_code != 0: return ReadResult(error=f"File not found: {path}") + stat_output = _strip_terminal_fence_leaks(stat_result.stdout) + if stat_output.strip() == NOT_REGULAR_SENTINEL: + return self._not_regular_error(path) try: - file_size = int(_strip_terminal_fence_leaks(stat_result.stdout).strip()) + file_size = int(stat_output.strip()) except ValueError: return ReadResult(error=f"Could not determine file size: {path}") if max_bytes is not None and file_size > max_bytes: