Opened 10 years ago

Last modified 5 years ago

#4078 reopened Bug

Better calculation of the bitfield length while still a magnet link

Reported by: jordan Owned by: jordan
Priority: Normal Milestone: 2.30
Component: libtransmission Version: 2.20
Severity: Minor Keywords:
Cc:

Description

The messageLengthIsCorrect() function in peer-msgs is used to determine if an incoming message from a peer is the correct length. If it's not, the message is discarded and we disconnect from the peer.

In the case of a bitfield message (BT message 5), it's possible that we won't know the message's correct length if the torrent is a magnet link that hasn't pulled down its info dict yet.

Transmission 2.21 handles this by disconnecting from peers that send bitfields while we're a magnet link, relying only on seeds that send a HAVE_ALL message instead of a bitfield. However, this has obvious weaknesses for swarms with few seeds.

In this specialized case of magnet links getting a bitfield message, we should be much more permissive about what we allow.

Change History (4)

comment:1 Changed 10 years ago by jordan

  • Status changed from new to assigned

comment:2 Changed 10 years ago by jordan

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

Fixed in trunk by r12065

comment:3 Changed 10 years ago by User294

Wow, it looks like you've fixed bug I suspected for a months!

I've noticed a while ago that T could be sometimes very slow and/or unreliable in getting metadata for magnets and disregards most of peers while actually finds them but no connection happens. However I was not able to find a real reason of this strange behavior because most of BT communications are encrypted (so sniffer isn't very useful) and I can't say that T haves a great packet-level protocol trace/debug facilities. Actually, verbose/debug logging in T isn't great at all. Can you tell me how you've caught this? Me attempted several times to understand what's wrong but failed.

I would be really happy to test this again :) (when issues with assertions fixed :P)

comment:4 Changed 5 years ago by cfpp2p

  • Resolution fixed deleted
  • Status changed from closed to reopened

With current trunk peer-msgs.c 14666 messageLengthIsCorrect() we have, beginning at line 1393

        case BT_BITFIELD:
            if (tr_torrentHasMetadata (msg->torrent))
                return len == (msg->torrent->info.pieceCount >> 3) + (msg->torrent->info.pieceCount & 7 ? 1 : 0) + 1u;
            /* we don't know the piece count yet,
               so we can only guess whether to send true or false */
            if (msg->metadata_size_hint > 0)
                return len <= msg->metadata_size_hint;
            return true;

r12065 incorporated the check of metadata_size_hint. metadata_size_hint is ultimately derived from metadata_size. As discussed in ticket:6048#comment:2 ..."I don't think there really is a way to know which one, if any at all, of metadata_size and total_size is valid"...
The lines

            if (msg->metadata_size_hint > 0)
                return len <= msg->metadata_size_hint;

should be eliminated and always return true; when there is no metadata yet. The guess seems incorrect in that it erroneously assumes metadata_size is always more likely to be correct than len.

Note: See TracTickets for help on using tickets.