Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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)

webseedmagnetdis.patch (931 bytes) - added by cfpp2p 8 years ago.
tr-5069-rev-01.diff (481 bytes) - added by jordan 8 years ago.
Possible fix for the task_request_next_chunk() crash from r12539 that x190 describes in comment:16
tr-5069-rev-02.diff (491 bytes) - added by jordan 8 years ago.
webseed.c's task_request_next_chunk() should check for metainfo before doing anything.
optimised_webseed_crash_fix.patch (2.2 KB) - added by x190 8 years ago.

Download all attachments as: .zip

Change History (34)

Changed 8 years ago by cfpp2p

comment:1 Changed 8 years ago by x190

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.

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; 
Last edited 8 years ago by x190 (previous) (diff)

comment:4 Changed 8 years ago by livings124

  • Version changed from 2.71+ to 2.71

comment:5 Changed 8 years ago by x190

I can't get either one to crash in r13542 (Mac Client) although oddly the second one doesn't show the webseed in the Inspector->Peers window (v2.22 does).

Guess we need that bt, amigo.

@livings124: N.B. "the second one doesn't show the webseed in the Inspector->Peers window" SL 10.6.8 r13542

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

Half of this patch already made it in for #5075, I've committed the other half here in r13547.

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

jordan,

Nice job combining this ticket with #5063 r13549 to fix both cleanly. I noticed resume.c saveProgress() was corrupting progress time-checked but wasn't sure of a nice way to fix. So I find your fix for all of this nicely done!

comment:11 Changed 8 years ago by x190

@livings124: N.B. "the second one doesn't show the webseed in the Inspector->Peers window" SL 10.6.8 r13542 or r13550. Webseeds (from magnet link) are non-functional on SL 10.6.8.

See trac ticket #5081.

Last edited 8 years ago by x190 (previous) (diff)

comment:12 follow-up: Changed 8 years ago by cfpp2p

the second one here is for test crash only. no webseed will occur.

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

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).

Last edited 8 years ago by x190 (previous) (diff)

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.

Last edited 8 years ago by jordan (previous) (diff)

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.

Last edited 8 years ago by x190 (previous) (diff)

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.

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

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.

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

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?

Last edited 8 years ago by jordan (previous) (diff)

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

Last edited 8 years ago by x190 (previous) (diff)

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.

Last edited 8 years ago by x190 (previous) (diff)

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.

Last edited 8 years ago by x190 (previous) (diff)

Changed 8 years ago by x190

Note: See TracTickets for help on using tickets.