Merge pull request #548 from jbenet/fix-connect-self

p2p/net/swarm: fix connect self problems

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)

p2p/net/swarm/addr/addr.go

@@ -41,19 +41,27 @@ func init() {
 	SupportedTransportProtocols = transports
 }
 
-// FilterAddrs is a filter that removes certain addresses
+// FilterAddrs is a filter that removes certain addresses, according to filter.
-// from a list. the addresses removed are those known NOT
+// if filter returns true, the address is kept.
-// to work with our network. Namely, addresses with UTP.
+func FilterAddrs(a []ma.Multiaddr, filter func(ma.Multiaddr) bool) []ma.Multiaddr {
-func FilterAddrs(a []ma.Multiaddr) []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))
+	return FilterAddrs(inp, AddrIsShareableOnWAN)
-	for _, a := range inp {
+}
-		if AddrIsShareableOnWAN(a) {
+
-			out = append(out, a)
+// 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 true
-	return out
+	})
 }
 
 // CheckNATWarning checks if our observed addresses differ. if so,

p2p/net/swarm/addr/addr_test.go

@@ -53,9 +53,9 @@ func TestFilterAddrs(t *testing.T) {
 		}
 	}
 
-	subtestAddrsEqual(t, FilterAddrs(bad), []ma.Multiaddr{})
+	subtestAddrsEqual(t, FilterUsableAddrs(bad), []ma.Multiaddr{})
-	subtestAddrsEqual(t, FilterAddrs(good), good)
+	subtestAddrsEqual(t, FilterUsableAddrs(good), good)
-	subtestAddrsEqual(t, FilterAddrs(goodAndBad), 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])
+		}
+	}
+}

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)
 		}

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.FilterUsableAddrs(bad), []ma.Multiaddr{})
-	subtestAddrsEqual(t, addrutil.FilterAddrs(good), good)
+	subtestAddrsEqual(t, addrutil.FilterUsableAddrs(good), good)
-	subtestAddrsEqual(t, addrutil.FilterAddrs(goodAndBad), good)
+	subtestAddrsEqual(t, addrutil.FilterUsableAddrs(goodAndBad), good)
 
 	// now test it with swarm
 

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.
@@ -38,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)
+	// get our own addrs
-	// make sure we can use the addresses.
-	remoteAddrs = addrutil.FilterAddrs(remoteAddrs)
-	if len(remoteAddrs) == 0 {
-		return nil, errors.New("peer has no addresses")
-	}
 	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,
@@ -56,23 +63,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.
+	// try to get a connection to any addr
-	// for simplicity, we do this sequentially.
+	connC, err := s.dialAddrs(ctx, d, p, remoteAddrs)
-	// 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
-		}
-	}
 	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 +82,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) {