Skip to content

fix(cron): distinguish binary skip from missing file in referenced-script scan - #80164

Closed
applex250 wants to merge 1 commit into
NousResearch:mainfrom
applex250:fix/lifecycle-guard-binary-skip
Closed

fix(cron): distinguish binary skip from missing file in referenced-script scan#80164
applex250 wants to merge 1 commit into
NousResearch:mainfrom
applex250:fix/lifecycle-guard-binary-skip

Conversation

@applex250

Copy link
Copy Markdown

Summary

_read_referenced_script in cron/lifecycle_guard.py returns (None, False) for two very different cases:

  1. the referenced local file doesn't exist (or is unreadable), and
  2. the referenced file is a binary (contains NUL bytes — ELF/Mach-O/PE/images).

Callers treat None as "local file missing" and fall back to read_remote_script (terminal_tool's _read_script_in_env), which decodes the bytes as UTF-8 text and feeds machine-code junk back into the recursion. That junk gets tokenized into bogus script paths containing embedded NUL bytes — the exact crash chain #77703 fixed at the os.open site (ValueError: embedded null byte) can still re-enter through the remote-read fallback path.

This PR makes the "binary, skip" case return "" instead of None, so callers never mistake a binary for a missing file and never invoke the remote-read fallback on binary content.

How it was found

Reproduction: a multi-line inline python3 -c command containing an absolute path inside parentheses (e.g. Image.open('/abs/path.png')). The guard's line-based tokenizer mis-parses the unquoted -c payload, picks the path up as a "referenced script", reads the PNG as binary → skips via the NUL check → None → remote-read fallback decodes the PNG as text → recursion tokenizes junk → os.open() raises ValueError: embedded null byte and the guard crashes instead of returning a clean block.

ValueError: open: embedded null character in path

The upstream os.open guard from #77703 prevents some of these paths from crashing, but the fallback re-entry makes the crash still reachable — this is the remaining defense-in-depth gap.

Changes

  • cron/lifecycle_guard.py: binary (NUL-byte) files return ("", False) — "nothing to scan, and don't bother the remote-read fallback" — instead of (None, False).

No behavior change for real shell scripts: text scripts still return their decoded text; missing files still return None (so normal path validation errors are unaffected).

Test plan

  • Reproduction command (multi-line python3 -c with an absolute PNG path in Image.open(...)) no longer crashes the guard — returns a clean result.
  • Regression: systemctl restart hermes-gateway, pkill -f hermes-gateway, sh -c 'hermes gateway stop', and shell scripts referencing gateway commands are still blocked.
  • Plain commands (ls -la) and absolute-executable invocations (/usr/bin/python3 script.py) still pass.
  • Upstream tests/ for the guard run clean.

Related: #77703 (same crash chain, different entry point — already fixed upstream).

…ript scan

_read_referenced_script returns (None, False) both when a referenced file
doesn't exist and when it's a binary (NUL bytes).  Callers treat None as
'local file missing' and fall back to read_remote_script, which decodes
the binary as text and feeds machine-code junk paths back into the
recursion — re-triggering the embedded-null-byte crash chain (NousResearch#77703
fixed the os.open ValueError, but the fallback re-entry path remained).

Return ('', False) for binaries so callers see 'binary, skip' and never
invoke the remote-read fallback on binary content.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management needs-decision Awaiting maintainer decision before any implementation sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Aug 6, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Closing as superseded by #80258, which fixes this whole bug class architecturally rather than per-callsite: path candidates are sanitized once at the ingestion boundary (NUL/empty/unexpandable tokens rejected before any OS call), text from any read_remote_script callback is sanitized at the recursion boundary (NUL = binary = nothing to scan; >1 MiB = fail closed), the remote fallback read is bounded at the source (head -c, so oversized binaries never cross the wire), and the public guard is total by construction — an unexpected walk failure logs and falls back to the direct-scan verdict instead of breaking every terminal command.

Your report and fix targeted a real member of this class — thank you. The per-callsite patches kept leaving sibling frames exposed (#76762#77703#77780#78256 each crashed one frame away from the previous fix), which is why we went with the boundary fix instead of merging the fragments individually. #80258 carries regression tests for the NUL-path, binary-callback, oversized-read, unset-HOME, and walk-crash cases plus an adversarial never-raises sweep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants