Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6048 closed Bug (fixed)

crash in torrent-magnet.c tr_torrentSetMetadataPiece()

Reported by: cfpp2p Owned by: mike.dld
Priority: Normal Milestone: 2.90
Component: libtransmission Version: 2.84
Severity: Normal Keywords:
Cc:

Description

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

Attachments (4)

tr-tsmp.patch (2.2 KB) - added by cfpp2p 6 years ago.
patch to fix
backtrace-segfault.txt (3.5 KB) - added by cantabile 6 years ago.
6048-metadata-crash.patch (3.8 KB) - added by mike.dld 6 years ago.
6048-metadata-crash.2.patch (3.8 KB) - added by mike.dld 6 years ago.

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by cfpp2p

patch to fix

Changed 6 years ago by cantabile

comment:1 follow-up: Changed 6 years ago by 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.

Changed 6 years ago by mike.dld

comment:2 follow-up: Changed 6 years ago by 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).

Changed 6 years ago by mike.dld

comment:3 Changed 6 years ago by mike.dld

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

comment:4 Changed 6 years ago by mike.dld

  • Owner changed from jordan to mike.dld
  • Status changed from new to assigned

comment:5 Changed 6 years ago by cantabile

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

comment:6 in reply to: ↑ 2 Changed 6 years ago by 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.

Last edited 6 years ago by cfpp2p (previous) (diff)

comment:7 in reply to: ↑ 1 Changed 6 years ago by 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().

comment:8 follow-up: Changed 6 years ago by mike.dld

  • Milestone changed from None Set to 2.90
  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:9 in reply to: ↑ 8 Changed 6 years ago by 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.

Last edited 6 years ago by cfpp2p (previous) (diff)

comment:10 Changed 6 years ago by 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.

Note: See TracTickets for help on using tickets.