Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#5081 closed Bug (fixed)

Transmission discards webseed URLs that have trailing whitespace

Reported by: x190 Owned by: jordan
Priority: Normal Milestone: 2.72
Component: libtransmission Version: 2.22+
Severity: Normal Keywords:
Cc:

Description

Webseeds are not used nor do they appear in Inspector->Peers post v2.22 on SL 10.6.8.

Test url (.torrent same issue):

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

Attachments (3)

tr-5081-rev-01.diff (872 bytes) - added by jordan 10 years ago.
strip whitespace from webseed URLs
tr-5081-rev-02.diff (287 bytes) - added by jordan 10 years ago.
fix rev 01 oops: tr_strstrip() is inplace & doesn't allocate a new string
tr-5081-rev-03.diff (704 bytes) - added by jordan 10 years ago.
Apply tr_strstrip() before calling tr_urlIsValid(). (Note to x190, this supercedes the previous revisions :)

Download all attachments as: .zip

Change History (35)

comment:1 Changed 10 years ago by x190

https://forum.transmissionbt.com/download/file.php

https://forum.transmissionbt.com/download/file.php

Starting torrent and getting metadata from peers makes no difference.

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

comment:2 Changed 10 years ago by livings124

  • Component changed from Mac Client to libtransmission
  • Owner changed from livings124 to jordan

comment:3 Changed 10 years ago by x190

livings124: Are you seeing this problem with Lion/Mtn? Lion as well? https://trac.transmissionbt.com/ticket/5072#comment:5 appears to indicate otherwise which is why I thought it was Mac Client Snow Leopard specific.

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

comment:4 follow-up: Changed 10 years ago by livings124

  • Component changed from libtransmission to Mac Client
  • Owner changed from jordan to livings124

The web seed appears on 10.8.2. Are there any error messages in Console.app?

comment:5 in reply to: ↑ 4 Changed 10 years ago by x190

Replying to livings124:

The web seed appears on 10.8.2. Are there any error messages in Console.app?

No.

Q: When did you drop 10.6 SDK (revision number)?

A: r12569, so that's not the problem.

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

comment:6 follow-up: Changed 10 years ago by livings124

  • Component changed from Mac Client to libtransmission
  • Owner changed from livings124 to jordan

On 10.8, the web seed doesn't seem to appear at first, only showing up after a bit. Punting back to libtransmission.

comment:7 in reply to: ↑ 6 Changed 10 years ago by x190

Replying to livings124:

On 10.8, the web seed doesn't seem to appear at first, only showing up after a bit. Punting back to libtransmission.

That may be due to r13547 and r13549. What happens when you test with older versions such as v2.30?

comment:8 Changed 10 years ago by x190

r12416 is the revision that broke webseeds.

livings, you're right, if you wait long enough they do show up for me too (r13555). But only sometimes and I think only if other peers don't fill up the bandwidth first. Also apparently, you mostly have to unpause the transfer but not always!!!!!???

Summary: All webseed revisions starting with r12416 and including r12848, r13547, and r13549 need to be reviewed and re-written.

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

comment:9 Changed 10 years ago by x190

  • Summary changed from Webseeds are broken on all versions of Mac Client v2.30 and up (SL 10.6.8) to Webseeds are broken on all versions of Mac Client r12416
  • Version changed from 2.30 to 2.22+

comment:10 Changed 10 years ago by x190

  • Summary changed from Webseeds are broken on all versions of Mac Client r12416 to Webseeds are broken on all versions of Mac Client r12416 and up.

comment:11 follow-up: Changed 10 years ago by cfpp2p

I'm not sure exactly what you are seeing on MAC since I don't use it, but my understanding of webseed is that webseed is sort of a backup in cases where peers are not supplying the necessary blocks. Gaps are spaces of multiple pieces in a row that the client does not have. http://www.bittorrent.org/beps/bep_0019.html So it may be that the implementation does not always or immediately use the webseed(s).

Can someone with MAC test this torrent and post the results?

http://computerfixpro.com/webseed-only.zip

comment:12 in reply to: ↑ 11 Changed 10 years ago by x190

Replying to cfpp2p:

I'm not sure exactly what you are seeing on MAC since I don't use it, but my understanding of webseed is that webseed is sort of a backup in cases where peers are not supplying the necessary blocks. Gaps are spaces of multiple pieces in a row that the client does not have. http://www.bittorrent.org/beps/bep_0019.html So it may be that the implementation does not always or immediately use the webseed(s).

Can someone with MAC test this torrent and post the results?

http://computerfixpro.com/webseed-only.zip

Works as a .torrent but not as a magnet. On the Mac Client a webseed should show up as peer in Inspector->Peers immediately, even for paused magnet links, as my screenshot illustrates. As stated in comment:8 this became broken in r12416 which tests for url validity. Even with r13555 it only rarely works with my provided link which you should be testing with in all relevant forms, i.e. no trailing slash, trailing slash, trailing "%20", trailing "%2F", etc. Of course now, geturllist() is also broken for isMagnet (r13547).

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

comment:13 Changed 10 years ago by cfpp2p

hhhhmmmmm??? I'm not getting this with r13555 and web client.

provided magnet link paused at retrieving metadata (0.0%) has url-list correct

http://computerfixpro.com/url-list-no-metadata.JPG

and also after metadata 100% is correct

http://computerfixpro.com/url-list-have-metadata.JPG

This was consistently correct whether tested with no trailing slash, trailing slash, trailing "%20", trailing "%2F and correctly showed the / or no /, etc.

I'm not sure this isn't just a MAC inspector problem loading the list that is already there in both the paused or started cases.

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

comment:14 Changed 10 years ago by x190

TO FIX:

1) Revert r13547. Apparently this is not the correct fix for #5069 anyway.

2) Whitespace at the end of a magnet link url (with webseed) whether in the form of "%20" or simply typed in via the "Open Torrent Address..." dialog is not being handled. This became an issue starting with r12416 which added the use of the tr_urlIsValid() function to geturllist() and now moved to fix_webseed_url() which is called by geturllist().

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

comment:15 Changed 10 years ago by cfpp2p

OK, so Ive tested r13557 with both magnetlink and .torrent local metafile of a multifile torrent with the webseeding server files in a sub-directory of root.

' BitTorrent? clients normally use the "name" from the torrent info section to make a folder, then use the "path/file" items from the info section within that folder. For the case of Multi-File torrents, the "url-list" must be a root folder where a client could add the same "name" and "path/file" to create the URL for the request. '

Validating that http://bittorrent.org/beps/bep_0019.html#metadata-extension is indeed working correctly and downloading properly. Variable amounts of whitespace and whether URLs for multfile torrents end in a slash or not ( see fix_webseed_url() ) all downloaded as expected.

Tested magnetlinks in formats such as &ws=http://<example.com/> and &ws=http://<example.com> where the weebseed files are located at http://<example.com/subdir/> functioned correctly.

The only case I could create that the webseed url was non-functional is when whitespace is between an included slash. i.e. &ws=http://<example.com /> and this I consider a fringe case.

So I don't see that the problem is a libtransmission issue.

comment:16 Changed 10 years ago by x190

TO FIX:

1) Revert r13547. Apparently this is not the correct fix for #5069 anyway.

Just because the url shows up in "url-list" doesn't get the job done for the Mac Client. geturllist() needs to be called as well and r13547 breaks this call for magnet links.

2)The trailing whitespace can occur on copy/paste or select/drag to Desktop (inetloc) from certain sites (like trac, for example) and will create frustration if not noticed. I suppose it could be fixed by checking for " [url-1] == '%20' " and then fixing the url. This was not an issue until r12416 and the introduction of tr_urlIsValid(), although further testing shows possibly that the webseed was not used but only listed in Inspector->Peers. The point would be to strip trailing whitespace from a webseed url as part of fix_webseed_url() rather than dropping it.

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

comment:17 in reply to: ↑ description Changed 10 years ago by x190

Replying to x190:

Webseeds are not used nor do they appear in Inspector->Peers post v2.22

(.torrent same issue):

(.torrent same issue): This refers only to the trailing whitespace issue which is common to both magnet and .torrent plus webseed. The point would be to strip trailing whitespace from a webseed url as part of fix_webseed_url() rather than dropping it.

The primary issue is r13547 which needs to be reverted as geturllist() no longer gets called for magnet links with webseeds.

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

comment:18 Changed 10 years ago by x190

  • Summary changed from Webseeds are broken on all versions of Mac Client r12416 and up. to Webseeds from magnet links are broken on all versions of Mac Client r12416 and up.

comment:19 Changed 10 years ago by x190

static char*
fix_webseed_url( const tr_info * inf, const char * url )
{
    char * ret = NULL;
    const size_t len = strlen( url );
    
    if( ( url[len-1] == ' ' ) && ( len > 0  ) )
    {
        ret = tr_strndup( url, len );
        ret[len-1] = '/' ;

	
	if( tr_urlIsValid( ret, len ) )
	    return ret;
    }

    else 
        if( tr_urlIsValid( url, len ) )
        {
            if( ( inf->fileCount > 1 ) && ( len > 0 ) && ( url[len-1] != '/' ) )
                ret = tr_strdup_printf( "%*.*s/", (int)len, (int)len, url );
            else
                ret = tr_strndup( url, len );
        }
   return ret;
}

Patch for trailing whitespace issue. A little more effort is needed in the case of multiple whitespaces but normally only one has to be dealt with.

http://www.unix.com/programming/21264-how-trim-white-space-around-string-c-program.html

comment:20 Changed 10 years ago by jordan

  • Milestone None Set deleted

r13558: (trunk libT) fix webseed-in-magnet-link regression introduced in r13547 (2.71+). This issue was tracked down by by x190 in ticket #5081

comment:21 follow-up: Changed 10 years ago by jordan

It looks to me that there are two unrelated bugs being talked about here: (1) the regression introduced in r13547, and (2) the need to strip whitespace from the URLs.

For bookkeeping's sake, let's handle these separately. Ordinarily I'd say let's file a separate ticket for (1) but since the regression is only 8 days old and not in any released version of Transmission, I've bumped it back to bug #5069 where the regression was introduced.

That leaves the whitespace for this ticket. I've changed the Summary to reflect this.

x190, we already have tr_strstrip() for stripping whitespace. If we use it here, we'll avoid duplicate code. Plus, tr_strstrip() already has unit tests.

cfpp2p, x190, what do you two think of tr-5081-rev-01.diff?

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

Changed 10 years ago by jordan

strip whitespace from webseed URLs

comment:22 Changed 10 years ago by jordan

  • Milestone set to 2.72
  • Status changed from new to assigned
  • Summary changed from Webseeds from magnet links are broken on all versions of Mac Client r12416 and up. to Transmission discards webseed URLs that have trailing whitespace

Changed 10 years ago by jordan

fix rev 01 oops: tr_strstrip() is inplace & doesn't allocate a new string

comment:23 in reply to: ↑ 21 Changed 10 years ago by x190

That leaves the whitespace for this ticket. I've changed the Summary to reflect this.

x190, we already have tr_strstrip() for stripping whitespace. If we use it here, we'll avoid duplicate code. Plus, tr_strstrip() already has unit tests.

cfpp2p, x190, what do you two think of tr-5081-rev-01.diff?

Testing with diff1 + diff2:

Gets TurboT's 5 crash (⤋⤋⤋⤋⤋) rating. Three for crashing and two more for invalid memory access and display. :(

The following works well for multiple trailing whitespaces. Could someone do more testing?

static char*
fix_webseed_url( const tr_info * inf, const char * url )
{
    char * ret = NULL;
    char * slashed = NULL;
    const size_t len = strlen( url );
    size_t fixed;
      
    if( ( len > 0  ) )
    {
	slashed = tr_strndup( url, len );
	ret = tr_strstrip( slashed ); /* Strip trailing whitespace before validity testing. */
	fixed = strlen( ret );
	
	if( tr_urlIsValid( ret, fixed ) && ( inf->fileCount <= 1 ) )
            /* tr_free ( slashed ) ????? */
	    return ret;
	
	else 
	    if( ( inf->fileCount > 1 ) && ( fixed > 0 ) && ( ret[fixed-1] != '/' ) )
	    {
		slashed = tr_strdup_printf( "%*.*s/", (int)fixed, (int)fixed, ret );
		
		if( tr_urlIsValid( slashed, fixed+1 ) )
                    /* tr_free ( ret ) ????? */
		    return slashed;
	    }
    }
 
    return NULL;
}

Note: Not yet tested with those tr_free bits.

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

comment:24 follow-up: Changed 10 years ago by cfpp2p

tr-5081-rev-01.diff should NOT be applied, it was a little unclear but rev-02 is a correction for rev-01. I applied tr-5081-rev-02.diff. It looks fine to me. Same tests as above in (comment 15) and same results. Still functioning correctly --- via web-client. With the web client and a third party client I can't seem to force through whitespace after the URL in the first place anyway.

comment:25 in reply to: ↑ 24 Changed 10 years ago by x190

Replying to cfpp2p:

tr-5081-rev-01.diff should NOT be applied, it was a little unclear but rev-02 is a correction for rev-01. I applied tr-5081-rev-02.diff. It looks fine to me. Same tests as above in (comment 15) and same results. Still functioning correctly --- via web-client. With the web client and a third party client I can't seem to force through whitespace after the URL in the first place anyway.

I did use both in conjunction, however, rev-02.diff by itself is useless, as urls will be dropped by tr_urlIsValid() long before they hit that one-liner. cfpp2p, "urllist" is not connected to what fix_webseed_url() does, at least afaict. Please give my code above a spin. ( And have a great day. :-) )

comment:26 Changed 10 years ago by cfpp2p

OK, I've finaly figured out a way to FORCE whitespace and have seen what is being described, so I'll patch and test when I get a chance...

Changed 10 years ago by jordan

Apply tr_strstrip() before calling tr_urlIsValid(). (Note to x190, this supercedes the previous revisions :)

comment:27 Changed 10 years ago by jordan

Well I'm on crack, rev03 leaks the new temporary string.

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

comment:28 Changed 10 years ago by jordan

r13567 (trunk, libT) #5081 'Transmission discards webseed URLs that have trailing whitespace' -- add unit test (currently failing) for this bug.

comment:29 Changed 10 years ago by jordan

r13568 (trunk, libT) #5081 'Transmission discards webseed URLs that have trailing whitespace' -- fixed. Unit test passes and Valgrind says 'All heap blocks were freed -- no leaks are possible'

comment:30 Changed 10 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:31 Changed 10 years ago by cfpp2p

jordan, I'm a little late but this worked good for me...

static char*
fix_webseed_url( const tr_info * inf, const char * url )
{
    char * ret = NULL;
    /* strip trailing whitespace */
    const char * url_nows = tr_strstrip( tr_strndup( url, strlen( url ) ) );
    const size_t len = strlen( url_nows );

    if( tr_urlIsValid( url_nows, len ) )
    {
        if( ( inf->fileCount > 1 ) && ( len > 0 ) && ( url_nows[len-1] != '/' ) )
            ret = tr_strdup_printf( "%*.*s/", (int)len, (int)len, url_nows );
        else
            ret = tr_strndup( url_nows, len );
    }

    return ret;
}

comment:32 Changed 10 years ago by x190

r13568 looks clean and in initial tests works correctly. TurboT rating is ☆☆☆☆☆.

Jordan, good luck with your c (programming of course) problem. :)

Note: See TracTickets for help on using tickets.