Skip to content

Return an error from a failed BEP 52 leaf read instead of raising - #22

Merged
daniboybye merged 1 commit into
fix/tracker-dns-and-rate-readoutfrom
fix/bep52-leaf-read-error-contract
Sep 12, 2026
Merged

daniboybye merged 1 commit into
fix/tracker-dns-and-rate-readoutfrom
fix/bep52-leaf-read-error-contract

Conversation

@daniboybye

Copy link
Copy Markdown
Owner

Stacked on #21 — the base is fix/tracker-dns-and-rate-readout, so the diff here is one commit. Merge #21 first and this retargets to master cleanly.

Picked up from the known-bugs list as one of the smallest open items.

The defect

Merkle.leaf_range_response_from_disk/7 is specced {:ok, [hash()]} | {:error, term()} and already handles a failed :file.open/2 that way. The per-leaf reads underneath it did not:

{:ok, block} = :file.pread(fd, offset, size)

An I/O error — or a file truncated between the stat that produced file_length and the read — raised MatchError out of the middle of a function whose contract says it returns errors.

Why it was low severity, and why it was still wrong

HashServe catches the raise and answers hash_reject, which is the correct thing to put on the wire for a hash request we cannot serve (BEP 52). So the shipped path behaved acceptably. It was wrong in two ways regardless: any other caller got an exception for an ordinary disk condition, and the reject was reached by accident rather than by decision — a rescue standing in for a return value.

The subtlety

:file.pread/3 answers a bare :eof, not an error tuple, when there is nothing at the offset. That is not the same as the legitimate case of a leaf beyond the end of the file, which BEP 52's power-of-two leaf padding makes routine and which hashes to the zero hash without reading at all.

The padded case is now a guard clause on offset >= file_length, so a bare :eof coming back from an actual read means file_length no longer describes the file, and is reported as a failure rather than silently hashing a short read. read_leaf_cache/4 short-circuits on the first unreadable leaf, since the cache is only useful complete — every consumer does Map.fetch!/2 on it.

Tests

+2, both in torrent_merkle_test.exs:

  • A file shorter than its declared file_length returns {:error, :eof} rather than raising. This is the truncated-under-us scenario, constructed by writing one block and declaring four.
  • A whole-file read whose final block is short still succeeds and verifies against the root, so the ragged tail that the new guard clause touches stays covered.

Verification

All 12 gates of .github/workflows/build-and-publish.yml job build run locally, in CI's order:

Gate Result
mix test 1772 passed (12 properties, 1760 tests)
coverage 91.2%
mix dialyzer 0 errors
mix credo --all clean
mix format --check-formatted clean
mix compile --warnings-as-errors (dev + test) clean
mix sobelow, mix hex.audit clean
mix deps.get --check-locked (dev + test), mix deps.unlock --check-unused clean
scripts/compile-property-deps.sh ok

The changelog entry was added to the unreleased 0.6.7 section rather than starting a new one, since #21 has not been merged or tagged yet.

Not reproducible locally, so not claimed green: the Trivy filesystem scan, the Codecov upload, and the SARIF upload to code scanning.

`Merkle.leaf_range_response_from_disk/7` documents `{:ok, [hash()]} | {:error,
term()}` and already handles a failed `:file.open/2` that way, but the per-leaf
reads underneath it pattern-matched `{:ok, block} = :file.pread(fd, offset,
size)`. An I/O error, or a file truncated between the stat that produced
`file_length` and the read, therefore raised `MatchError` out of the middle of a
function whose contract says it returns errors.

`HashServe` catches the raise and answers `hash_reject`, which is the correct
thing to put on the wire for a hash request we cannot serve (BEP 52) — so this was
low severity in the shipped path, and wrong everywhere else: any other caller got
an exception for an ordinary disk condition, and the reject was reached by
accident rather than by decision.

`read_leaf_cache/4` now short-circuits on the first unreadable leaf, since the
cache is only useful complete — every consumer does `Map.fetch!/2` on it.

One subtlety worth naming: `:file.pread/3` answers a bare `:eof`, not an error
tuple, when there is nothing at the offset. That is *not* the same as the
legitimate case of a leaf beyond the end of the file, which BEP 52's power-of-two
leaf padding makes routine and which hashes to the zero hash without reading at
all. The padded case is now a guard clause on `offset >= file_length`, so a bare
`:eof` from an actual read means `file_length` no longer describes the file and is
reported as a failure.

+2 tests: the truncated-file contract, and a whole-file read whose last block is
short, so the ragged tail the new guard touches stays covered.

Co-authored-by: Cursor <cursoragent@cursor.com>
@daniboybye

Copy link
Copy Markdown
Owner Author

CI note: build-and-publish.yml is triggered by pull_request: branches: ["master"], so this PR gets no checks while its base is fix/tracker-dns-and-rate-readout. Dispatched the same workflow manually against this branch instead: https://github.com/daniboybye/ElixirTorrent/actions/runs/34700625050 — and it will pick up the normal PR trigger once #21 merges and this retargets to master.

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
lib/elixir_torrent/torrent/merkle.ex 80.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

@daniboybye
daniboybye merged commit d1e26f3 into fix/tracker-dns-and-rate-readout Sep 12, 2026
4 checks passed
@daniboybye
daniboybye deleted the fix/bep52-leaf-read-error-contract branch September 12, 2026 18:29
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