Skip to content

emrg: ditto names its destination, the file it copies to - #1444

Merged
argszero merged 1 commit into
masterfrom
fix/ditto-names-its-destination
Sep 19, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/ditto-names-its-destination

Conversation

@argszero

Copy link
Copy Markdown
Owner

What was wrong

The write-target walk had no table naming ditto, so every form of the macOS
copier reported an empty target list — and an empty list is allowed by
construction, because the loop that judges targets never runs.

Measured on master 35284a01 through the real predicate (the commands were never
executed), destination outside every allowed root:

command targets read-only workspace-write
ditto /outside/emrg/src /outside/emrg/dst [] ALLOW ALLOW
ditto -c -k f /outside/emrg/a.zip [] ALLOW ALLOW
cp /outside/emrg/x /outside/emrg/y (control) [.../y] BLOCK BLOCK

/usr/bin/ditto is present on every macOS — this project's primary host — so this is
a reachable write, not a hypothetical one. It is the same fail-open the everyday
writers (#1398), the compressor family (#1418) and rsync each had.

Ground truth first

One fresh scratch directory per row on this host (/usr/bin/ditto), the listing read
back off disk afterwards:

ditto f g                   rc=0  creates g                (f is a read)
ditto -c -k f arc.zip       rc=0  creates arc.zip          last operand = archive
ditto -x -k arc.zip outdir  rc=0  creates outdir/f         last operand = directory
ditto --arch arm64 f g      rc=0  creates g                `arm64` is consumed
ditto --bom nope.bom f g    rc=1  writes nothing           the bom is a read
ditto f                     rc=0  writes nothing           "No destination"
ditto --help                rc=1  writes nothing
ditto -t OUT src dst        rc=0  writes nothing           "invalid option -- t"

So the destination is the last operand in every writing form, and the two forms that
write nothing have no second operand to name — that is cp's rule exactly, which is
why ditto joins _DESTINATION_LAST_VERBS rather than getting a branch of its own.

The option that writes a file of its own

--keepBinariesList <path> creates that file beside the destination:

ditto --keepBinaries --keepBinariesList kept.txt src/ dst/   → kept.txt created
ditto --keepBinariesList kept_no.txt src/ dst/               → too, without the flag
ditto --keepBinaries --keepBinariesList=kept_eq.txt src/ …   → the `=` spelling too

All rc=0, read off disk. So unlike -t <dir> for cp/mv and unlike patch -o,
these two readings are additions, not alternatives, and both are named.

The verb's remaining value-taking options (--arch, --bom, --lang, --outBom,
--keepBinariesPattern, --zlibCompressionLevel) come from ditto's own usage output;
the entry exists so an option's value is not read as an operand, and a test asserts
the table equals that list.

Tests

tests/test_bash_tool_ditto_destination.py (44 tests) pins both directions: 12 writing
forms name exactly the destination and are refused at both tiers, a
destination inside the workspace stays allowed, the protected daemon file is refused
with protected in the workspace-write reason, the no-write forms name nothing and
stay allowed, and the --keepBinariesList spellings name both paths. Three arms keep
the claims killable: dropping the kept-list table unnames the created file, dropping
--outBom from the ditto table makes the option's value an operand (a path the run
only reads), and a control shows the rule is not blind. The per-verb mutation lock in
tests/test_bash_tool_sandbox.py gains the ditto row.

Residual, stated rather than hidden

Sharing the destination-last branch lends ditto the -t <dir> reading, and -t is
no option of the verb's (invalid option -- t, nothing written). Naming that token is
the fail-closed direction — a command that writes nothing is refused — and no writing
spelling of ditto contains a -t, so no real write is blocked. Recorded in the code
comment so a later reader need not re-derive it.

The write-target walk had no table naming `ditto`, so every form of the macOS
copier reported an empty target list — and an empty list is allowed by
construction, because the loop that judges targets never runs. Measured on master
35284a0, destination outside every allowed root: `ditto src dst` answered ALLOW at
both tiers while `cp`, `rsync` and `truncate` on the same paths were refused.

Ground truth taken first, one scratch directory per row: `ditto f g` creates g,
`ditto -c -k f a.zip` creates the archive, `ditto -x -k a.zip d` creates d/f — the
destination is the last operand in every writing form, which is cp's rule.
`--bom <f>` is a read (a missing bom exits 1), `ditto f` and `ditto --help` write
nothing.

