Opened 4 years ago

Last modified 4 years ago

#5773 new Bug

heap overflow with some torrents, randomly resulting in segfault

Reported by: c1234 Owned by:
Priority: Normal Milestone: None Set
Component: Transmission Version: 2.51+
Severity: Major Keywords: heap overflow, segfault, resume, loadProgress
Cc:

Description

With some torrents, I get a seg fault at startup which prevents transmission from launching.

One way to get around this problem was for me to manually remove both the resume and torrent file from the transmission .config directory. (thank you very much for the fact that the organization of data in the .config directory is crystal clear)

Then, I tracked down the bug and here is all the information (and a potential patch), that I found. First, this occurs for multiple files torrent, when the N last torrents have size 0. (this may happen as people tend to use 0-sized files to store meta-data via the file name). Example: Piece count: 6491 Piece size: 524288 Total size: 3403153408 File 0 has length: 244657 File 1 has length: 279631 File 2 has length: 3402563740 File 3 has length: 65380 File 4 has length: 0 File 5 has length: 0 File 6 has length: 0 File 7 has length: 0

On such torrent, the information stored for file 4 to file 7 in the torrent information leads to heap overflow line 534 of file libtransmission/resume.c. In more details, if we look at function loadProgress (starting line 487), when fi is equal to 4, then f->firstPiece will be equal to f->lastPiece and exactly inf->pieceCount. So this line: 'p->timeChecked = (time_t)t;' will write just outside the allocated area for the pieces information.

To make myself clearer, I have written an automatic non-regression test (see attached file). When run with valgrind, this test will exhibit the heap overflow.

One quick patch, could be to add the condition "&& f->firstPiece<inf->pieceCount" to the halting condition of the loop line 533. But that is really a quick fix, which is not fully satisfactory.

A better solution, would rather to change the way the information is encoded for each file: instead of keeping firstPiece and lastPiece, one could equivalently keep firstPiece and pieceCount (for the file). So that the loop could iterate over pieceCount. Then files of size 0, would have a pieceCount of 0 and the loop would not be entered.

I believe this bug is still present (I did a rapid manual check) in the trunk version of transmission.

Anyway, thank you for maintaining a great software!

Attachments (4)

resume_unit_test.c (3.1 KB) - added by c1234 4 years ago.
non-regression test with bug reproduction
ZERO-Byte-mult-file-test.zip (3.1 KB) - added by cfpp2p 4 years ago.
ZERO-Byte-mult-file-test2.zip (3.2 KB) - added by cfpp2p 4 years ago.
file ends exactly on piece boundary, still no crash for me
ZERO-Byte-mult-file-test3.zip (4.9 KB) - added by cfpp2p 4 years ago.

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by c1234

non-regression test with bug reproduction

Changed 4 years ago by cfpp2p

comment:1 Changed 4 years ago by cfpp2p

I tried to create a test torrent of this situation but could not generate a crash upon restart or any other time either. Tested 2.51, 2.52, 2.77 and trunk. Attached is the test torrent and data.

c1234, could you try the attached and post the results.

Maybe I got something wrong.

Changed 4 years ago by cfpp2p

file ends exactly on piece boundary, still no crash for me

comment:2 Changed 4 years ago by cfpp2p

Ok, when there are multiple files and they are mixed with zero length files in the file order like this:

resume_unit_test1.c         16,384
zero1                       0
zero2                       0
zero3                       0
zero4-resume_unit_test.c    16,384
zero5                       0
zero6                       0

then 2.51 & 2.52 do crash but 2.77 and 2.84 don't. Whatever the problem was it appears fixed to me, at least with the test data ZERO-Byte-mult-file-test3.zip I've attached now.

Changed 4 years ago by cfpp2p

comment:3 Changed 4 years ago by cfpp2p

I'm now reasonably sure now that what c1234 reported is fixed by r13592 as described at ticket:5097#comment:1

Last edited 4 years ago by cfpp2p (previous) (diff)
Note: See TracTickets for help on using tickets.