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

CHANGELOG.md

@@ -51,6 +51,18 @@ speed of 0 B/s reported on a torrent moving at 100 KB/s.
   oscillates, and a torrent slower than one piece per window reads 0 until its
   first piece lands.
 
+- A disk error while serving a BEP 52 hash request returns an error instead of
+  raising. `Merkle.leaf_range_response_from_disk/7` documents
+  `{:error, term()}` and handles a failed `:file.open/2` that way, but the
+  per-leaf reads underneath it pattern-matched `{:ok, block} = :file.pread/3`, so
+  an I/O error — or a file truncated between the stat that produced `file_length`
+  and the read — raised `MatchError` from the middle of the function. `HashServe`
+  catches that and answers `hash_reject`, which is correct on the wire but
+  reached by the wrong path; any other caller got an exception for a disk
+  condition. `:file.pread/3` also answers a bare `:eof` rather than an error
+  tuple, which is now distinguished from the legitimate padded-leaf case that
+  reads no bytes at all.
+
 ### Changed
 
 - The HTTP stack behind tracker announces and BEP 19 web seeds moved up: hackney

lib/elixir_torrent/torrent/merkle.ex

@@ -679,9 +679,8 @@ defmodule Torrent.Merkle do
         ctx.proof_layers
       )
 
-    cache = read_leaf_cache(fd, ctx.file_length, ctx.block_count, indices)
-
-    with :ok <-
+    with {:ok, cache} <- read_leaf_cache(fd, ctx.file_length, ctx.block_count, indices),
+         :ok <-
            verify_piece_subtrees(
              cache,
              ctx.piece_hashes,
@@ -1274,34 +1273,48 @@ defmodule Torrent.Merkle do
     |> MapSet.filter(&(&1 < block_count))
   end
 
+  # Stops at the first unreadable leaf rather than hashing the rest, because the
+  # cache is only useful complete — every consumer below does `Map.fetch!/2`.
   defp read_leaf_cache(fd, file_length, block_count, indices) do
-    Enum.reduce(indices, %{}, fn leaf, acc ->
-      Map.put(acc, leaf, leaf_hash_from_fd(fd, file_length, block_count, leaf))
+    Enum.reduce_while(indices, {:ok, %{}}, fn leaf, {:ok, acc} ->
+      case leaf_hash_from_fd(fd, file_length, block_count, leaf) do
+        {:ok, hash} -> {:cont, {:ok, Map.put(acc, leaf, hash)}}
+        {:error, reason} -> {:halt, {:error, reason}}
+      end
     end)
   end
 
   defp leaf_hash_from_fd(fd, file_length, block_count, leaf_index) do
     offset = leaf_index * @block_size
 
-    data =
+    with {:ok, data} <- read_leaf_bytes(fd, offset, file_length) do
       cond do
-        offset >= file_length ->
-          <<>>
+        leaf_index >= block_count -> {:ok, @zero_hash}
+        byte_size(data) == 0 -> {:ok, @zero_hash}
+        true -> {:ok, :crypto.hash(:sha256, data)}
+      end
+    end
+  end
 
-        offset + @block_size > file_length ->
-          size = file_length - offset
-          {:ok, block} = :file.pread(fd, offset, size)
-          block
+  # A leaf past the end of the file is a legitimate request on a padded tree, not
+  # a read at all: BEP 52 pads the leaf layer to a power of two and those leaves
+  # hash to `@zero_hash`.
+  defp read_leaf_bytes(_fd, offset, file_length) when offset >= file_length, do: {:ok, <<>>}
 
-        true ->
-          {:ok, block} = :file.pread(fd, offset, @block_size)
-          block
-      end
+  defp read_leaf_bytes(fd, offset, file_length) do
+    case :file.pread(fd, offset, min(@block_size, file_length - offset)) do
+      {:ok, block} ->
+        {:ok, block}
 
-    cond do
-      leaf_index >= block_count -> @zero_hash
-      byte_size(data) == 0 -> @zero_hash
-      true -> :crypto.hash(:sha256, data)
+      # `pread` answers a bare `:eof` instead of an error tuple when there is
+      # nothing at the offset. Reaching it here means `file_length` no longer
+      # describes the file — truncated under us between the `File.stat` and this
+      # read — so it is a real failure and not the padded-leaf case above.
+      :eof ->
+        {:error, :eof}
+
+      {:error, reason} ->
+        {:error, reason}
     end
   end
 

test/torrent_merkle_test.exs

@@ -533,6 +533,78 @@ defmodule Torrent.MerkleTest do
       assert Merkle.leaf_range_response_from_disk(path, 1, [hash("x")], @block_size, 1, 2, 0) ==
                {:error, :invalid_index}
     end
+
+    test "returns an error tuple when the file is shorter than the declared length" do
+      # `file_length` comes from a stat taken before the read, so a file truncated
+      # or replaced in between leaves `:file.pread/3` reading past the real EOF.
+      # The function documents `{:error, term()}` and already handles a failed
+      # `:file.open/2` that way; a failed read used to raise `MatchError` out of
+      # the middle instead. `HashServe` catches that and answers `hash_reject`,
+      # which is the right thing on the wire (BEP 52) reached by the wrong path —
+      # and anything else calling this got an exception for a disk condition.
+      blocks = for byte <- [?a, ?b, ?c, ?d], do: :binary.copy(<<byte>>, @block_size)
+      content = IO.iodata_to_binary(blocks)
+      {:ok, tree} = Merkle.build(content)
+      piece_length = 2 * @block_size
+      {:ok, layer_bin} = Merkle.piece_layer(tree, piece_length)
+
+      piece_hashes =
+        for <<digest::binary-size(32) <- layer_bin>> do
+          digest
+        end
+
+      dir = Path.join(System.tmp_dir!(), "merkle_short_#{System.unique_integer([:positive])}")
+      File.mkdir_p!(dir)
+      path = Path.join(dir, "truncated.bin")
+      # Only the first block is actually on disk.
+      File.write!(path, :binary.part(content, 0, @block_size))
+      on_exit(fn -> File.rm_rf!(dir) end)
+
+      assert Merkle.leaf_range_response_from_disk(
+               path,
+               byte_size(content),
+               piece_hashes,
+               piece_length,
+               0,
+               4,
+               0
+             ) == {:error, :eof}
+    end
+
+    test "still serves a whole file whose last block is short" do
+      # The guard added alongside the error contract must not disturb the ordinary
+      # ragged-tail case: a final partial block is normal, not a truncated file.
+      full = for byte <- [?a, ?b, ?c, ?d, ?e, ?f, ?g], do: :binary.copy(<<byte>>, @block_size)
+      content = IO.iodata_to_binary(full) <> :binary.copy(<<?h>>, 100)
+      {:ok, tree} = Merkle.build(content)
+      root = Merkle.root(tree)
+      piece_length = 4 * @block_size
+      {:ok, layer_bin} = Merkle.piece_layer(tree, piece_length)
+
+      piece_hashes =
+        for <<digest::binary-size(32) <- layer_bin>> do
+          digest
+        end
+
+      dir = Path.join(System.tmp_dir!(), "merkle_ragged_#{System.unique_integer([:positive])}")
+      File.mkdir_p!(dir)
+      path = Path.join(dir, "ragged.bin")
+      File.write!(path, content)
+      on_exit(fn -> File.rm_rf!(dir) end)
+
+      assert {:ok, hashes} =
+               Merkle.leaf_range_response_from_disk(
+                 path,
+                 byte_size(content),
+                 piece_hashes,
+                 piece_length,
+                 0,
+                 2,
+                 2
+               )
+
+      assert Merkle.verify_hashes(root, 0, 0, 2, 2, hashes, 8)
+    end
   end
 
   describe "libtorrent flat proof helpers" do