fix(u6): a FIFO at the registry path hung the render, and unquote leaked control characters
Both found by the in-session adversarial pass while the cold panels were still out. The first is this repo's own 2026-09-22 lesson recurring in a new file. _read_bytes bounded the READ and its docstring claimed that closed the named-pipe hole. It does not: open() blocks on a FIFO with no writer, before any byte cap can apply. read_benches runs on the board page's render path, so one FIFO there is a request that never returns and, with enough hits, the threadpool behind every route. Guarded with S_ISREG before the open, which is what marks.py has done since it learned the same thing. The bounded read stays for the case a stat cannot answer: a regular file that grew between the two. booth_target handed back whatever unquote produced, including NUL and newline. Neither can name a directory, and unfiltered they reach is_dir() -- which raises ValueError on an embedded NUL, and ValueError is not an OSError, so it escapes the dead marker's guard -- plus the refusal message the CLI prints and the marker the board renders. Both tests are written to go red under the exact change that defeats them: the FIFO test blocks rather than fails if the regular-file check is removed, and the control-character rows need their own case because %2e%2e and %2f stay green without the clause.
This commit is contained in:
+18
-6
@@ -26,6 +26,7 @@ from __future__ import annotations
|
||||
import fcntl
|
||||
import json
|
||||
import os
|
||||
import stat
|
||||
from dataclasses import dataclass, replace
|
||||
from datetime import datetime, timezone
|
||||
from pathlib import Path
|
||||
@@ -177,14 +178,25 @@ def _bench_from(bench_id: str, row: object) -> Bench:
|
||||
|
||||
|
||||
def _read_bytes(path: Path) -> bytes:
|
||||
"""Read at most BENCHES_MAX_BYTES + 1 bytes.
|
||||
"""Read at most BENCHES_MAX_BYTES + 1 bytes from a REGULAR FILE.
|
||||
|
||||
BOUNDS THE READ, NEVER THE STAT. A FIFO reports st_size 0 and then blocks
|
||||
forever; a size cap that trusts `st_size` inherits a meaning it does not
|
||||
have, and the 2026-09-22 incident in this repo was exactly that — a bound
|
||||
that opened a service-wide hang. Reading one byte past the cap is how you
|
||||
learn you are over it without reading the rest.
|
||||
REGULAR-FILE FIRST, THEN SIZE, THEN A BOUNDED READ — in that order, and the
|
||||
order is the whole point. A named pipe blocks in `open()`, before any byte
|
||||
cap can apply: bounding the read does NOT close that hole, and an earlier
|
||||
draft of this module claimed it did while hanging on the first FIFO put at
|
||||
this path. `read_benches` is on the board page's render path, so that hang
|
||||
is a request that never returns and, with enough of them, the threadpool
|
||||
behind every route. `marks.py` learned this on 2026-09-22 and guards with
|
||||
`S_ISREG`; this is the same guard, not a new idea.
|
||||
|
||||
The bounded read stays, for the case the stat cannot answer: a regular file
|
||||
that GREW between the stat and the read.
|
||||
"""
|
||||
st = os.stat(path)
|
||||
if not stat.S_ISREG(st.st_mode):
|
||||
raise ValueError(f"{path.name} is not a regular file")
|
||||
if st.st_size > BENCHES_MAX_BYTES:
|
||||
raise ValueError(f"registry is larger than {BENCHES_MAX_BYTES} bytes")
|
||||
with path.open("rb") as fh:
|
||||
return fh.read(BENCHES_MAX_BYTES + 1)
|
||||
|
||||
|
||||
@@ -243,4 +243,10 @@ def booth_target(url: str) -> str | None:
|
||||
return None
|
||||
if not name or name.startswith(".") or "/" in name or "\\" in name or ".." in name:
|
||||
return None
|
||||
# `unquote` will happily hand back a NUL or a newline, and neither can name
|
||||
# a directory. Unfiltered they reach `is_dir()` (ValueError on an embedded
|
||||
# NUL, which is NOT an OSError and so escapes the marker's guard), the
|
||||
# refusal message the CLI prints, and the marker the board renders.
|
||||
if any(ch in name for ch in "\x00") or any(ord(ch) < 0x20 for ch in name):
|
||||
return None
|
||||
return name
|
||||
|
||||
@@ -540,3 +540,42 @@ def test_a_bad_url_posted_to_the_route_does_not_500(tmp_path):
|
||||
follow_redirects=False)
|
||||
assert r.status_code in (302, 303, 400)
|
||||
assert c.get("/b/links/").status_code == 200
|
||||
|
||||
|
||||
# ---- found by the in-session adversarial pass, after the cold panels shipped -
|
||||
|
||||
|
||||
def test_a_fifo_at_the_registry_path_cannot_hang_the_render(tmp_path):
|
||||
"""A NAMED PIPE IS NOT A REGULAR FILE, AND open() BLOCKS ON IT.
|
||||
|
||||
This is the 2026-09-22 lesson recurring in a new file: a size cap that
|
||||
bounds the READ does not help, because the hang is in `open()` — a FIFO
|
||||
with no writer blocks there forever, before a single byte is bounded.
|
||||
`read_benches` runs on the board page's render path, so one FIFO would hang
|
||||
that request and, with enough hits, the threadpool behind every route.
|
||||
|
||||
The guard is a REGULAR-FILE check before the open, which is what marks.py
|
||||
already does (`stat.S_ISREG`). Defeating change: reverting to `path.open()`
|
||||
guarded only by a byte cap — which is what this unit shipped first, while
|
||||
its docstring claimed the cap closed exactly this hole.
|
||||
"""
|
||||
os.mkfifo(tmp_path / BENCHES_FILE)
|
||||
benches, err = read_benches(tmp_path) # must RETURN, not block
|
||||
assert benches == [] and err
|
||||
|
||||
|
||||
def test_a_directory_at_the_registry_path_is_an_error_not_a_crash(tmp_path):
|
||||
(tmp_path / BENCHES_FILE).mkdir()
|
||||
benches, err = read_benches(tmp_path)
|
||||
assert benches == [] and err
|
||||
|
||||
|
||||
@pytest.mark.parametrize("encoded", ["%00", "%0a", "%0d", "%09", "%1b"])
|
||||
def test_a_control_character_is_not_an_addressable_booth(encoded):
|
||||
"""`unquote` happily produces a NUL or a newline, and neither can name a
|
||||
real directory. Left unfiltered they reach `is_dir()` (which raises
|
||||
ValueError on an embedded NUL on some paths), the refusal message the CLI
|
||||
prints, and the marker the board renders. Defeating change: dropping the
|
||||
control-character clause — the `%2e%2e` and `%2f` rows above stay green
|
||||
under it, so this needs its own."""
|
||||
assert booth_target(f"http://h:8090/b/{encoded}/") is None
|
||||
|
||||
Reference in New Issue
Block a user