[tor-commits] [snowflake/main] Store net.Addr in clientIDAddrMap
cohosh at torproject.org
cohosh at torproject.org
Mon Jun 21 14:14:36 UTC 2021
commit 6634f2bec9ea7c538a819619d003db4ea2947a60
Author: Cecylia Bocovich <cohosh at torproject.org>
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)
More information about the tor-commits
mailing list