anacrolix / torrent

Full-featured BitTorrent client package and utilities
Mozilla Public License 2.0
5.58k stars 630 forks source link

Piece hash check fails for last piece of some torrent files #949

Closed spaceface777 closed 5 months ago

spaceface777 commented 5 months ago

I'm currently working on a custom torrent client and storage backend for the library, and I created some test torrent files in order to test it (using qBittorrent 4.6.5, in case that helps), but I've noticed that one of them fails to download because the last piece's hash check fails (so the peer gets banned). At first I assumed this was a bug in either my code or the torrent file I created, but after further testing I experienced the same issue with a few other public torrent files, so I decided to look into it further, and discovered that the bug still occurs when using the sample client in this repo.

Here is the same test.torrent.zip that I used, in case it helps.

I wrote the following patch for this library, which does work around the issue for me, but I'm not familiar enough with the codebase or the bittorrent protocol in general to know if this is a proper fix.

diff --git a/torrent.go b/torrent.go
diff --git a/cmd/torrent/download.go b/cmd/torrent/download.go
index 60482018..8c1aa386 100644
--- a/cmd/torrent/download.go
+++ b/cmd/torrent/download.go
@@ -146,6 +146,9 @@ func addTorrents(
                if flags.Progress {
                        torrentBar(t, flags.PieceStates)
                }
+               // download the last piece of the torrent first to make reproduction quicker
+               t.Piece(t.NumPieces() - 1).SetPriority(torrent.PiecePriorityNow)
+
                t.AddPeers(testPeers)
                wg.Add(1)
                go func() {
diff --git a/torrent.go b/torrent.go
index 3a5d358c..3b350747 100644
--- a/torrent.go
+++ b/torrent.go
@@ -1175,6 +1175,19 @@ func (t *Torrent) hashPiece(piece pieceIndex) (
                var sum [20]byte
                sumExactly(sum[:], h.Sum)
                correct = sum == *p.hash
+
+               if !correct && piece == t.NumPieces()-1 {
+                       // try to pad the last piece with zeroes
+                       paddingLen := t.info.PieceLength - p.Info().Length()
+                       io.CopyN(h, zeroReader, paddingLen)
+                       sumExactly(sum[:], h.Sum)
+                       correct = sum == *p.hash
+
+                       if correct {
+                               log.Printf("patch worked")
+                       }
+               }
        } else if p.hashV2.Ok {
                h := merkle.NewHash()
                differingPeers, err = t.hashPieceWithSpecificHash(piece, h)
anacrolix commented 5 months ago

What version of anacrolix/torrent are you using? To give some context, some of this would have changed recently with the BitTorrent v2 support. It would likely be a regression to have lost support for that. I see from the code it's likely the latest version.

spaceface777 commented 5 months ago

Yes, this is the latest release (v1.56.0)

anacrolix commented 5 months ago

Thank you for this detailed report. I believe the issue is that your hybrid torrent has a padding file after the last file. anacrolix/torrent doesn't expect that. I'll make some fixes to this logic in the next few days.

anacrolix commented 5 months ago

Reading the specification for this suggests that the padding after the last file should be observed https://www.bittorrent.org/beps/bep_0047.html, so anacrolix/torrent is probably deriving the wrong value for use in hashing.

anacrolix commented 5 months ago

@spaceface777 could you try https://github.com/anacrolix/torrent/tree/trailing-padding-file-v1-info?

spaceface777 commented 5 months ago

Yes, I can confirm that indeed resolves the issue. Thanks for the quick fix!

anacrolix commented 5 months ago

This is released in v1.56.1.