Fix skademlia ID address race condition.

skademlia/client.go

@@ -144,7 +144,7 @@ func (c *Client) ClosestPeers(opts ...DialOption) []*grpc.ClientConn {
 	var conns []*grpc.ClientConn
 
 	for i := range ids {
-		if conn, err := c.Dial(ids[i].address, opts...); err == nil {
+		if conn, err := c.Dial(ids[i].Address(), opts...); err == nil {
 			conns = append(conns, conn)
 		}
 	}
@@ -224,7 +224,7 @@ func (c *Client) getPeerID(conn *grpc.ClientConn, timeout time.Duration) (*ID, e
 }
 
 func (c *Client) DialContext(ctx context.Context, addr string) (*grpc.ClientConn, error) {
-	if addr == c.table.self.address {
+	if addr == c.table.self.Address() {
 		return nil, errors.New("attempted to dial self")
 	}
 
@@ -393,10 +393,8 @@ func (c *Client) Bootstrap() (results []*ID) {
 }
 
 func (c *Client) FindNode(target *ID, k int, a int, d int) (results []*ID) {
-	type request ID
-
 	type response struct {
-		requester *request
+		requester *ID
 		ids       []*ID
 	}
 
@@ -419,14 +417,14 @@ func (c *Client) FindNode(target *ID, k int, a int, d int) (results []*ID) {
 
 	for _, lookup := range lookups { // Perform d parallel disjoint lookups.
 		go func(lookup queue.Queue) {
-			requests := make(chan *request, a)
+			requests := make(chan *ID, a)
 			responses := make(chan *response, a)
 
 			for i := 0; i < a; i++ { // Perform α queries in parallel per disjoint lookup.
 				go func() {
 					for id := range requests {
 						f := func() error {
-							conn, err := c.Dial(id.address, WithTimeout(3*time.Second))
+							conn, err := c.Dial(id.Address(), WithTimeout(3*time.Second))
 
 							if err != nil {
 								responses <- nil
@@ -470,7 +468,7 @@ func (c *Client) FindNode(target *ID, k int, a int, d int) (results []*ID) {
 
 			for lookup.Len() > 0 || pending > 0 {
 				for lookup.Len() > 0 && len(requests) < cap(requests) {
-					requests <- (*request)(lookup.PopFront().(*ID))
+					requests <- lookup.PopFront().(*ID)
 					pending++
 				}
 

skademlia/identity.go

@@ -26,12 +26,15 @@ import (
 	"github.com/pkg/errors"
 	"golang.org/x/crypto/blake2b"
 	"io"
+	"sync"
 )
 
 type ID struct {
 	address   string
 	publicKey edwards25519.PublicKey
 
+	sync.RWMutex
+
 	id, checksum, nonce [blake2b.Size256]byte
 }
 
@@ -49,8 +52,11 @@ func NewID(address string, publicKey edwards25519.PublicKey, nonce [blake2b.Size
 	}
 }
 
-func (m ID) Address() string {
-	return m.address
+func (m *ID) Address() string {
+	m.RLock()
+	t := m.address
+	m.RUnlock()
+	return t
 }
 
 func (m ID) PublicKey() edwards25519.PublicKey {
@@ -66,16 +72,24 @@ func (m ID) Nonce() [blake2b.Size256]byte {
 }
 
 func (m ID) String() string {
-	return fmt.Sprintf("%s[%x]", m.address, m.publicKey)
+	return fmt.Sprintf("%s[%x]", (&m).Address(), m.publicKey)
+}
+
+func (m *ID) SetAddress(address string) {
+	m.Lock()
+	m.address = address
+	m.Unlock()
 }
 
 func (m ID) Marshal() []byte {
-	buf := make([]byte, 2+len(m.address)+edwards25519.SizePublicKey+blake2b.Size256)
+	address := (&m).Address()
+
+	buf := make([]byte, 2+len(address)+edwards25519.SizePublicKey+blake2b.Size256)
 
-	binary.BigEndian.PutUint16(buf[0:2], uint16(len(m.address)))
-	copy(buf[2:2+len(m.address)], m.address)
-	copy(buf[2+len(m.address):2+len(m.address)+edwards25519.SizePublicKey], m.publicKey[:])
-	copy(buf[2+len(m.address)+edwards25519.SizePublicKey:2+len(m.address)+edwards25519.SizePublicKey+blake2b.Size256], m.nonce[:])
+	binary.BigEndian.PutUint16(buf[0:2], uint16(len(address)))
+	copy(buf[2:2+len(address)], address)
+	copy(buf[2+len(address):2+len(address)+edwards25519.SizePublicKey], m.publicKey[:])
+	copy(buf[2+len(address)+edwards25519.SizePublicKey:2+len(address)+edwards25519.SizePublicKey+blake2b.Size256], m.nonce[:])
 
 	return buf
 }

skademlia/protocol.go

@@ -203,7 +203,7 @@ func (p Protocol) Server(info noise.Info, conn net.Conn) (net.Conn, error) {
 	p.client.logger.Printf("Client %s has connected to you", id)
 
 	go func() {
-		if _, err = p.client.Dial(id.address, WithTimeout(3*time.Second)); err != nil {
+		if _, err = p.client.Dial(id.Address(), WithTimeout(3*time.Second)); err != nil {
 			p.client.logger.Printf("Client %s was not able to be dialed back, closing connection", id)
 			_ = conn.Close()
 		} else {

skademlia/table.go

@@ -111,8 +111,7 @@ func (t *Table) Update(target *ID) error {
 		b.Lock()
 
 		// address might differ for same public key (checksum
-		id := found.Value.(*ID)
-		id.address = target.address
+		found.Value.(*ID).SetAddress(target.address)
 
 		b.MoveToFront(found)
 		b.Unlock()

skademlia/table_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/perlin-network/noise/edwards25519"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/crypto/blake2b"
+	"sync"
 	"testing"
 	"testing/quick"
 )
@@ -172,16 +173,25 @@ func TestUpdateSamePublicKey(t *testing.T) {
 	// we create new id with same public key but different address
 	addressToChange := "127.0.0.3"
 	updatedCopy := NewID(addressToChange, updated.publicKey, updated.nonce)
-	if !assert.NoError(t, table.Update(updatedCopy)) {
-		return
-	}
+
+	wg := sync.WaitGroup{}
+	// ensure that updating table (and id) safe to be done concurrently
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		if !assert.NoError(t, table.Update(updatedCopy)) {
+			return
+		}
+	}()
 
 	found := table.FindClosest(rootID, 10)
 	if !assert.Equal(t, 1, len(found)) {
 		return
 	}
 
+	wg.Wait()
+
 	// we expect id in the table to have same public key but updated address
 	assert.Equal(t, updated.publicKey, found[0].publicKey)
-	assert.Equal(t, addressToChange, found[0].address)
+	assert.Equal(t, addressToChange, found[0].Address())
 }