Extension protocol IDs are wrongly evaluated

[KyleK]

Current code in peer-msgs.c checks incoming messages from the extention protocol against constants:

if( ltep_msgid == LTEP_HANDSHAKE )
    {
        dbgmsg( msgs, "got ltep handshake" );
        parseLtepHandshake( msgs, msglen, inbuf );
        if( tr_peerIoSupportsLTEP( msgs->peer->io ) )
        {
            sendLtepHandshake( msgs );
            sendPex( msgs );
            sendTex( msgs );
        }
    }
    else if( ltep_msgid == TR_LTEP_PEX )
    {
        dbgmsg( msgs, "got ut pex" );
        msgs->peerSupportsPex = 1;
        parseUtPex( msgs, msglen, inbuf );
    }

According to ​BEP 10, "[...] only one ID is specified, 0. If the ID is 0, the message is a handshake message [...]".

Any additional extensions are identified by an extension name, which are far less likely to collide than a hard-coded set of IDs.

As a result, the BEP specifies that "The extension IDs must be stored for every peer, becuase [sic] every peer may have different IDs for the same extension.". Transmission does that already, the value is just not evaluated at all.

The above code must be changed to this:

/* This is OK, the handshake has a static ID of 0 */
if( ltep_msgid == LTEP_HANDSHAKE )
    {
        dbgmsg( msgs, "got ltep handshake" );
        parseLtepHandshake( msgs, msglen, inbuf );
        if( tr_peerIoSupportsLTEP( msgs->peer->io ) )
        {
            sendLtepHandshake( msgs );
            sendPex( msgs );
            sendTex( msgs );
        }
    }
    /* compare msg ID to the peer's own ut_pex ID */
    else if( ltep_msgid == msgs->ut_pex_id)
    {
        dbgmsg( msgs, "got ut pex" );
        msgs->peerSupportsPex = 1;
        parseUtPex( msgs, msglen, inbuf );
    }

The fact that the current code works fine for now can only be attributed to the low number of extensions and the rather low number of clients implementing them. As a result, most IDs are currently identical.

This might not be the case for long however, which in the best case means Transmission would dismiss a potential client in some way (disabling PEX for that client, for example, even though it supports it). At worst, Transmission might interpret received data wrongly, which could lead to data corruption or a program crash.

[KyleK]

Proposed patch

[charles]

It's easy to get confused about this because the spec is confusingly worded on this subtle point..

​http://lists.ibiblio.org/pipermail/bittorrent/2007-October/002184.html

So, uTorrent 1.7.5 sends me the id of the extension *I* sent to it, and expects me to send to it the extension *it* sent to me.

Actually, re-re-reading the spec again, it seems that uTorrent is right, though the wording of the spec is VERY subtle and one can easily miss the real meaning:

The extension message IDs are the IDs used to send the extension messages to the peer sending this handshake. i.e. The IDs are local to this particular peer.

So uTorrent seems to be right. Extension message ids YOU send should be the ones peer sent you in the handshake, and vice versa.

[charles]

Closing this ticket as invalid because Transmission's currently doing what I think it should be doing -- the extension message IDs Transmission sends (msgs->ut_pex_id) are the ones the peer sent us in the handshake, and vice versa (TR_LTEP_PEX).

Feel free to reopen if I'm confused about this. It wouldn't be the first time... ​both Transmission and Bitflu were confused on this before.

[KyleK]

I've stumbled upon this issue quite some while ago, but didn't want to open a ticket then because I figured maybe I haven't understood the BEP correctly. But since then I've read it multiple times and I am now quite sure that the spec contains this meaning:

While doing the handshake, the remote peer just tells you:

#1 What eproto extensions it does support (everything > listed && != 0) #2 What Id the peer will use to SEND eproto messages to YOU

m => { ut_pex = 5 } just means that the peer will use an id of '5' to send ut_pex messages to you. It does NOT mean that the peer is expecting ut_pex messages with an id of 5. You are free to choose (hardcode) your own ids for messages that YOU send.

I've now read it again after reading the other responses in that mailing thread, but I still think I'm right :)

Of course, if uTorrent does it the other way around, changing things up would break things.

Unfortunately, I don't see any significant advantage to the specification I favor, so I guess it is pretty mich irrelevant which variant is used.