Skip to content

Fuzz the edges: 24 bugs found and fixed, including a flatten that could certify a file it had overwritten - #4

Open
owenpkent wants to merge 5 commits into
retry-resumefrom
fuzz-edge-cases
Open

Fuzz the edges: 24 bugs found and fixed, including a flatten that could certify a file it had overwritten#4
owenpkent wants to merge 5 commits into
retry-resumefrom
fuzz-edge-cases

Conversation

@owenpkent

Copy link
Copy Markdown
Owner

Stacked on #3 (retry-resume), so the diff here is only the four commits
below. Retarget to main once #3 merges.

What this changes

Two new property-test modules, then fixes for the 24 bugs they found.

tests/test_fuzz.py already fuzzed what a filename can contain. These go
after two layers it never reached:

  • tests/test_fuzz_edges.py — what a card can contain. A BRAW clip whose
    atom headers lie, a tree that maps two sources onto one destination, a
    directory junction pointing at its own parent, a retry policy someone
    hand-edited into a preset.
  • tests/test_edge_cases.py — the layers between the bytes and the
    paperwork. What ffprobe hands back, what a number formats to, what a digest
    compares equal to, what a name renders to.

Each bug was committed first as a xfail(strict=True) naming the offending
line, then fixed. Both suites now run clean, so every test that was pinning a
reproduction is pinning a regression.

Two of these were worth the whole exercise.

Flattening could destroy a file and certify it. With --flat (or the app's
"Recreate the source folder structure" unticked), every source mapped to
destination/name and nothing checked two sources for one target. Two clips of
the same name in different card folders left one file on disk and both rows
reading Verified
— each was verified as it landed, before the next
overwrote it, so full verification passed them too. That is the worst shape a
bug can take here: the report attests to a file that is no longer there.

source: 2 files / 3000B      dest: 1 file / 2000B
non-VERIFIED: none           job.errors: none

The collision is now caught against the scan, before a byte moves, and the job
refuses naming both paths.

One malformed clip aborted the whole offload. engine.run called probe()
unguarded and probe() did not guard its own parsers. The realistic trigger
turned out not to be a crafted file at all: it is ffprobe writing "N/A" in a
numeric field, which it does routinely for streams it cannot fully describe.
The clips after the bad one were never copied and no report was written.

The remaining 22, grouped by what they had in common:

Family Fix
Trusting a length that came out of the file _read_timing, the ilst loop, _descend's recursion and _find_moov's read are all bounded by what is actually present
Text that cannot survive the trip The XML 1.0 filter reports/mhl.py always had now lives in util and covers the CSV and ASC MHL writers
Comparing unlike with unlike Digests compare per algorithm: case-folded for hex, exact for C4
Trusting a file we wrote A damaged history.json no longer raises out of History()
Arithmetic that ignored its own edges format_size, _parse_rate, naming.build

What you verified

  • The collision fix refuses before writing, not after. The check runs on the
    scan output; the destination directory does not exist when it raises.
    Confirmed end to end, not just in the unit test.
  • The collision fix does not cost the ordinary case. A separate test
    flattens two differently-named files and asserts both land.
  • --flat was reachable three ways before this mattered: the CLI flag,
    Simple mode's checkbox, and the preset editor's. All three go through
    engine.run, so the guard covers them.
  • The malformed-clip fix does not weaken a verdict. Metadata is read after
    the bytes are copied and verified, so containing it cannot make Verified
    easier to reach. The file still copies, still verifies, and the job carries a
    warning.
  • Every fix has a test that fails against the old code. That is how they
    were written: the reproduction was committed as a strict xfail first
    (64c37f0, ed6cdc0), and 821b39b turns all 49 green.
  • The BRAW fuzzer finds the crash independently. The random-atom-tree sweep
    passes and is kept as a control, because a random tree almost never grows the
    vide handler _read_timing requires. A second sweep pins that shape and
    fuzzes only the two bodies read at fixed offsets; it rediscovers the crash and
    minimises it to two variants.
  • Two reported findings were not bugs, checked rather than assumed:
    argparse's choices= rejects an unknown --verify/--profile before the
    enum constructor runs, so there is no traceback. Both are kept as passing
    controls.
  • Soak at 3000 examples per property finds nothing beyond what is pinned
    (482 pass, ~3 min).

Behaviour changes worth a second opinion

Three judgement calls, flagged because they are reversible and you may want
them reversed:

  1. --flat refuses rather than auto-renaming. Renaming preserves the
    intent but silently changes filenames an NLE may be relinking against.
    Refusing matches the existing assert_safe_destinations pre-flight. Cost:
    --flat now fails on cards it used to "work" on.
  2. RetryPolicy clamps instead of raising. presets.json is hand-editable
    and a nonsensical delay should not raise mid-transfer.
  3. History fingerprints are seeded with the source name, so entries recorded
    before this change will not match. Duplicate-card detection degrades to not
    warning, which is the safe direction, and re-records correctly on the next
    offload. Chosen over special-casing empty sources because the same collision
    class exists for non-empty ones.

Docs

docs/data-safety.md carries the weight: five new rows in the guarded table, a
section on why flattening is refused, and one honest addition to what is still
not protected
— the collision check compares through os.path.normcase, so
copying on Linux or macOS from a case-sensitive source onto a case-insensitive
destination is the one arrangement it does not catch.

Also: docs/braw.md on reading a file that is lying to you, docs/ascmhl.md on
the round-trip bug and per-algorithm comparison, docs/performance.md on
junctions, and corrected test counts in README (it claimed 413 at 82%;
it is 482 at 83%), CONTRIBUTING and the PR template.

CI's soak job only ever ran test_fuzz.py, so the two new modules would not
have been soaked at all. It now runs all three.

Checklist

  • pytest passes (482 tests)
  • ruff check src tests passes
  • Tests added for the behaviour changed

Touches the copy and verification path:

  • There is a test that fails against the old code — 49 of them, each
    committed as a strict xfail before the fix
  • This does not make a "Verified" verdict easier to reach. It makes one
    harder: a layout that would lose a file is refused, and metadata failures
    no longer pass silently
  • docs/data-safety.md still describes reality,
    including its "what is still not protected" list

Changes documented behaviour:

  • The relevant docs/*.md is updated
  • The test count in README.md is updated

test_fuzz.py fuzzes what a filename can contain. This fuzzes what a card
can contain: a BRAW clip whose atom headers lie, a tree that maps two
sources onto one destination, a junction pointing at its own parent, a
retry policy someone hand-edited into a preset.

Each xfail(strict=True) is a reproduced bug naming its own offending
line, so fixing one turns the test XPASS and pytest asks for the marker
back. The unmarked tests are controls that pin behaviour which already
holds, several of which are the sibling of a broken path: write_mhl
survives names that crash write_csv, and preserving the tree keeps
destinations injective where --flat does not.

The worst of it, in order:

  - --flat maps every source onto dest_root/name with no collision
    check, so two takes named the same in different card folders leave
    one file on disk and two VERIFIED rows in the report. FULL
    verification passes both: each is verified before the next clobbers
    it.
  - engine.run calls probe() with no try/except and probe()'s BRAW
    branch has none either, so one malformed clip aborts the whole job.
    The clips after it are never copied and no report is written.
  - braw._read_timing indexes and unpacks at offsets taken from an
    atom's declared size without checking the buffer reaches that far.
  - braw._descend recurses per nested container with no depth limit, so
    a 16 KB file exhausts the stack.
  - ascmhl.py never grew the _xml_safe filter reports/mhl.py has, and
    read_manifest_hashes swallows the resulting ParseError, so a file
    can leave the chain of custody with nothing to show for it.

The random-atom-tree sweep passes and is kept as a control: a random
tree almost never grows the vide handler _read_timing requires, so the
truncated-trak sweep pins that shape and fuzzes only the two bodies
read at fixed offsets. It rediscovers the crash and minimises it to
two variants.
test_fuzz_edges.py goes after the copy engine and the binary parsers.
This goes after everything downstream: what ffprobe hands back, what a
number formats to, what a digest compares equal to, what a name renders
to.

Each failure is a value that is legal where it is produced and fatal, or
silently wrong, where it is consumed, because the two ends disagree
about the contract:

  - ffprobe writes "N/A" in a numeric field and _build calls int() on
    it. This is the realistic version of "one bad file kills the job"
    that the BRAW tests found the hard way: no crafted file needed, just
    a clip whose audio stream ffprobe cannot fully describe. _build sits
    outside the try that catches SubprocessError and JSONDecodeError, so
    the ValueError escapes probe() and then engine.run().
  - _parse_rate accepts "inf/1" because float() does, and every fps
    formatter then calls round() on it, which does not. Both ends are
    tested here so whichever one moves, the other is pinned.
  - Another tool writes its hex digests uppercase and verify.py compares
    with ==, so every byte-identical file reports MISMATCH. The fix has
    to be per-algorithm rather than a blanket .lower(), because C4
    digests are base58 and genuinely case-sensitive.
  - render substitutes token values into the string it is still
    substituting into, so a card folder literally named {index} has its
    name overwritten by the sequence number.
  - format_size tests `< 1000` before rounding and before considering
    sign, so it prints a 1000.0 mantissa at every decade boundary and
    never promotes a negative size out of bytes.
  - fingerprint hashes the joined file lines and nothing else, so every
    empty source shares da39a3ee, the SHA-1 of nothing.

Two of the CLI findings turned out to be non-bugs on inspection and are
kept as passing controls rather than dropped: argparse's choices= catches
an unknown --verify or --profile before the enum constructor runs, so
there is no traceback, and _parse_rate is total over rationals and junk.
Knowing which neighbouring paths are already safe is what makes the
xfails above worth trusting.
The two suites go from 49 strict xfails to none: every test that was
pinning a reproduction is now pinning a regression. 482 pass, and the
3000-example soak passes too.

Two of these were worth the whole exercise.

Flattening could destroy a file and certify it. Every source mapped to
destination/name with nothing checking two sources for one target, so
two clips of the same name in different card folders left one file on
disk and both rows reading VERIFIED. Each was verified as it landed,
before the next overwrote it, so full verification passed them as well.
The collision is now caught against the scan, before a byte moves, and
the job refuses naming both paths. Comparison goes through
os.path.normcase, which also catches the case-only collision I could
not build a portable test for: two names distinct on the case-sensitive
volume they came from, one file on the volume they are going to.

One malformed clip aborted the whole offload. engine.run called probe()
unguarded and probe() did not guard its own parsers. The realistic
trigger was not a crafted file at all, it was ffprobe writing "N/A" in
a numeric field, which it does routinely. Metadata is now contained at
both call sites: the bytes are copied and verified before any of it
runs, so an unreadable container costs its own metadata and a line in
the report rather than the rest of the card.

The rest, grouped by what they had in common:

  - Trusting a length that came out of the file. braw's _read_timing,
    its ilst loop, _descend's recursion and _find_moov's read are all
    now bounded by what is actually present.
  - Text that cannot survive the trip. The XML 1.0 filter that
    reports/mhl.py always had now lives in util and covers the CSV and
    ASC MHL writers too. ASC MHL filters where the filename first
    enters, so the structure hashes and the <path> text they describe
    are computed from the same string.
  - Comparing unlike with unlike. Digests now compare per algorithm:
    case-folded for hex, exact for C4, whose base58 alphabet uses case
    to carry information.
  - Trusting a file we wrote. A damaged history.json no longer raises
    out of History(), and read_json treats invalid UTF-8 as unreadable
    rather than letting UnicodeDecodeError through, since that is a
    ValueError and not an OSError.
  - Arithmetic that ignored its own edges. format_size rounds before
    picking a unit and handles sign; _parse_rate rejects a rate that is
    not finite and positive; naming.build's searches are bounded by the
    number of taken names instead of a fixed 999.

Two behaviour changes worth knowing about. RetryPolicy now clamps rather
than rejects, because presets.json is hand-editable and a nonsensical
delay should not raise mid-transfer. And history fingerprints are seeded
with the source's name, so entries recorded before this change will not
match: duplicate-card detection degrades to not warning, which is the
safe direction, and re-records correctly on the next offload.
Behaviour that changed, documented where someone would look for it
rather than only in the changelog.

docs/data-safety.md carries the weight, since that is the file the PR
template points at for anything touching the copy path. Five new rows in
the guarded table, a section on why flattening is now refused rather
than allowed to lose a file, and one honest addition to "what is still
not protected": the collision check compares through os.path.normcase,
so copying on Linux or macOS from a case-sensitive source onto a
case-insensitive destination is the one arrangement it does not catch.
Detecting that means asking the destination filesystem what it considers
distinct, which is not implemented.

docs/braw.md gains a section on reading a file that is lying to you: a
declared size is a claim and the buffer is the fact. docs/ascmhl.md
records the round-trip bug alongside the ones the reference diff caught,
and explains why digests compare per algorithm rather than as strings.
docs/performance.md no longer implies junctions are simply unhandled;
they are still not reproduced at the destination, but one pointing at
its own parent can no longer send the scan round in circles.

README gets the corrected counts (482 tests, 83% coverage; it claimed
413 at 82%), the three property modules described by what each one
fuzzes, and the --flat entry noting the refusal. CONTRIBUTING and the PR
template had their own stale counts.

CI's soak job only ever ran test_fuzz.py, so the two new property
modules would not have been soaked at all. It now runs all three, which
is 93 tests in about two and a half minutes.
The first sweep updated docs/data-safety.md to say the duplicate-card
fingerprint covers the source name plus the listing, and left README
saying the listing alone. Two files, two answers, same function.

Three gaps alongside it. The fingerprint change makes pre-upgrade
entries stop matching, which belonged in the honest-limits list rather
than only in the changelog. RetryPolicy's clamping is now noted in the
paragraph that says 'or per preset', since a hand-edited preset is
exactly how a negative delay reaches time.sleep. And CONTRIBUTING's
'testing without a camera card' table now names the new modules, which
cover four of its own categories: a lying BRAW header, a junction cycle,
a destination collision, and ffprobe returning N/A.
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.

1 participant