[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [snowflake/main] Store net.Addr in clientIDAddrMap



commit 6634f2bec9ea7c538a819619d003db4ea2947a60
Author: Cecylia Bocovich <cohosh@xxxxxxxxxxxxxx>
Date:   Sat Jun 19 11:16:38 2021 -0400

    Store net.Addr in clientIDAddrMap
    
    This fixes a stats collection bug where we were converting client
    addresses between a string and net.Addr using the clientAddr function
    multiple times, resulting in an empty string for all addresses.
---
 server/lib/http.go             |  2 +-
 server/lib/snowflake.go        |  2 +-
 server/lib/turbotunnel.go      | 15 ++++----
 server/lib/turbotunnel_test.go | 84 +++++++++++++++++++++++-------------------
 4 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/server/lib/http.go b/server/lib/http.go
index c612422..3dff45c 100644
--- a/server/lib/http.go
+++ b/server/lib/http.go
@@ -138,7 +138,7 @@ func turbotunnelMode(conn net.Conn, addr net.Addr, pconn *turbotunnel.QueuePacke
 	// recent WebSocket connection that has had to do with a session, at the
 	// time the session is established, is the IP address that should be
 	// credited for the entire KCP session.
-	clientIDAddrMap.Set(clientID, addr.String())
+	clientIDAddrMap.Set(clientID, addr)
 
 	var wg sync.WaitGroup
 	wg.Add(2)
diff --git a/server/lib/snowflake.go b/server/lib/snowflake.go
index 319acd8..48c6d9e 100644
--- a/server/lib/snowflake.go
+++ b/server/lib/snowflake.go
@@ -181,7 +181,7 @@ func (l *SnowflakeListener) acceptStreams(conn *kcp.UDPSession) error {
 			}
 			return err
 		}
-		l.QueueConn(&SnowflakeClientConn{Conn: stream, address: clientAddr(addr)})
+		l.QueueConn(&SnowflakeClientConn{Conn: stream, address: addr})
 	}
 }
 
diff --git a/server/lib/turbotunnel.go b/server/lib/turbotunnel.go
index bb16fa3..741992d 100644
--- a/server/lib/turbotunnel.go
+++ b/server/lib/turbotunnel.go
@@ -1,12 +1,13 @@
 package lib
 
 import (
+	"net"
 	"sync"
 
 	"git.torproject.org/pluggable-transports/snowflake.git/common/turbotunnel"
 )
 
-// clientIDMap is a fixed-capacity mapping from ClientIDs to address strings.
+// clientIDMap is a fixed-capacity mapping from ClientIDs to a net.Addr.
 // Adding a new entry using the Set method causes the oldest existing entry to
 // be forgotten.
 //
