Repository navigation
fix(sandbox): pin utf-8 decoding on the local sandbox backend's subprocess calls - #5905
Conversation
…ocess calls The local container backend shells out to docker / the container runtime with text-mode subprocess calls that do not pass an encoding, so output is decoded with the platform's preferred encoding. On hosts whose ANSI code page is not UTF-8 (cp936 Chinese Windows, cp1252 Western European), any non-ASCII byte in the CLI output raises inside subprocess's reader thread: stdout/stderr come back None, the JSON-parsing call sites then crash on None or silently mis-parse, and stderr diagnostics are dropped. Pin encoding="utf-8" with errors="replace" on every text-mode call in the module. The container CLIs write UTF-8 on every platform; replace keeps the parseable portion of any non-conforming output alive instead of discarding it wholesale.
|
|
github-actions
Bot
added
risk:medium
|
@siye566 Thanks for your contribution, please click the CLA button to sign the CLA first. |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Verified against head e9a951c: all 22 text-mode subprocess.run calls in local_backend.py now pass encoding="utf-8" + errors="replace" (counts are exactly 22/22 in the head file), the one remaining binary-mode call (the copied docker-exec in _start_network_proxy) intentionally avoids locale decoding via read_bytes() and an explicit .decode(errors='replace') on stderr, and no other file in the aio_sandbox package shells out in text mode — so the fix is complete for the module and consistent with the repo's explicit-encoding-over-locale convention. Behavior is byte-identical on UTF-8 locales and strictly better on cp936-style hosts. One suggestion below on making the new static guard structurally robust rather than count-based.
| source = Path(local_backend_module.__file__).read_text(encoding="utf-8") | ||
| text_modes = len(re.findall(r"\btext=True\b", source)) | ||
| pinned = len(re.findall(r'encoding="utf-8"', source)) | ||
| assert text_modes == 0 or pinned >= text_modes, f"{text_modes} text=True call(s) but only {pinned} encoding pins; new text-mode subprocess calls must pass encoding='utf-8' + errors='replace'" |
There was a problem hiding this comment.
This guard is count-based module-wide, which leaves two gaps it claims to close. (1) It compares total text=True occurrences against total encoding="utf-8" occurrences rather than pairing them per call, so once any unrelated encoding="utf-8" literal shows up in the module (a docstring, an open(..., encoding="utf-8"), a comment), a new unpinned text=True subprocess call passes the guard. Counts are exactly 22/22 today so it is tight right now, but that balance is accidental, not structural. (2) It never inspects errors= at all, even though the module contract (the docstrings above and the PR) requires errors="replace"; test_docker_subprocess_calls_pin_utf8_decoding does assert errors=, but it only observes the calls on the runtime-detection/_docker_server_is_desktop path it happens to drive, not the other ~20 sites. Suggestion: walk the module with ast and, for every subprocess.run(...) call carrying text=True, assert the call's own keywords include both encoding="utf-8" and errors="replace" — that makes the guard per-call and closes both gaps.
Fixes #5904
Why
While auditing the local container sandbox backend I found all 22 text-mode
subprocess.runcalls decode docker / container-runtime output with the platform's preferred encoding (noencoding=anywhere in the module). On hosts whose ANSI code page is not UTF-8 — cp936 Chinese Windows is the big one for this project's user base — any non-ASCII byte in the CLI output raises insidesubprocess's reader thread, whichsubprocess.runswallows:stdout/stderrcome backNone. The JSON-parsing call sites then crash onNone(network-proxy pending/deny-pending paths) or silently return an empty result (json.loads(result.stdout or "[]")), and stderr diagnostics are dropped. Issue #5904 has the decoding-mechanism reproduction and the per-shape breakdown.What changed
Every text-mode subprocess call in
local_backend.pynow pinsencoding="utf-8"witherrors="replace". The container CLIs write UTF-8 on every platform, so UTF-8-locale hosts are byte-identical to before; non-UTF-8-locale hosts stop losing entire streams, and stray non-conforming bytes degrade to replacement characters instead of discarding the rest of the output. No call-site semantics changed: same commands, same parsing, same error handling.Surface area
docker/or sandboxed executionbackend/tests/test_aio_sandbox_local_backend.py(no runtime behavior change for UTF-8 locales)Test
test_docker_subprocess_calls_pin_utf8_decoding— drives the docker-detection path with a recorded fakesubprocess.runand assertsencoding="utf-8"/errors="replace"are passed.test_every_text_mode_subprocess_call_pins_utf8— static guard: the module must never carry atext=Truecall without anencoding="utf-8"pin, so future call sites can't reintroduce the gap. Both tests fail onmainand pass with this change.backend/tests/test_aio_sandbox_local_backend.py: 104 passed / 3 skipped. The two pre-existing failures intest_aio_sandbox_provider.py(test_get_lark_cli_runtime_mounts_uses_user_auth_dirs,test_get_extra_mounts_provisioner_payload_has_unique_container_paths) fail identically on a cleanmaincheckout on this machine and are unrelated (Windows-specific fixtures).ruff check/ruff format --checkclean on both files.Notes