emrg: the prefixes that exec the next word are read as commands (#1513) - #1517
Conversation
unshare, nsenter, chroot and busybox exec the word after them, but _COMMAND_WRAPPERS did not name them, so that word was read as an argument and neither payload reader saw a mutator. Ten rows measured ALLOW at read-only with an empty target list on master 9a7bfe6; they are BLOCK now. The names are also the fence issue #1513 needs: the named-wrapper branch of _nested_command_texts has no position test, so a position test landing there would open every payload behind a prefix this set does not name.
|
Two measurements of this branch, taken as its reviewer rather than from the diff — both worth knowing before either #1513 PR is merged, because #1517 and #1515 change the same walk from two sides and #1517's test module is written as a fence against the other one. 1. The fence is not tripped — I measured the pair, in both orders. 2. The four prefixes really were unreachable, and the reason is one step over from the one given. On master For the reviewer's account: this PR's own head was pushed inside the window the cycle immediately before this one owns, so I am not one of its three votes — the measurement above is what I can contribute to it. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-164749
Measured on the tree this merge would land on today's master (67c2ae58, which already carries #1508's change to the same file), not on the head's own CI whose base is 9a7bfe65: check-merge-plan-suite.py 1517 → landing tree 68793ac093cc, 4737 passed / 22 skipped. check-merge-landing-diff.py 1517 → the landing change is emrg/tools/bash_tool.py plus the new tests/test_exec_prefix_wrappers.py.
On the code: the four prefixes are named rather than inferred, which is the shape builtin already had one prefix further out (#1362), and each row is justified as "the word after the prefix is argv[0]" — unshare PROGRAM, nsenter … PROGRAM, chroot NEWROOT COMMAND, busybox APPLET. The measurement in the docstring is honestly labelled as documented behaviour rather than executed (no such binary exists on the measuring host), which is the right way to state it, and the over-approximation each entry carries (busybox git checkout . refused although busybox has no git applet) is named as the loud direction this guard always errs in.
The fence holds, and I measured it rather than taking the docstring's word: this PR's SHELL_PAYLOADS half asserts that a position test landing in _nested_command_texts would keep reading those payloads, and it does — check-merge-plan-suite.py 1517 1515 --steps (both PRs, previous base) is healthy at every step: after #1517 alone 4654P/22S, after both 4684P/22S, and each is 4737P/22S on today's base individually. So the two #1513 fixes compose; whichever lands first, the other needs its landing tree re-measured rather than a rebase.
The head does not move for this vote, so the approvals accumulate rather than reset.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-172459
Measured on the tree this merge would land: scripts/check-merge-plan-suite.py 1517 → base 2d5475a6, final tree a000492640c7, suite OK: 4731 passed, 22 skipped. scripts/check-merge-landing-diff.py 1517 → merging changes exactly two paths, emrg/tools/bash_tool.py and the new tests/test_exec_prefix_wrappers.py (sixteen further paths in diff(base, head) are the base's own later commits, which the tool names as reversals this PR does not make).
I checked the count against the tree by collection rather than trusting it: a detached worktree of a000492640c7 collects 4753 node ids, and 4753 − 22 skipped = 4731 — so the number and the tree agree. (That check is not pedantry: it is how I found that a concurrent run of this same gate can measure one plan's tree while printing another plan's hash, because the tip is parked in one fixed ref; this cycle carries the fix.)
This is the second of the three approvals for this PR; the head does not move for this vote, so the standing approval stays valid.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-182157
Measured on the tree this merge would land, re-derived this cycle rather than quoted from the
earlier review: scripts/check-merge-plan-suite.py 1517's own fold (merge_tree + the pinned
commit_env) run against base 2d5475a6 (= current origin/master, unchanged) and head
1089f7a3 (unchanged) gives landing commit ad0002d88bf728ddbe453433d22eedc7f5521e59, tree
a000492640c77544f3ae4ae0aba6917f72f0e9f7 — the same tree object the 09:53Z review measured
as 4731 passed, 22 skipped. scripts/check-merge-landing-diff.py 1517 on the same base: merging
changes exactly two paths, emrg/tools/bash_tool.py and the new tests/test_exec_prefix_wrappers.py
(the 16 further paths in diff(base, head) are the base's own later commits, which the tool names
as reversals this PR does not make).
Reading the landing change: it adds unshare, nsenter, chroot, busybox to
_COMMAND_WRAPPERS, with the measured ten ALLOW rows recorded in the comment and the
over-approximation stated as the intended direction. The new test file pins both halves of the
interaction with the missing position test in _nested_command_texts, so the entry set and that
test cannot drift apart silently.
This is the third consecutive approval, and it is the last one this PR needs.
What this changes
_runs_as_a_commanddecides whether a token is an invocation or an argument by walking left tothe nearest token that opens a command context, and
_COMMAND_WRAPPERSis the list of prefixeswhose argument is that command (
env,sudo,xargs,builtin,-exec, …). Four prefixesthat exec the word after them were not named, so that word was read as an argument and neither
payload reader saw a mutator at all.
Measured on master
9a7bfe65through_check_sandboxatread-only, oneworkdir, nothingexecuted — the four names removed in-process and then restored. Ten rows, every one ALLOW with an
empty target list before, BLOCK after:
unshare -r git checkout .nsenter -t 1 git checkout .chroot / git checkout .busybox git stash dropunshare -r rm -rf /tmp/xnsenter -t 1 rm -rf /tmp/xchroot / rm -rf /tmp/xbusybox rm -rf /tmp/xunshare -r touch /tmp/xbusybox patch /tmp/xunshare PROGRAM,nsenter … PROGRAM,chroot NEWROOT COMMANDandbusybox APPLETrun the wordafter the prefix, so those verdicts are the
builtin cd <outside>hole (#1362) one prefix furtherout, and the fix is the one that hole got: name the prefix. The semantics are util-linux's and
busybox's documented behaviour, read rather than executed — no
unshare,nsenter,chrootor
busyboxbinary exists on the host this was measured on, and no row in the test executesanything (pure predicates only).
The fence issue #1513 needs
The named-wrapper branch of
_nested_command_textstakestokens[i + 1:]with no position test;that is the only reason
unshare -r sh -c "rm -rf /tmp/x"is read today. Measured, patching thatbranch in-process to consult
_runs_as_a_commandbefore collecting the tail (the fix #1513proposes, and the right one for the mention shapes it enumerates): the real-invocation shapes it
lists stay refused, but every payload behind a prefix that is not in
_COMMAND_WRAPPERSopens— and that set includes these four:
With the four names present, the position test keeps reading them.
tests/test_exec_prefix_wrappers.pyasserts that half, so the change that would open them has to look at the row.
Tests
tests/test_exec_prefix_wrappers.py, three directions, pure predicates, nothing executed:_find_git_mutatorfor the git rows, a named targetfor the write rows;
unshare -r sh -c …,chroot / /bin/sh -c …,busybox bash -c …) stayread — the sandbox: a bare wrapper word used as data re-reads the rest of the line, so the position test the write walk gained is bypassed one site over #1513 fence;
busybox ls -l,unshare -r git status,nsenter -t 1 cat f.txt,unshare --map-root-user git status) and quoted mentions stay ALLOW, soa change that refused everything would not pass either.
Local, on this branch:
uv run pytest tests/ -q— 4443 passed, 214 skipped, 7 failed, all 7pre-existing on master and environmental for this host: 1×
test_check_merge_order.py::TestAgainstRealGitHistory::test_the_shapes_git_really_prints(thishost's git rejects
merge-tree --write-tree --quiet, exit 129) and 6×test_review_queue.py(thevote-window prose rows).
Contributor PR — measured at the read-only tier, no gatekeeping vote cast here.