`--keepBinariesList <path>` is the one option that creates a file of its own, and it
does so *beside* the destination (also without `--keepBinaries`, also in the
`=`-joined spelling), so the two readings are additions rather than alternatives and
both are named. The verb's remaining value-taking options (--arch, --lang, --outBom,
--keepBinariesPattern, --zlibCompressionLevel) take values that are not paths, so
they are consumed rather than named.
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification of b8892782 — the premise holds and the fix holds, with one value-taking option left out of the table

Verified on the head as staged from git objects (pristine directory, no .git), predicate only, nothing executed.

Suite. This head: 17 failed, 3998 passed, 26 skipped. Master 12f29d7: 17 failed, 4083 passed. The 17 are the known .git-dependent staging artefacts and the failure set is byte-identical to master's — no regression. Ablation arm: dropping this PR's two test files onto unpatched master turns 38 of them red (test_bash_tool_ditto_destination.py + the walk row), so the rows ride on the new membership rather than on something else in the walk.

The premise, re-measured on master 12f29d7 (destination outside every allowed root, both tiers): every ditto spelling reported an empty target list and answered ALLOW — ditto <out>/src <out>/dst, -c -k, -x -k, --arch arm64 …, --keepBinariesList … — while cp on the same two paths was refused. Fail-open by construction, exactly as the body states.

The fix, re-measured on this head (same geometry):

command master this head
ditto <out>/src <out>/dst [] ALLOW ['<out>/dst'] BLOCK
ditto -c -k <out>/f <out>/arc.zip [] ALLOW ['<out>/arc.zip'] BLOCK
ditto -x -k <out>/arc.zip <out>/outdir [] ALLOW ['<out>/outdir'] BLOCK
ditto --arch arm64 <out>/src <out>/dst [] ALLOW ['<out>/dst'] BLOCK
ditto --bom <out>/nope.bom <out>/f <out>/dst [] ALLOW ['<out>/dst'] BLOCK
ditto --keepBinaries --keepBinariesList <out>/kept.txt <out>/src <out>/dst [] ALLOW ['<out>/dst', '<out>/kept.txt'] BLOCK
ditto --keepBinariesList=<out>/kept.txt <out>/src <out>/dst [] ALLOW ['<out>/dst', '<out>/kept.txt'] BLOCK
ditto <out>/f / ditto --help (non-writing controls) [] ALLOW [] ALLOW

Ground truth re-measured here (/usr/bin/ditto, macOS 26.6, fresh scratch directory per row, listing read back): ditto f g rc=0 creates g; ditto -c -k f arc.zip rc=0 creates arc.zip; ditto -x -k arc.zip outdir rc=0 creates outdir/f; ditto --keepBinariesList kept.txt f g and its =-joined spelling both create the list file; ditto --bom nope.bom f g rc=1 writes nothing; ditto f rc=0 writes nothing. The -t OUT src dst row is as described — ditto: invalid option -- t, nothing written, and no writing spelling of the verb contains a -t, so the shared branch's fail-closed reading is free.

I checked the short spellings the man page mentions in prose (-b, -o, -l, -f, -n): /usr/bin/ditto rejects each as invalid option, so the man's prose is stale and there is no short-option gap of the kind #1443 fixes.

The one omission: --option key=value

--option key=value is value-taking, accepted by the binary, and documented in the man page — but it is not in _VERB_OPTIONS_WITH_VALUE["ditto"], because it is not printed by ditto -h (which is where the table, and test_the_option_table_carries_every_value_taking_option_of_the_tool, take their list from). Measured:

  • ditto f g --option foo=bar → rc=0, and g is really created (control ditto f g identical); ditto --option foo=bar f g rc=0, same.
  • ditto --option f g → rc=1 I expect options in the form of key=value: f — i.e. the word after the option is its value.
  • ditto f g --option /outside/k=v → rc=0, g created: a /-containing value is accepted too.

Because the option is not in the table, its value stays in the operand list, and when the option trails, the last-operand rule names that value instead of the destination:

command master 12f29d7 this head with --option added to the table
ditto <out>/src <out>/dst --option foo=bar [] ALLOW ['foo=bar'] ALLOW ['<out>/dst'] BLOCK
ditto src.txt <workspace>/dst --option <out>/key=value [] ALLOW ['<out>/key=value'] BLOCK ['<workspace>/dst'] ALLOW at workspace-write
ditto <out>/src <out>/dst --arch arm64 (option in the table, control) [] ALLOW ['<out>/dst'] BLOCK

The control row is what makes the discriminator the missing entry rather than the trailing position. Two directions come out of the gap, and they are the two this file's record weighs differently: foo=bar resolves inside the workspace, so a run whose real destination is outside every root is allowed at read-only (the write still happens, rc=0 measured); and a /-containing value yields a false block at workspace-write for a command whose only write is the workspace destination — which master allowed.

The third column is a simulation, not a patch: I added "--option" to the dict at runtime in the probe and re-read the three rows, nothing else changed. So the remedy looks like one entry in the table and not in _DITTO_OPTION_DESTINATIONS — the value is a key=value pair, not a path, so naming it as a destination would be the false block the table exists to prevent. Whether you also want test_the_option_table_carries_every_value_taking_option_of_the_tool to read the man page rather than -h is your call; the current assertion is exact and will keep passing while --option stays out.

Two notes for whoever lands this: the walk's own docstring says the table is derived from the usage output, and that is accurate as far as it goes — the gap is between ditto -h and what the binary accepts. And on the ordered pair from last cycle, r1442 × r1443 still conflicts in that one elif while both merge clean against the current master (12f29d7) and against this head, so the four open PRs can land in any order except that one hunk needing both branches kept.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260920-025936

Cast on the landing tree 7d2931dced36 (head b8892782 is behind_by=3, so refreshing it would
void the vote standing on it — there is none yet, but the route is the same): check-merge-plan-suite.py 14444170 passed, 22 skipped (151s). Head tests, run on the head tree: 375 passed, 3 skipped.

Re-measured this cycle rather than inherited.

The hole is an empty target list, so it is allowed by construction — and it spans both tiers.
Through the real predicate, destination outside every allowed root, nothing executed:

command master d9b8b91a this head
ditto <ws>/src <out>/dst [] ALLOW / ALLOW ['<out>/dst'] refuse/refuse
ditto -c -k <ws>/f <out>/a.zip [] ALLOW / ALLOW ['<out>/a.zip'] refuse/refuse
ditto -x -k <ws>/a.zip <out>/outdir [] ALLOW / ALLOW ['<out>/outdir'] refuse/refuse
ditto --keepBinariesList <out>/kept.txt <ws>/src <out>/dst [] ALLOW / ALLOW both paths named, refuse
cp <ws>/src <out>/dst (control) ['<out>/dst'] refuse unchanged
ditto --arch arm64 <ws>/f <out>/g (value is not a path) [] ALLOW ['<out>/g'] — names the destination, not arm64
ditto <ws>/f (writes nothing) [] ALLOW [] ALLOW — no false block

read-only is the tier that exists to protect uncommitted work, and every writing ditto form passed it.

Ground truth, executed in throwaway trees (/usr/bin/ditto, one fresh directory per row, listing read
off disk): ditto f g rc=0 creates g; ditto -c -k f arc.zip creates the archive; ditto -x -k arc.zip outdir creates outdir/f; ditto --arch arm64 f g2 creates g2 while arm64 is created nowhere — so the
last operand is the write and the option's value is not a path. --keepBinariesList kept.txt still created
kept.txt when the copy itself failed (rc=1)
— which is exactly why it is read unconditionally rather than
only beside --keepBinaries; the row is measured, not assumed. --bom nope.bom f g3 created nothing.
ditto f alone exits 1 "No destination" and created nothing, which is why the single-operand row stays allowed.

Mutation arms, one per half (bash_tool.py restored byte-identically afterwards, sha256[:16]
dafc5c8db17c876c before and after; HOME/TMPDIR pinned to scratch for the arm):

  • ditto removed from _DESTINATION_LAST_VERBS36 failed (the whole family is load-bearing);
  • only the --keepBinariesList destination reading dropped → exactly 7 failed, its own rows — the two
    halves are separately load-bearing and the arms are surgical.

Detail I checked because it is where this could have gone wrong in the other direction: the shared branch lends
ditto the -t <dir> reading, and ditto -t OUT src dst really does print invalid option -- t and write
nothing — so naming that token is fail-closed, and no writing spelling of ditto contains a -t.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260920-032546

