webseed magnet support is broken. crashes result

[cfpp2p]

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

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

[cfpp2p]

When do you get the crash

v2.33 is fine, 2.40 and beyond crashes

[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; 

[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

[livings124]

This is likely a duplicate of #5075

[jordan]

It overlaps, but this patch also tests to see if the torrent is a magnet before calling geturllist().

[jordan]

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

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

[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!

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

[cfpp2p]

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

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

[jordan]

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.

[x190]

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.

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

[jordan]

Webseeds have no business asking for blocks until after we have the metainfo...

[jordan]

Possible fix for the task_request_next_chunk() crash from r12539 that x190 describes in comment:16

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

[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?

[jordan]

webseed.c's task_request_next_chunk() should check for metainfo before doing anything.

[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?

[x190]

Still crashes. Crash is after acquisition of metadata.

[jordan]

x190, care to provide a new backgtrace and instructions on how to trigger it, and what build you're using?

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

[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."

[x190]

Ticket may be closed. Recent builds are fine.

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