#5069 closed Bug (fixed)
webseed magnet support is broken. crashes result
Reported by: | cfpp2p | Owned by: | jordan |
---|---|---|---|
Priority: | Normal | Milestone: | 2.72 |
Component: | libtransmission | Version: | 2.71 |
Severity: | Normal | Keywords: | |
Cc: |
Description
webseed magnet support is broken. crashes result
magnet:?xt=urn:btih:14FFE5DD23188FD5CB53A1D47F1289DB70ABF31E&dn=ubuntu+12+04+1+desktop+32+bit&tr=http%3A%2F%2Ftracker.publicbt.com%2Fannounce&tr=udp%3A%2F%2Ftracker.publicbt.com%3A80&ws
magnet:?xt=urn:btih:14FFE5DD23188FD5CB53A1D47F1289DB70ABF31E&dn=ubuntu+12+04+1+desktop+32+bit&tr=http%3A%2F%2Ftracker.publicbt.com%2Fannounce&tr=udp%3A%2F%2Ftracker.publicbt.com%3A80&ws=http://transmissionbt.com
two different problems.
webseedmagnetdis.patch stops these crashes but disables webseed support for magnet links ONLY. webseed is OK with .torrent metadata files
Attachments (4)
Change History (34)
Changed 8 years ago by cfpp2p
comment:1 Changed 8 years ago by x190
comment:2 Changed 8 years ago by cfpp2p
When do you get the crash
v2.33 is fine, 2.40 and beyond crashes
comment:3 Changed 8 years ago by x190
All tickets closed by v2.40
https://trac.transmissionbt.com/query?milestone=2.40&group=component&order=severity
https://trac.transmissionbt.com/changeset/12848
static void 358 386 geturllist( tr_info * inf, … … 374 402 if( tr_bencGetStr( tr_bencListChild( urls, i ), &url ) ) 375 403 { 376 const size_t len = strlen( url ); 377 378 if( tr_urlIsValid( url, len ) ) 379 inf->webseeds[inf->webseedCount++] = tr_strndup( url, len ); 404 char * fixed_url = fix_webseed_url( inf, url ); 405 406 if( fixed_url != NULL ) 407 inf->webseeds[inf->webseedCount++] = fixed_url;
comment:4 Changed 8 years ago by livings124
- Version changed from 2.71+ to 2.71
comment:5 Changed 8 years ago by x190
comment:6 Changed 8 years ago by livings124
This is likely a duplicate of #5075
comment:7 Changed 8 years ago by jordan
- Milestone changed from None Set to 2.72
- Status changed from new to assigned
It overlaps, but this patch also tests to see if the torrent is a magnet before calling geturllist().
comment:8 Changed 8 years ago by jordan
- Resolution set to fixed
- Status changed from assigned to closed
comment:9 Changed 8 years ago by cfpp2p
see if the torrent is a magnet before calling geturllist()
Jordan,
This effectively disables the ability to use a webseed from a magnet link. I haven't been able to fix the actual problem and allow for a webseed from a magnet link...
comment:10 Changed 8 years ago by cfpp2p
comment:11 Changed 8 years ago by x190
comment:12 follow-up: ↓ 13 Changed 8 years ago by cfpp2p
the second one here is for test crash only. no webseed will occur.
comment:13 in reply to: ↑ 12 Changed 8 years ago by x190
Replying to cfpp2p:
the second one here is for test crash only. no webseed will occur.
It should appear (as a url) in the Mac Client Inspector->Peers window regardless. This is confirmed by livings124 using Lion (https://trac.transmissionbt.com/ticket/5072#comment:5).
comment:14 Changed 8 years ago by jordan
- Resolution fixed deleted
- Status changed from closed to reopened
Okay, to summarize: sounds like the call to disable geturllist() (which cfpp2p wrote, and which I approved and committed in r13547) was a workaround to prevent the webseed crash. Since that crash is fixed anyway in r13546, the workaround is unnecessary, so even if that was the whole story it would already be enough to merit reverting the workaround.
In addition, x190's reporting (here and, in more detail, in bug #5081) that the workaround is causing a regression. So that's additional reason to revert.
Reverted in r13558.
If I'm misunderstanding the issue, please reopen this ticket and explain. Thanks.
comment:15 Changed 8 years ago by jordan
- Resolution set to fixed
- Status changed from reopened to closed
comment:16 Changed 8 years ago by x190
- Resolution fixed deleted
- Status changed from closed to reopened
The crash referred to by this ticket is due to r12539 ( tested with magnet url #2 in 'Description').
Thread 2 Crashed: 0 org.m0k.transmission 0x0000000100106cda task_request_next_chunk + 394 1 org.m0k.transmission 0x00000001001067df on_idle + 879 2 org.m0k.transmission 0x0000000100106321 webseed_timer_func + 81 3 org.m0k.transmission 0x000000010013c392 event_process_active_single_queue + 386 4 org.m0k.transmission 0x00000001001387ae event_process_active + 158 5 org.m0k.transmission 0x00000001001379e2 event_base_loop + 690 6 org.m0k.transmission 0x000000010013772a event_base_dispatch + 26 7 org.m0k.transmission 0x00000001000dbe50 libeventThreadFunc + 288 8 org.m0k.transmission 0x00000001000c89dd ThreadFunc + 45 9 libSystem.B.dylib 0x00007fff81b73fd6 _pthread_start + 331 10 libSystem.B.dylib 0x00007fff81b73e89 thread_start + 13
r13581 does seem to work correctly.
comment:17 Changed 8 years ago by cfpp2p
r13581 does seem to work correctly.
Yes it does.
I don't get this crash with 2.32 r12526 which doesn't include r12539 so x190 may have found the culprit. But I do crash with 2.33 r12564.
Now what r13549 does is inadvertently solve the crash of magnet url #2 in 'Description'. r13549's rebuildWebseedArray() inadvertently solves the crash because when tr_webseedNew() is initially created within webseed_timer_func() it is set corrupt/unknown if starting from a magnet link for some unknown reason. (It wasn't like this way back before ~2.40) So now we have rebuildWebseedArray() invoked on both torrentNew() or when the magnet link successfully has acquired the metadata from peers ( tr_peerMgrOnTorrentGotMetainfo() ); which is before the crash. Clearing the webseed array prevents the crash and then reloads correct webseed url data. What I have found is that magnet links with webseeds immediately and correctly utilize the webseed(s) whether with r13547 or r13547-reverted(r13558). It is confusing but the real fix comes from r13549. r13547 initially solved the crash, but did NOT allow webseeds from magnet links at all, but the reload from r13549 fixes that, whether or not we have r13547. As long as we have r13549 rebuildWebseedArray() reloads webseeds after the fact and works for me kicks in. Someone who could accurately and precisely follow the paths to the original crash via magnet url #2 in 'Description' will have to have the patience of a saint.
I consider this ticket works for me but not fixed; it is the illogical truth of massively patched software.
comment:18 Changed 8 years ago by jordan
- Resolution set to fixed
- Status changed from reopened to closed
Webseeds have no business asking for blocks until after we have the metainfo...
comment:19 Changed 8 years ago by jordan
- Resolution fixed deleted
- Status changed from closed to reopened
Changed 8 years ago by jordan
Possible fix for the task_request_next_chunk() crash from r12539 that x190 describes in comment:16
comment:20 Changed 8 years ago by cfpp2p
jordan,
that can't be the whole story as I've already tested with:
else if( !w->is_stopping && tor && tor->isRunning && !tr_torrentIsSeed( tor ) && want && tr_torrentHasMetadata( tor ) )
and still got crashes. Yours has a different order than what I tried but I'll try it again with your patch.
comment:21 Changed 8 years ago by jordan
cfpp2p, that's ok; the order shouldn't matter.
x190, cfpp2p, any suggestions on how to force this crash? I've downloaded the first 2-3 MB of that second ubuntu-12.04.01 torrent 8 or 9 times now inside of valgrind/gdb, with r13549 backed out, with no crashes. :(
x190: actually, after rereading your last comment and backtrace... are you saying this crash is still happening for you, or are you running an older version to get that crash?
Changed 8 years ago by jordan
webseed.c's task_request_next_chunk() should check for metainfo before doing anything.
comment:22 Changed 8 years ago by jordan
Okay, finally got it.
==9695== Thread 5: ==9695== Invalid read of size 4 ==9695== at 0x80CB1B1: task_request_next_chunk (webseed.c:507) ==9695== by 0x80CB4B2: on_idle (webseed.c:379) ==9695== by 0x80CB5D1: webseed_timer_func (webseed.c:544) ==9695== by 0x48FBA89: event_base_loop (in /usr/lib/i386-linux-gnu/libevent-2.0.so.5.1.7) ==9695== by 0x48FCBE2: event_base_dispatch (in /usr/lib/i386-linux-gnu/libevent-2.0.so.5.1.7) ==9695== by 0x4C8FD3D: clone (clone.S:130) ==9695== Address 0x0 is not stack'd, malloc'd or (recently) free'd
So urls is NULL because t->webseed->file_urls is NULL because there's no metainfo. I've added a patch (rev-02) to explicitly look for this, but IMO it's still hypothetical if r13549 prevents those conditions from ever being reached... so I'm a little confused why this ticket was reopened?
comment:23 Changed 8 years ago by x190
comment:24 Changed 8 years ago by x190
Still crashes. Crash is after acquisition of metadata.
comment:25 Changed 8 years ago by jordan
x190, care to provide a new backgtrace and instructions on how to trigger it, and what build you're using?
comment:26 Changed 8 years ago by cfpp2p
Jordan,
With current build I consider r13549 the correct fix (for this ticket#5069 also) with one minor change. The crash problem stems from peer-mgr.c torrentNew() loading the webseed array before we have metadata. If torrentNew() would have the line:
if( tr_torrentHasMetadata( tor ) ) rebuildWebseedArray( t, tor );
instead of just:
rebuildWebseedArray( t, tor )
I will consider this ticket#5069 closed. r12539 I don't believe was ever working correctly for magnet links with included webseeds.
Magnet links with webseeds will now function because we load the webseed array after we 100% acquire the completed metadata via tr_torrentSetMetadataPiece() eventually getting to rebuildWebseedArray() correctly.
torrent-magnet.c tr_torrentSetMetadataPiece() -> if( success ) tr_torrentGotNewInfoDict( tor ) -> torrent.c tr_torrentGotNewInfoDict() tr_peerMgrOnTorrentGotMetainfo( tor ) -> peer-mgr.c tr_peerMgrOnTorrentGotMetainfo() calling rebuildWebseedArray() once we have the complete magnet metadata.
tr_ptrArrayDestruct() inside rebuildWebseedArray() shouldn't be needed if were using torrentNew():
if( tr_torrentHasMetadata( tor ) ) rebuildWebseedArray( t, tor );
but it doesn't harm a thing other than a little wasted processing time.
comment:27 Changed 8 years ago by jordan
Still, I'd like to hear from x190 on this one. The revision history on comment:23 confuses me, and I'd like to hear more about this than just "Still crashes."
comment:28 Changed 8 years ago by x190
Ticket may be closed. Recent builds are fine.
comment:29 Changed 8 years ago by livings124
- Resolution set to fixed
- Status changed from reopened to closed
comment:30 Changed 8 years ago by x190
This patch is to be applied against r13594.
Avoids creating a corrupt webseed array as the array will only be built when we have the metadata.
Neither crash on my (admittedly customized :-) ) version of Transmission (v2.22). When do you get the crash and could you please post a CR from a nightly.