@@ -23,7 +24,7 @@ type clientIDMap struct {
 	// entries is a circular buffer of (ClientID, addr) pairs.
 	entries []struct {
 		clientID turbotunnel.ClientID
-		addr     string
+		addr     net.Addr
 	}
 	// oldest is the index of the oldest member of the entries buffer, the
 	// one that will be overwritten at the next call to Set.
@@ -38,7 +39,7 @@ func newClientIDMap(capacity int) *clientIDMap {
 	return &clientIDMap{
 		entries: make([]struct {
 			clientID turbotunnel.ClientID
-			addr     string
+			addr     net.Addr
 		}, capacity),
 		oldest:  0,
 		current: make(map[turbotunnel.ClientID]int),
@@ -48,7 +49,7 @@ func newClientIDMap(capacity int) *clientIDMap {
 // Set adds a mapping from clientID to addr, replacing any previous mapping for
 // clientID. It may also cause the clientIDMap to forget at most one other
 // mapping, the oldest one.
-func (m *clientIDMap) Set(clientID turbotunnel.ClientID, addr string) {
+func (m *clientIDMap) Set(clientID turbotunnel.ClientID, addr net.Addr) {
 	m.lock.Lock()
 	defer m.lock.Unlock()
 	if len(m.entries) == 0 {
@@ -73,13 +74,13 @@ func (m *clientIDMap) Set(clientID turbotunnel.ClientID, addr string) {
 
 // Get returns a previously stored mapping. The second return value indicates
 // whether clientID was actually present in the map. If it is false, then the
-// returned address string will be "".
-func (m *clientIDMap) Get(clientID turbotunnel.ClientID) (string, bool) {
+// returned address will be nil.
+func (m *clientIDMap) Get(clientID turbotunnel.ClientID) (net.Addr, bool) {
 	m.lock.Lock()
 	defer m.lock.Unlock()
 	if i, ok := m.current[clientID]; ok {
 		return m.entries[i].addr, true
 	} else {
-		return "", false
+		return nil, false
 	}
 }
diff --git a/server/lib/turbotunnel_test.go b/server/lib/turbotunnel_test.go
index ba4cf60..85404af 100644
--- a/server/lib/turbotunnel_test.go
+++ b/server/lib/turbotunnel_test.go
@@ -2,6 +2,7 @@ package lib
 
 import (
 	"encoding/binary"
+	"net"
 	"testing"
 
 	"git.torproject.org/pluggable-transports/snowflake.git/common/turbotunnel"
@@ -19,7 +20,7 @@ func TestClientIDMap(t *testing.T) {
 	expectGet := func(m *clientIDMap, clientID turbotunnel.ClientID, expectedAddr string, expectedOK bool) {
 		t.Helper()
 		addr, ok := m.Get(clientID)
-		if addr != expectedAddr || ok != expectedOK {
+		if (ok && addr.String() != expectedAddr) || ok != expectedOK {
 			t.Errorf("expected (%+q, %v), got (%+q, %v)", expectedAddr, expectedOK, addr, ok)
 		}
 	}
@@ -32,6 +33,15 @@ func TestClientIDMap(t *testing.T) {
 		}
 	}
 
+	// Convert a string to a net.Addr
+	ip := func(addr string) net.Addr {
+		ret, err := net.ResolveIPAddr("ip", addr)
+		if err != nil {
+			t.Errorf("received error: %s", err.Error())
+		}
+		return ret
+	}
+
 	// Zero-capacity map can't remember anything.
 	{
 		m := newClientIDMap(0)
@@ -39,12 +49,12 @@ func TestClientIDMap(t *testing.T) {
 		expectGet(m, id(0), "", false)
 		expectGet(m, id(1234), "", false)
 
-		m.Set(id(0), "A")
+		m.Set(id(0), ip("1.1.1.1"))
 		expectSize(m, 0)
 		expectGet(m, id(0), "", false)
 		expectGet(m, id(1234), "", false)
 
-		m.Set(id(1234), "A")
+		m.Set(id(1234), ip("1.1.1.1"))
 		expectSize(m, 0)
 		expectGet(m, id(0), "", false)
 		expectGet(m, id(1234), "", false)
@@ -56,60 +66,60 @@ func TestClientIDMap(t *testing.T) {
 		expectGet(m, id(0), "", false)
 		expectGet(m, id(1), "", false)
 
-		m.Set(id(0), "A")
+		m.Set(id(0), ip("1.1.1.1"))
 		expectSize(m, 1)
-		expectGet(m, id(0), "A", true)
+		expectGet(m, id(0), "1.1.1.1", true)
 		expectGet(m, id(1), "", false)
 
-		m.Set(id(1), "B") // forgets the (0, "A") entry
+		m.Set(id(1), ip("1.1.1.2")) // forgets the (0, "1.1.1.1") entry
 		expectSize(m, 1)
 		expectGet(m, id(0), "", false)
-		expectGet(m, id(1), "B", true)
+		expectGet(m, id(1), "1.1.1.2", true)
 
-		m.Set(id(1), "C") // forgets the (1, "B") entry
+		m.Set(id(1), ip("1.1.1.3")) // forgets the (1, "1.1.1.2") entry
 		expectSize(m, 1)
 		expectGet(m, id(0), "", false)
-		expectGet(m, id(1), "C", true)
+		expectGet(m, id(1), "1.1.1.3", true)
 	}
 
 	{
 		m := newClientIDMap(5)
-		m.Set(id(0), "A")
-		m.Set(id(1), "B")
-		m.Set(id(2), "C")
-		m.Set(id(0), "D") // shadows the (0, "D") entry
-		m.Set(id(3), "E")
+		m.Set(id(0), ip("1.1.1.1"))
+		m.Set(id(1), ip("1.1.1.2"))
+		m.Set(id(2), ip("1.1.1.3"))
+		m.Set(id(0), ip("1.1.1.4")) // shadows the (0, "1.1.1.1") entry
+		m.Set(id(3), ip("1.1.1.5"))
 		expectSize(m, 4)
-		expectGet(m, id(0), "D", true)
-		expectGet(m, id(1), "B", true)
-		expectGet(m, id(2), "C", true)
-		expectGet(m, id(3), "E", true)
+		expectGet(m, id(0), "1.1.1.4", true)
+		expectGet(m, id(1), "1.1.1.2", true)
+		expectGet(m, id(2), "1.1.1.3", true)
+		expectGet(m, id(3), "1.1.1.5", true)
 		expectGet(m, id(4), "", false)
 
-		m.Set(id(4), "F") // forgets the (0, "A") entry but should preserve (0, "D")
+		m.Set(id(4), ip("1.1.1.6")) // forgets the (0, "1.1.1.1") entry but should preserve (0, "1.1.1.4")
 		expectSize(m, 5)
-		expectGet(m, id(0), "D", true)
-		expectGet(m, id(1), "B", true)
-		expectGet(m, id(2), "C", true)
-		expectGet(m, id(3), "E", true)
-		expectGet(m, id(4), "F", true)
-
-		m.Set(id(5), "G") // forgets the (1, "B") entry
-		m.Set(id(0), "H") // forgets the (2, "C") entry and shadows (0, "D")
+		expectGet(m, id(0), "1.1.1.4", true)
+		expectGet(m, id(1), "1.1.1.2", true)
+		expectGet(m, id(2), "1.1.1.3", true)
+		expectGet(m, id(3), "1.1.1.5", true)
+		expectGet(m, id(4), "1.1.1.6", true)
+
+		m.Set(id(5), ip("1.1.1.7")) // forgets the (1, "1.1.1.2") entry
+		m.Set(id(0), ip("1.1.1.8")) // forgets the (2, "1.1.1.3") entry and shadows (0, "1.1.1.4")
 		expectSize(m, 4)
-		expectGet(m, id(0), "H", true)
+		expectGet(m, id(0), "1.1.1.8", true)
 		expectGet(m, id(1), "", false)
 		expectGet(m, id(2), "", false)
-		expectGet(m, id(3), "E", true)
-		expectGet(m, id(4), "F", true)
-		expectGet(m, id(5), "G", true)
-
-		m.Set(id(0), "I") // forgets the (0, "D") entry and shadows (0, "H")
-		m.Set(id(0), "J") // forgets the (3, "E") entry and shadows (0, "I")
-		m.Set(id(0), "K") // forgets the (4, "F") entry and shadows (0, "J")
-		m.Set(id(0), "L") // forgets the (5, "G") entry and shadows (0, "K")
+		expectGet(m, id(3), "1.1.1.5", true)
+		expectGet(m, id(4), "1.1.1.6", true)
+		expectGet(m, id(5), "1.1.1.7", true)
+
+		m.Set(id(0), ip("1.1.1.9"))  // forgets the (0, "1.1.1.4") entry and shadows (0, "1.1.1.8")
+		m.Set(id(0), ip("1.1.1.10")) // forgets the (3, "1.1.1.5") entry and shadows (0, "1.1.1.9")
+		m.Set(id(0), ip("1.1.1.11")) // forgets the (4, "1.1.1.6") entry and shadows (0, "1.1.1.10")
+		m.Set(id(0), ip("1.1.1.12")) // forgets the (5, "1.1.1.7") entry and shadows (0, "1.1.1.11")
 		expectSize(m, 1)
-		expectGet(m, id(0), "L", true)
+		expectGet(m, id(0), "1.1.1.12", true)
 		expectGet(m, id(1), "", false)
 		expectGet(m, id(2), "", false)
 		expectGet(m, id(3), "", false)

_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits