[FIXED] transmission-daemon 2.30b4 generate broken announce

[blacklion]

Here is such request, copy-pasted from tcpdump:

Code: Select all

GET /ann?uk=xxxxxxxxxx&info_hash=%d2%b5i%03%3ckj%e7%e6%e9%8eu%0fi%15K%b1%f8%ae%05&peer_id=-TR222X-qk4227aepazp&port=16881&uploaded=0&downloaded=0&left=0&numwant=0&key=300fcba3&compact=1&supportcrypto=1&event=stopped&ipv6=2001%3A470%3A923f%3A1%3A21e%3A8cff%3Afe75%3A30dannounce":"http://ix4.rutracker.net/ann?uk=xxxxxxxxxx","announceState":0,"downloadCount":0,"hasAnnounced":true,"hasScraped":true,"host":"http://ix4.rutracker.net:80","id":2,"isBackup":false,"lastAnnouncePeerCount":8,"lastAnnounceResult":"Could not connect to tracker","lastAnnounceStartTime":1304588128,"lastAnnounceSucceeded":true,"lastAnnounceTime":1304588129,"lastAnnounceTimedOut":false,"lastScrapeResult":"","lastScrapeStartTime":0,"lastScrapeSucceeded":true,"lastScrapeTime":1304588129,"lastScrapeTimedOut":0,"leecherCount":0,"nextAnnounceTime":0,"nextScrapeTime":1304589930,"scrape":"","scrapeState":1,"seederCount":0,"tier":2}],"uploadRatio":1.3943,"uploadedEver":6101209062},{"activityDate":1304535057,"downloadedEver":3461054,"errorString":"","eta":-1,"id":357,"leftUntilDone":0,"name":"The Bat!","peersGettingFromUs":0,"peersSendingToUs":0,"rateDownload":0,"rateUpload":0,"recheckProgress":0,"sizeWhenDone":61286940,"status":16,"totalSize":61286940,"trackerStats":[{"announce":"http://bt.example.com/ann?uk=xxxxxxxxxx","announceState":0,"downloadCount":-1,"hasAnnounced":true,"hasScraped":false,"host":"http://bt.example.com:80","id":0,"isBackup":false,"lastAnnouncePeerCount":0,r":3651819164},{60,,{ HTTP/1.1

Please note these strange things:
(1) Part after &ipv6 -- it seems, that here is something missed, I don't believe, that "%3A30dannounce":" is Ok.
(2) "http://ix4.rutracker.net/ann" is part of OTHER torrent, this one doesn't contain this tracker, only "http://bt.example.com/ann..."
(3) All request us truncated, ",{60,,{ " doesn't look as proper end of request to me.
(4) This is, really, was "started" message! I've unpause torrent to capture this!
(5) Sometimes there is binary garbage after "ipv6=" part of request, in case of true "stopped" message.

It seems, that problem is here:

(announce-http.c:111)

Code: Select all

    ipv6 = tr_globalIPv6( );
    if( ipv6 ) {
        char ipv6_readable[INET6_ADDRSTRLEN];
        evutil_inet_ntop( AF_INET6, ipv6, ipv6_readable, INET6_ADDRSTRLEN );
        evbuffer_add_printf( buf, "&ipv6=");
        tr_http_escape( buf, ipv6_readable, -1, true );
    }

    return buf;

Older versions (2.22) uses such code:

Code: Select all

    ipv6 = tr_globalIPv6( );
    if( ipv6 ) {
        char ipv6_readable[INET6_ADDRSTRLEN];
        inet_ntop( AF_INET6, ipv6, ipv6_readable, INET6_ADDRSTRLEN );
        evbuffer_add_printf( buf, "&ipv6=");
        tr_http_escape( buf, ipv6_readable, -1, TRUE );
    }

    return evbuffer_free_to_str( buf );

[blacklion]

Ok, problem is not exactly here, but something in evbuffer_pullup() which now converts evbuffer into URL string.
Documentation on evbuffer_pullup() DOESN'T guarantee '\0' as last character of returned array. And new code (announcer-http.c:283) relies on this zero byte! Old code appends zero ``by hands''

[blacklion]

This patch helps me:

Code: Select all

--- libtransmission/announcer-http.c.orig       2011-04-30 04:58:23.000000000 +0400
+++ libtransmission/announcer-http.c    2011-05-05 14:29:40.000000000 +0400
@@ -115,6 +115,7 @@
         evbuffer_add_printf( buf, "&ipv6=");
         tr_http_escape( buf, ipv6_readable, -1, true );
     }
+    evbuffer_add( buf, "\0", 1);

     return buf;
 }

[blacklion]

Other suspicious evbuffer_pullup() usages (all other usages seems to use evbuffer_get_length() together with evbuffer_pullup()):

Code: Select all

daemon/remote.c:1764
                fprintf( stderr, "Unexpected response: %s\n", evbuffer_pullup( buf, -1 ) );

libtransmission/announcer-http.c:216
            fprintf( stderr, "Announce response:\n< %s\n", evbuffer_pullup( buf, -1 ) );

libtransmission/announcer.c:743
        tr_deepLog( __FILE__, __LINE__, name, "announce queue is %s", evbuffer_pullup( buf, -1 ) );

libtransmission/bencode-test.c:327
    serialized = (char*) evbuffer_pullup( buf, -1 );

libtransmission/peer-msg.c:254
        fputs( (const char*)evbuffer_pullup( buf, -1 ), fp );

libtransmission/utils.c:244
        OutputDebugString( evbuffer_pullup( buf, -1 ) );
        if( fp )
            fputs( (const char*)evbuffer_pullup( buf, -1 ), fp );

libtransmission/webseed.c:293
        tr_webRunWithBuffer( t->session, (char *) evbuffer_pullup( url, -1 ),
                             range, NULL, web_response_func, t, t->content );

It is mostly debug output, but libtransmission/webseed.c:293 looks very bad.

[Jordan]

This is very close to the right fix... and, great detective work BTW.

evbuffer_pullup() isn't the issue though. The problem is mixing use of evbuffer_add_printf() (which generates trailing zeroes) with evbuffer_add() (which doesn't) and then assuming the result is zero-terminated.

It looks to me like the real bug is in web.c's tr_http_escape() which has a branch that *doesn't* finish with a trailing zero... that can bite announcer-http.c's announce_url_new() and webseed.c's make_url() because they both have code branches that can call tr_http_escape() last.

One clean fix this is to make tr_http_escape() not mix the evbuffer calls. Does r12417 fix the issue for you?

[blacklion]

Yep, I've reverted my fix and added this one -- it works!