Skip to content

fix: terminal tool crashes with 'embedded null byte' when command references a binary via absolute path - #79151

Closed
lightnet19 wants to merge 1 commit into
NousResearch:mainfrom
lightnet19:fix/terminal-embedded-null-byte
Closed

fix: terminal tool crashes with 'embedded null byte' when command references a binary via absolute path#79151
lightnet19 wants to merge 1 commit into
NousResearch:mainfrom
lightnet19:fix/terminal-embedded-null-byte

Conversation

@lightnet19

Copy link
Copy Markdown

Summary

Fixes ValueError: embedded null byte crash in the terminal tool when a command references a binary by absolute path (e.g. the Hermes venv python). Fixes the whole bug class, including the sibling call path missed by #76762.

Root cause

cron/lifecycle_guard.py already skips binaries in _read_referenced_script (NUL byte in first chunk -> "nothing to scan", #76762). But tools/terminal_tool.py passes _read_script_in_env as read_remote_script=. When the local read reports "nothing to scan", the guard calls the fallback, which decoded ELF bytes with errors="replace". NUL (U+0000) is valid UTF-8, so it survives into the returned text. The guard then recurses into that NUL-laden text as if it were a shell script; tokenization produces NUL-bearing paths and os.open raises ValueError: embedded null byte, failing every terminal call that references a binary by absolute path.

Changes

  • tools/terminal_tool.py: _read_script_in_env now mirrors _read_referenced_script — returns None when the file chunk contains a NUL byte (binary == nothing to scan); also guards the remote cat output for NUL.
  • cron/lifecycle_guard.py: _read_referenced_script tolerates ValueError from os.open just as it already tolerates it from Path.resolve, so a NUL-bearing path token can never crash the guard.

Tests

Adds two regression tests:

  • test_binary_read_via_remote_callback_does_not_crash_guard — reproduces the gateway path (a read_remote_script callback that decodes a binary with errors="replace")
  • test_nul_bearing_script_path_does_not_crash_guard — defense-in-depth for NUL-bearing path tokens

Full suite: 84 passed. Runtime verified after gateway restart: venv/bin/python -c 'print(1)' previously crashed, now runs clean.

Closes #79148

…erences a binary via absolute path

The gateway lifecycle guard (cron/lifecycle_guard.py) scans command tokens for
referenced shell scripts. _read_referenced_script already skips binaries
(NUL byte in first chunk -> 'nothing to scan', NousResearch#76762), but tools/terminal_tool.py
passes a remote-read fallback (_read_script_in_env) as read_remote_script=. When
the local read reports 'nothing to scan', the guard calls the fallback, which
decoded the ELF bytes with errors='replace'. NUL (U+0000) is valid UTF-8, so it
survives into the returned text. The guard then recursed into that NUL-laden
text as if it were a shell script; tokenization produced paths with embedded
NUL bytes and os.open raised ValueError: embedded null byte, failing every
terminal call that references a binary by absolute path (e.g. venv python).

Fix the whole bug class:
- tools/terminal_tool.py: _read_script_in_env now mirrors _read_referenced_script
  and returns None when the file chunk contains a NUL byte (binary == nothing
  to scan); also guards the remote cat output for NUL.
- cron/lifecycle_guard.py: _read_referenced_script tolerates ValueError from
  os.open just as it already tolerates it from Path.resolve, so a NUL-bearing
  path token can never crash the guard.

Adds two regression tests:
- test_binary_read_via_remote_callback_does_not_crash_guard (reproduces the
  gateway path: read_remote_script decodes a binary with errors='replace')
- test_nul_bearing_script_path_does_not_crash_guard (defense-in-depth)

Full suite: 84 passed. Runtime verified after the gateway was restarted:
venv/bin/python -c 'print(1)' previously crashed, now runs clean.
@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.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels Aug 6, 2026
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 P2 Medium — degraded but workaround exists 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.

fix: terminal tool crashes with 'embedded null byte' when command references a binary via absolute path

3 participants