From ddd5c4faee3ab1c4fc680a576acff5fc5679f8e5 Mon Sep 17 00:00:00 2001 From: Juan Batiz-Benet Date: Mon, 12 Jan 2015 20:17:54 -0800 Subject: [PATCH 1/3] p2p/net/swarm: fix connect self problems This adds two checks after a successful conn.Dial * if the remote peer is not who we wanted, close conn * if the remove peer is outselves, close conn (the second is redundant, but the codebase may evolve to end up disabling the first check, so keeping the second in place helps) note: Loopback addresses are actually sent out (they _have to be_, in cases where there are >1 node in the same machine), so many times when trying connections, nodes end up dialing themselves. --- p2p/net/swarm/swarm_dial.go | 50 ++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/p2p/net/swarm/swarm_dial.go b/p2p/net/swarm/swarm_dial.go index d17d31c8d..041e8a0db 100644 --- a/p2p/net/swarm/swarm_dial.go +++ b/p2p/net/swarm/swarm_dial.go @@ -10,6 +10,7 @@ import ( lgbl "github.com/jbenet/go-ipfs/util/eventlog/loggables" context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context" + ma "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-multiaddr" ) // Dial connects to a peer. @@ -56,23 +57,11 @@ func (s *Swarm) Dial(ctx context.Context, p peer.ID) (*Conn, error) { PrivateKey: sk, } - // try to connect to one of the peer's known addresses. - // for simplicity, we do this sequentially. - // A future commit will do this asynchronously. - var connC conn.Conn - var err error - for _, addr := range remoteAddrs { - connC, err = d.Dial(ctx, addr, p) - if err == nil { - break - } - } + // try to get a connection to any addr + connC, err := s.dialAddrs(ctx, d, p, remoteAddrs) if err != nil { return nil, err } - if connC == nil { - err = fmt.Errorf("failed to dial %s", p) - } // ok try to setup the new connection. swarmC, err := dialConnSetup(ctx, s, connC) @@ -87,6 +76,39 @@ func (s *Swarm) Dial(ctx context.Context, p peer.ID) (*Conn, error) { return swarmC, nil } +func (s *Swarm) dialAddrs(ctx context.Context, d *conn.Dialer, p peer.ID, remoteAddrs []ma.Multiaddr) (conn.Conn, error) { + + // try to connect to one of the peer's known addresses. + // for simplicity, we do this sequentially. + // A future commit will do this asynchronously. + for _, addr := range remoteAddrs { + connC, err := d.Dial(ctx, addr, p) + if err != nil { + continue + } + + // if the connection is not to whom we thought it would be... + if connC.RemotePeer() != p { + log.Infof("misdial to %s through %s (got %s)", p, addr, connC.RemoteMultiaddr()) + connC.Close() + continue + } + + // if the connection is to ourselves... + // this can happen TONS when Loopback addrs are advertized. + // (this should be caught by two checks above, but let's just make sure.) + if connC.RemotePeer() == s.local { + log.Infof("misdial to %s through %s", p, addr) + connC.Close() + continue + } + + // success! we got one! + return connC, nil + } + return nil, fmt.Errorf("failed to dial %s", p) +} + // dialConnSetup is the setup logic for a connection from the dial side. it // needs to add the Conn to the StreamSwarm, then run newConnSetup func dialConnSetup(ctx context.Context, s *Swarm, connC conn.Conn) (*Conn, error) { From 7aa4a83f2e100da7029124fa55321eee4c632809 Mon Sep 17 00:00:00 2001 From: Juan Batiz-Benet Date: Mon, 12 Jan 2015 20:49:06 -0800 Subject: [PATCH 2/3] addr: proper filter + subtract --- core/core.go | 2 +- p2p/net/swarm/addr/addr.go | 36 +++++++++++++++++++--------- p2p/net/swarm/addr/addr_test.go | 40 +++++++++++++++++++++++++++++--- p2p/net/swarm/swarm.go | 2 +- p2p/net/swarm/swarm_addr_test.go | 6 ++--- p2p/net/swarm/swarm_dial.go | 2 +- 6 files changed, 68 insertions(+), 20 deletions(-) diff --git a/core/core.go b/core/core.go index be8f8e643..0a008c83c 100644 --- a/core/core.go +++ b/core/core.go @@ -359,7 +359,7 @@ func constructPeerHost(ctx context.Context, ctxg ctxgroup.ContextGroup, cfg *con // make sure we error out if our config does not have addresses we can use log.Debugf("Config.Addresses.Swarm:%s", listenAddrs) - filteredAddrs := addrutil.FilterAddrs(listenAddrs) + filteredAddrs := addrutil.FilterUsableAddrs(listenAddrs) log.Debugf("Config.Addresses.Swarm:%s (filtered)", listenAddrs) if len(filteredAddrs) < 1 { return nil, debugerror.Errorf("addresses in config not usable: %s", listenAddrs) diff --git a/p2p/net/swarm/addr/addr.go b/p2p/net/swarm/addr/addr.go index 91e8febf4..bae6e8605 100644 --- a/p2p/net/swarm/addr/addr.go +++ b/p2p/net/swarm/addr/addr.go @@ -41,19 +41,27 @@ func init() { SupportedTransportProtocols = transports } -// FilterAddrs is a filter that removes certain addresses -// from a list. the addresses removed are those known NOT -// to work with our network. Namely, addresses with UTP. -func FilterAddrs(a []ma.Multiaddr) []ma.Multiaddr { +// FilterAddrs is a filter that removes certain addresses, according to filter. +// if filter returns true, the address is kept. +func FilterAddrs(a []ma.Multiaddr, filter func(ma.Multiaddr) bool) []ma.Multiaddr { b := make([]ma.Multiaddr, 0, len(a)) for _, addr := range a { - if AddrUsable(addr, false) { + if filter(addr) { b = append(b, addr) } } return b } +// FilterUsableAddrs removes certain addresses +// from a list. the addresses removed are those known NOT +// to work with our network. Namely, addresses with UTP. +func FilterUsableAddrs(a []ma.Multiaddr) []ma.Multiaddr { + return FilterAddrs(a, func(m ma.Multiaddr) bool { + return AddrUsable(m, false) + }) +} + // AddrOverNonLocalIP returns whether the addr uses a non-local ip link func AddrOverNonLocalIP(a ma.Multiaddr) bool { split := ma.Split(a) @@ -228,13 +236,19 @@ func AddrIsShareableOnWAN(addr ma.Multiaddr) bool { // WANShareableAddrs filters addresses based on whether they're shareable on WAN func WANShareableAddrs(inp []ma.Multiaddr) []ma.Multiaddr { - out := make([]ma.Multiaddr, 0, len(inp)) - for _, a := range inp { - if AddrIsShareableOnWAN(a) { - out = append(out, a) + return FilterAddrs(inp, AddrIsShareableOnWAN) +} + +// Subtract filters out all addrs in b from a +func Subtract(a, b []ma.Multiaddr) []ma.Multiaddr { + return FilterAddrs(a, func(m ma.Multiaddr) bool { + for _, bb := range b { + if m.Equal(bb) { + return false + } } - } - return out + return true + }) } // CheckNATWarning checks if our observed addresses differ. if so, diff --git a/p2p/net/swarm/addr/addr_test.go b/p2p/net/swarm/addr/addr_test.go index 7f355ef5e..13e2028cf 100644 --- a/p2p/net/swarm/addr/addr_test.go +++ b/p2p/net/swarm/addr/addr_test.go @@ -53,9 +53,9 @@ func TestFilterAddrs(t *testing.T) { } } - subtestAddrsEqual(t, FilterAddrs(bad), []ma.Multiaddr{}) - subtestAddrsEqual(t, FilterAddrs(good), good) - subtestAddrsEqual(t, FilterAddrs(goodAndBad), good) + subtestAddrsEqual(t, FilterUsableAddrs(bad), []ma.Multiaddr{}) + subtestAddrsEqual(t, FilterUsableAddrs(good), good) + subtestAddrsEqual(t, FilterUsableAddrs(goodAndBad), good) } func subtestAddrsEqual(t *testing.T, a, b []ma.Multiaddr) { @@ -196,3 +196,37 @@ func TestWANShareable(t *testing.T) { t.Error("should be zero") } } + +func TestSubtract(t *testing.T) { + + a := []ma.Multiaddr{ + newMultiaddr(t, "/ip4/127.0.0.1/tcp/1234"), + newMultiaddr(t, "/ip4/0.0.0.0/tcp/1234"), + newMultiaddr(t, "/ip6/::1/tcp/1234"), + newMultiaddr(t, "/ip6/::/tcp/1234"), + newMultiaddr(t, "/ip6/fe80::1/tcp/1234"), + newMultiaddr(t, "/ip6/fe80::/tcp/1234"), + } + + b := []ma.Multiaddr{ + newMultiaddr(t, "/ip4/127.0.0.1/tcp/1234"), + newMultiaddr(t, "/ip6/::1/tcp/1234"), + newMultiaddr(t, "/ip6/fe80::1/tcp/1234"), + } + + c1 := []ma.Multiaddr{ + newMultiaddr(t, "/ip4/0.0.0.0/tcp/1234"), + newMultiaddr(t, "/ip6/::/tcp/1234"), + newMultiaddr(t, "/ip6/fe80::/tcp/1234"), + } + + c2 := Subtract(a, b) + if len(c1) != len(c2) { + t.Error("should be the same") + } + for i, ca := range c1 { + if !c2[i].Equal(ca) { + t.Error("should be the same", ca, c2[i]) + } + } +} diff --git a/p2p/net/swarm/swarm.go b/p2p/net/swarm/swarm.go index 54d53f8d9..8080fcde9 100644 --- a/p2p/net/swarm/swarm.go +++ b/p2p/net/swarm/swarm.go @@ -41,7 +41,7 @@ func NewSwarm(ctx context.Context, listenAddrs []ma.Multiaddr, local peer.ID, peers peer.Peerstore) (*Swarm, error) { if len(listenAddrs) > 0 { - filtered := addrutil.FilterAddrs(listenAddrs) + filtered := addrutil.FilterUsableAddrs(listenAddrs) if len(filtered) < 1 { return nil, fmt.Errorf("swarm cannot use any addr in: %s", listenAddrs) } diff --git a/p2p/net/swarm/swarm_addr_test.go b/p2p/net/swarm/swarm_addr_test.go index 65e196efa..6b0988d81 100644 --- a/p2p/net/swarm/swarm_addr_test.go +++ b/p2p/net/swarm/swarm_addr_test.go @@ -51,9 +51,9 @@ func TestFilterAddrs(t *testing.T) { } } - subtestAddrsEqual(t, addrutil.FilterAddrs(bad), []ma.Multiaddr{}) - subtestAddrsEqual(t, addrutil.FilterAddrs(good), good) - subtestAddrsEqual(t, addrutil.FilterAddrs(goodAndBad), good) + subtestAddrsEqual(t, addrutil.FilterUsableAddrs(bad), []ma.Multiaddr{}) + subtestAddrsEqual(t, addrutil.FilterUsableAddrs(good), good) + subtestAddrsEqual(t, addrutil.FilterUsableAddrs(goodAndBad), good) // now test it with swarm diff --git a/p2p/net/swarm/swarm_dial.go b/p2p/net/swarm/swarm_dial.go index 041e8a0db..655d7783a 100644 --- a/p2p/net/swarm/swarm_dial.go +++ b/p2p/net/swarm/swarm_dial.go @@ -41,7 +41,7 @@ func (s *Swarm) Dial(ctx context.Context, p peer.ID) (*Conn, error) { remoteAddrs := s.peers.Addresses(p) // make sure we can use the addresses. - remoteAddrs = addrutil.FilterAddrs(remoteAddrs) + remoteAddrs = addrutil.FilterUsableAddrs(remoteAddrs) if len(remoteAddrs) == 0 { return nil, errors.New("peer has no addresses") } From 9b4d42f7b065ad2a4a44867be3ab210159209bd9 Mon Sep 17 00:00:00 2001 From: Juan Batiz-Benet Date: Mon, 12 Jan 2015 20:51:18 -0800 Subject: [PATCH 3/3] p2p/net/swarm: dial - filter out own addrs --- p2p/net/swarm/swarm_dial.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/p2p/net/swarm/swarm_dial.go b/p2p/net/swarm/swarm_dial.go index 655d7783a..96581009e 100644 --- a/p2p/net/swarm/swarm_dial.go +++ b/p2p/net/swarm/swarm_dial.go @@ -39,17 +39,23 @@ func (s *Swarm) Dial(ctx context.Context, p peer.ID) (*Conn, error) { log.Warning("Dial not given PrivateKey, so WILL NOT SECURE conn.") } - remoteAddrs := s.peers.Addresses(p) - // make sure we can use the addresses. - remoteAddrs = addrutil.FilterUsableAddrs(remoteAddrs) - if len(remoteAddrs) == 0 { - return nil, errors.New("peer has no addresses") - } + // get our own addrs localAddrs := s.peers.Addresses(s.local) if len(localAddrs) == 0 { log.Debug("Dialing out with no local addresses.") } + // get remote peer addrs + remoteAddrs := s.peers.Addresses(p) + // make sure we can use the addresses. + remoteAddrs = addrutil.FilterUsableAddrs(remoteAddrs) + // drop out any addrs that would just dial ourselves. use ListenAddresses + // as that is a more authoritative view than localAddrs. + remoteAddrs = addrutil.Subtract(remoteAddrs, s.ListenAddresses()) + if len(remoteAddrs) == 0 { + return nil, errors.New("peer has no addresses") + } + // open connection to peer d := &conn.Dialer{ LocalPeer: s.local,