Opened 8 years ago

Last modified 2 years ago

#2910 assigned Bug

Treat peers with same IP address but different port as different peers

Reported by: wereHamster Owned by: gvdl
Priority: Normal Milestone: Sometime
Component: libtransmission Version: 1.83
Severity: Minor Keywords: net patch-needed
Cc: tom@…, nkarthiks@…, taem@…, gvdl@…, nikoli@…

Description

Currently the code doesn't allow two different peers with the same IP address but different ports. For example getExistingAtom() in peer-mgr.c only checks the IP address but not the port.

This limitation makes it impossible to test file transfer between two Transmission peers on the same host.

Change History (20)

comment:1 Changed 8 years ago by charles

  • Milestone changed from None Set to 1.91
  • Severity changed from Normal to Minor
  • Status changed from new to assigned
  • Version changed from 1.83+ to 1.83

comment:2 Changed 8 years ago by charles

  • Milestone changed from 1.91 to Sometime

comment:3 in reply to: ↑ description Changed 8 years ago by stdisease

It would also make it impossible to communicate to more than 1 client from a NAT'ed network environment.

comment:4 Changed 8 years ago by charles

Do either of you want to cook up a patch for this?

comment:5 Changed 8 years ago by gkelly

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

It appears that this issue was resolved in r7231, as getExistingAtom and friends all use tr_compareAddresses, which performs a bytewise comparison of the in_addr (or in6_addr) structures associated with each address. Those structures contain port designations, so the peers are already being differentiated by their ports.

comment:6 Changed 8 years ago by livings124

I'm a bit confused why this was set to version 1.83 when r7231 was in 1.40. I wonder if this was an issue when it was opened.

comment:7 follow-up: Changed 8 years ago by nkarthiks

  • Cc nkarthiks@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

struct in_addr is defined as a single uint32_t (netinet/in.h) and hence holds only the 4 byte IP address of a node (and not its port). And hence, tr_address does not include the port. Also, getExistingAtom (peer-mgr.h) only takes tr_address as its argument, and loses out on the port when comparing peers. Again its comparison function tr_compareAddresses does not consider the port.

Two different peers behind a NAT will be ignored, and similarly 2 peers on the same machine. I am unsure of how to make an actual fix, but would be happy to help. (This would benefit my research.)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by wereHamster

Replying to nkarthiks:

I am unsure of how to make an actual fix, but would be happy to help. (This would benefit my research.)

Start by fixing the code and then attaching the patch to this ticket so we can review it. Before I created this ticket I started looking into a fix, and while it seemed easy, it turned out much more complicated once I saw how much code would need to be changed. Anyway, probably the easiest way would be to replace tr_address with sockaddr_storage where both the IP address and port is needed.

comment:9 Changed 7 years ago by taem

  • Cc taem@… added
  • Keywords patch-needed added

comment:10 in reply to: ↑ 8 Changed 7 years ago by ijuxda

Replying to wereHamster:

Replying to nkarthiks:

I am unsure of how to make an actual fix, but would be happy to help. (This would benefit my research.)

Start by fixing the code and then attaching the patch to this ticket so we can review it. Before I created this ticket I started looking into a fix, and while it seemed easy, it turned out much more complicated once I saw how much code would need to be changed. Anyway, probably the easiest way would be to replace tr_address with sockaddr_storage where both the IP address and port is needed.

On first glance instead of reverting to the sockaddr types for this purpose, I suggest adding a new type

typedef struct
{
    tr_address addr;
    tr_port port;
}
tr_endpoint;

and using that where you feel the port is significant. This could also be used to reduce the size of the argument list of many functions that always use the address + port combination.

comment:11 Changed 6 years ago by gvdl

I'd like to take over this bug. A nice big project sweeping but not complicated will be a brilliant way to get to know the code base. Has there been any movement in the last year or so. Is there a good reason not to implement ijuxda's suggestion of creating an tr_endpoint structure and using this for peer comparison?

comment:12 Changed 6 years ago by gvdl

Actually we can use the tr_pex structure for this, the modifications seems to be in and around the getExistingHandshake and the tr_compare_address function. This function is only used in 5 places:

libtransmission/peer-mgr.c|290| <<handshakeCompareToAddr>> return tr_address_compare( tr_handshakeGetAddr( a, NULL ), vb );
libtransmission/peer-mgr.c|313| <<comparePeerAtomToAddress>> return tr_address_compare( &a->addr, vb );
libtransmission/peer-mgr.c|348| <<peerCompare>> return tr_address_compare( tr_peerAddress( a ), tr_peerAddress( b ) );
libtransmission/peer-mgr.c|2254| <<tr_pexCompare>> if(( i = tr_address_compare( &a->addr, &b->addr )))
libtransmission/peer-mgr.c|3626| <<compareAtomPtrsByAddress>> return tr_address_compare( &a->addr, &b->addr );

[gvdl edited to reformat the uses of tr_address_compare in peer-mgr.c]

Last edited 6 years ago by gvdl (previous) (diff)

comment:13 Changed 6 years ago by gvdl

  • Owner changed from charles to gvdl
  • Status changed from reopened to new

comment:14 Changed 6 years ago by gvdl

Jordan: Before I start work on this I'd like your input. The change will be localised to tr_address_compare and that function only seems to be used in libtransmission. The easiest thing may be to change the internal address format to the pex format and then re-implement tr_pexCompare. Thoughts?

comment:15 Changed 6 years ago by gvdl

I'm going to need access to another server with a different IP address to test these changes. Are there any shared resources out there to use as a test bed?

comment:16 Changed 6 years ago by gvdl

  • Cc gvdl@… added

comment:17 follow-up: Changed 6 years ago by jordan

gvdl, off the top of my head I think we'd be better off localizing this change in peer-mgr.c. libtransmission uses tr_address in many places where ports aren't important.

Possibly there should be a private compare() function in peer-mgr.c that tests both the ports and the addresses (via tr_address_compare()).

comment:18 in reply to: ↑ 17 Changed 6 years ago by gvdl

  • Status changed from new to assigned

Replying to jordan:

Agreed, I'll start looking at it and I'll get you to review the patches.

comment:19 Changed 5 years ago by Nikoli

  • Cc nikoli@… added

comment:20 Changed 2 years ago by x190

@gvdl: any progress on this?

Note: See TracTickets for help on using tickets.