crash in torrent-magnet.c tr_torrentSetMetadataPiece()

[cfpp2p]

In tr_torrentSetMetadataPiece() received extension header hand-shake metadata_size is not checked for validity against received data message total_size. A crash in tr_torrentSetMetadataPiece() will occur when metadata_size exceeds total_size.

Occurs when a faulty client connects to transmission.

i.e.

metadata is 1637022352 bytes in 99916 pieces (torrent-magnet.c:81)
[µTorrent 3.4.0 (Beta)]: got ut_metadata msg: type 1, piece 0, total_size 14308 (peer-msgs.c:990)

reference: ​http://www.bittorrent.org/beps/bep_0009.html

[cfpp2p]

patch to fix

[cantabile]

Does your crash look like my crash? The backtrace is attached. I, too, have a magnet link with one uTorrent 3.4.0 beta peer. Your patch makes the crashes stop, but then the peer just sits there and the metadata isn't downloaded.

I'm running Transmission 2.84 (the daemon) on Arch Linux. The latest revision from svn (r14655) has the same problem.

[mike.dld]

The issue I see is that memory allocation fails and m->metadata stays NULL, which leads to crash afterwards. I don't think there really is a way to know which one, if any at all, of metadata_size and total_size is valid (correct me if I'm wrong). Well, except for collecting statistics of what different peers report and use the most frequent one...

Attaching another patch which should prevent crashes (wasn't yet able to reproduce myself).

[mike.dld]

Sorry, wanted to replace existing patch but failed :) Second one just moves size check in tr_torrentSetMetadataSizeHint down after debug message output.

[cantabile]

There are no more crashes with 6048-metadata-crash.2.patch.

[cfpp2p]

Replying to cantabile:

Does your crash look like my crash?

Yes.

Your patch makes the crashes stop, but then the peer just sits there and the metadata isn't downloaded.

To stop that we would need something similar to tr_peerMgrGotBadPiece() https://trac.transmissionbt.com/browser/trunk/libtransmission/peer-mgr.c#L2216
With the patch if other good peers are available the metadata will be downloaded OK.

Replying to mike.dld:

I don't think there really is a way to know which one, if any at all, of metadata_size and total_size is valid (correct me if I'm wrong).

/* does this data pass the smell test? */
The original 'smell test' has been since 2009-11-24 https://trac.transmissionbt.com/browser/trunk/libtransmission/torrent-magnet.c?rev=9550#L142
So no doubt this is an obscure circumstance that has not been reported for more than 6 years.

Once we have the metadata I have found that the bad uTorrent 3.4.0 beta peer downloads torrent data correctly.

[cfpp2p]

Replying to cantabile:

...but then the peer just sits there and the metadata isn't downloaded.

None of the presented patches stop that. However, if we get the metadata from other peers and then the bad metadata peer (or any other peer) sends bad torrent piece data the peer will be dropped by peer-mgr.c addStrike().

[mike.dld]

Committed my patch with some more changes in r14664. Since crashes should no longer occur I'm going to close this one. Feel free to create another ticket for possible demagnetizing stalling.

[cfpp2p]

Replying to mike.dld:

Committed my patch with some more changes in r14664. Since crashes should no longer occur I'm going to close this one. Feel free to create another ticket for possible demagnetizing stalling.

Thank you for r14664. Although I might prefer a less heavy duty patch, it does stop these crashes. I'm only getting stalling if there is just one peer and it is the bad peer. This might be expected, as the bad peer might not be able to transfer the correct metadata anyway.

[cfpp2p]

In unpatched transmission code the µTorrent 3.4.0 (Beta) client can result in negative values too:

metadata is -1218034096 bytes in -74341 pieces (torrent-magnet.c:81)

So the problem was not just when metadata_size exceeded total_size as I stated in the initial description. Just wanted to clarify that.