Sort bencoded dictionary in extension protocol handshake
Needs RevisionPublic

Authored by nalvarez on Apr 12 2020, 7:22 AM.

Details

Summary

The extension protocol handshake packet (BEP-0010) contains a dictionary with data like this:

{
  'm': {'ut_pex': 1, 'ut_metadata': 2},
  'p': 12345,
  'reqq': 250,
  'metadata_size': 12542,
  'upload_only': '0',
  'v': 'KTorrent'
}

The code that sends this packet (sendExtProtHandshake) is creating a bencoded dictionary with the keys in the order shown above. However, all bencoded dictionaries must have the keys *sorted*. For example ut_metadata *must* come before ut_pex.

This happens to work because BitTorrent clients are liberal in what they accept, but a client strictly implementing the spec would likely refuse KTorrent connections.

A known consequence is that RTorrent was unable to get .torrent files from KTorrent when given a magnet link (BEP-0009). Strangely, RTorrent doesn't like the unsorted dictionary when it's getting torrent metadata files and drops the connection after the handshake, yet it accepts the same handshake when doing a normal BitTorrent connection to exchange data.

This bug was introduced with the first part of the magnet implementation (ktorrent.git d57e8ed869, SVN 1052409, nov 2009).

This commit manually puts the dictionary entries in order, fixing the incompatibility with RTorrent.

Test Plan

I gave RTorrent a magnet URL for a torrent where KTorrent is the only other peer. I tested that without this patch, RTorrent drops the connection after the handshake, and with this patch, it gets the .torrent from KTorrent successfully, then reconnects and starts downloading the file contents.

Diff Detail

Repository
R472 KTorrent Library
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25096
Build 25114: arc lint + arc unit
nalvarez requested review of this revision.Apr 12 2020, 7:22 AM
nalvarez created this revision.
nalvarez edited the summary of this revision. (Show Details)Apr 12 2020, 7:23 AM
nalvarez edited the test plan for this revision. (Show Details)
nalvarez edited the summary of this revision. (Show Details)
svuorela accepted this revision.Apr 12 2020, 8:07 AM
svuorela added a subscriber: svuorela.

Though maybe give the maintainer a day or two to react as well.

This revision is now accepted and ready to land.Apr 12 2020, 8:07 AM
ouwerkerk added a subscriber: ouwerkerk.EditedApr 12 2020, 9:23 AM

General comment: your patch might fix this, but there is no source code comment explaining things *must* be added sorted on key order and the code still does not enforce this invariant either. (Which makes a comment about it all the more important.)

Ideally there would also be an autotest :)

src/peer/peer.cpp
745

Coding style: the old code also has this issue but if you're changing things you might as well make it prettier by having only one statement per line. This also applies to the line above it, as well.

ouwerkerk requested changes to this revision.Apr 12 2020, 10:58 AM
This revision now requires changes to proceed.Apr 12 2020, 10:58 AM