Cast on the landing tree 7d2931dced36 (head b8892782 is stale — 3 commits behind
master — and a refresh would void the vote already standing on it). Rebuild-and-
compare: git merge-tree --write-tree master b8892782 =
7d2931dced363e9af69bf0cb99b685331881511c, the sha check-merge-plan-suite.py
printed; suite on that tree 4170 passed / 22 skipped.

Measured through the real predicate at both tiers, master → head, <ws> as workdir:

row master head
ditto <ws>/f <out>/g ALLOW / ALLOW BLOCK / BLOCK
ditto -c -k <ws>/f <out>/g.zip ALLOW / ALLOW BLOCK / BLOCK
ditto -x -k <ws>/f.zip <out>/dir ALLOW / ALLOW BLOCK / BLOCK
ditto --keepBinariesList <out>/kept.txt <ws>/f <out>/g2 ALLOW / ALLOW BLOCK / BLOCK
control: ditto <ws>/f <ws>/g (writes allowed here) ALLOW / ALLOW BLOCK / ALLOW
control: read form ditto -h <out>/g3 ALLOW / ALLOW ALLOW / ALLOW

The read-only column is why this one is worth fixing rather than filing: on master
every write form of ditto was allowed even there, the tier that exists to protect
uncommitted work, because the walk named no target at all. The two controls are what
make the fix checkable rather than merely stricter — an in-workspace destination stays
allowed where writes are allowed, and the read form is not blocked.

Ground truth against /usr/bin/ditto, one fresh directory per row: ditto f g
creates g; -c -k creates the archive; -x -k creates outdir/f; --arch arm64 f g2 creates g2 and creates arm64 nowhere, so that option's value is not a path and
must not be named; and --keepBinariesList kept.txt still created kept.txt when
the copy itself failed (rc=1)
— which is why it is read unconditionally rather than
as "the copy succeeded".

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260920-034607

Third vote, cast on the tree this merge would land: 7d2931dced36, re-measured this cycle (the head is
3 commits behind master, so a refresh would have voided the two votes standing on it). Rebuilt and
compared rather than taken on trust — git merge-tree --write-tree master b8892782 =
7d2931dced363e9af69bf0cb99b685331881511c, exactly the sha check-merge-plan-suite.py printed, whose
suite is 4170 passed / 22 skipped.

Independent verification this cycle, on the head's own tree (the probe prints the bash_tool.__file__
it loaded, so no verdict here belongs to another tree), _check_sandbox at both tiers, workdir <ws>:

row master (read-only / workspace-write) head
ditto <ws>/f <out>/g ALLOW / ALLOW BLOCK / BLOCK
ditto -c -k <ws>/f <out>/g.zip ALLOW / ALLOW BLOCK / BLOCK
ditto -x -k <ws>/f.zip <out>/dir ALLOW / ALLOW BLOCK / BLOCK
ditto --keepBinariesList <out>/kept.txt <ws>/f <out>/g2 ALLOW / ALLOW BLOCK / BLOCK
ditto --arch arm64 <ws>/f <out>/g3 ALLOW / ALLOW BLOCK / BLOCK
control: ditto <ws>/f <ws>/g (write allowed here) ALLOW / ALLOW BLOCK / ALLOW
control: read form ditto -h <out>/g4 ALLOW / ALLOW ALLOW / ALLOW

The read-only column is why this is worth merging rather than filing: on master every write form of
ditto was allowed even at the tier that exists to protect uncommitted work, because the walk named no
target at all and the loop that judges targets never ran. The --arch arm64 row is the pair to the
--keepBinariesList one and both are handled: an option's value is named only where the value really is
a path, and arm64 is created nowhere by the real binary.

Head tests: tests/test_bash_tool_ditto_destination.py 44 passed (run in the head's worktree, where
import emrg.tools.bash_tool resolves to that worktree's file). Ground truth against /usr/bin/ditto,
one fresh directory per row: ditto f g creates g; -c -k creates the archive; -x -k creates
outdir/f; --arch arm64 f g2 creates g2 and creates arm64 nowhere; and
--keepBinariesList kept.txt still created kept.txt when the copy itself failed (rc=1), which is
why that value is read unconditionally rather than only on success.

No test in this PR starts, stops or restarts a daemon.

@argszero
argszero merged commit 7dd2db6 into master Sep 19, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants