From a670272a372ee61b9f2f5f0f2922b14eb5516819 Mon Sep 17 00:00:00 2001 From: David Wragg Date: Wed, 18 Mar 2015 12:04:22 +0000 Subject: [PATCH 1/5] Remove name parameter from (*Peers)ForEach consumer function It's part of the Peer anyway. --- router/connection_maker.go | 2 +- router/json.go | 2 +- router/peers.go | 8 ++++---- router/routes.go | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/router/connection_maker.go b/router/connection_maker.go index d9820abb..61417feb 100644 --- a/router/connection_maker.go +++ b/router/connection_maker.go @@ -132,7 +132,7 @@ func (cm *ConnectionMaker) checkStateAndAttemptConnections() time.Duration { // Add targets for peers that someone else is connected to, but we // aren't - cm.peers.ForEach(func(name PeerName, peer *Peer) { + cm.peers.ForEach(func(peer *Peer) { for _, conn := range peer.Connections() { otherPeer := conn.Remote().Name if otherPeer == cm.ourself.Name || ourConnectedPeers[otherPeer] { diff --git a/router/json.go b/router/json.go index 81407fff..07e538e5 100644 --- a/router/json.go +++ b/router/json.go @@ -36,7 +36,7 @@ func (cache *MacCache) MarshalJSON() ([]byte, error) { func (peers *Peers) MarshalJSON() ([]byte, error) { var ps []*Peer - peers.ForEach(func(_ PeerName, peer *Peer) { ps = append(ps, peer) }) + peers.ForEach(func(peer *Peer) { ps = append(ps, peer) }) return json.Marshal(ps) } diff --git a/router/peers.go b/router/peers.go index 62963e34..5edf5639 100644 --- a/router/peers.go +++ b/router/peers.go @@ -55,11 +55,11 @@ func (peers *Peers) Fetch(name PeerName) (*Peer, bool) { return peer, found // GRRR, why can't I inline this!? } -func (peers *Peers) ForEach(fun func(PeerName, *Peer)) { +func (peers *Peers) ForEach(fun func(*Peer)) { peers.RLock() defer peers.RUnlock() - for name, peer := range peers.table { - fun(name, peer) + for _, peer := range peers.table { + fun(peer) } } @@ -129,7 +129,7 @@ func (peers *Peers) GarbageCollect() []*Peer { func (peers *Peers) String() string { var buf bytes.Buffer - peers.ForEach(func(name PeerName, peer *Peer) { + peers.ForEach(func(peer *Peer) { fmt.Fprint(&buf, peer, "\n") for _, conn := range peer.Connections() { established := "" diff --git a/router/routes.go b/router/routes.go index 8a20e62f..2c99e034 100644 --- a/router/routes.go +++ b/router/routes.go @@ -124,7 +124,7 @@ func (routes *Routes) calculateBroadcast() map[PeerName][]PeerName { broadcast := make(map[PeerName][]PeerName) ourself := routes.ourself - routes.peers.ForEach(func(name PeerName, peer *Peer) { + routes.peers.ForEach(func(peer *Peer) { hops := []PeerName{} if found, reached := peer.Routes(ourself, true); found { // This is rather similar to the inner loop on @@ -143,7 +143,7 @@ func (routes *Routes) calculateBroadcast() map[PeerName][]PeerName { } } } - broadcast[name] = hops + broadcast[peer.Name] = hops }) return broadcast } From bc45a363ddb6cdfc1ded1091c6247a8bad783a55 Mon Sep 17 00:00:00 2001 From: David Wragg Date: Wed, 18 Mar 2015 15:31:36 +0000 Subject: [PATCH 2/5] Make Encryptor.AppendFrame more consistent with FrameConsumer Also removes references to Peer type in crypto.go, to keep it agnostic of higher-level concerns. --- router/crypto.go | 19 +++++++++---------- router/forwarder.go | 9 +++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index c4f29d26..f1f8ae51 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -107,7 +107,7 @@ type Encryptor interface { PacketOverhead() int IsEmpty() bool Bytes() []byte - AppendFrame(*ForwardedFrame) + AppendFrame(src []byte, dst []byte, frame []byte) TotalLen() int } @@ -159,18 +159,17 @@ func (ne *NonEncryptor) Bytes() []byte { return buf } -func (ne *NonEncryptor) AppendFrame(frame *ForwardedFrame) { +func (ne *NonEncryptor) AppendFrame(src []byte, dst []byte, frame []byte) { bufTail := ne.bufTail - srcLen := copy(bufTail, frame.srcPeer.NameByte) + srcLen := copy(bufTail, src) bufTail = bufTail[srcLen:] - dstLen := copy(bufTail, frame.dstPeer.NameByte) + dstLen := copy(bufTail, dst) bufTail = bufTail[dstLen:] - frameLen := len(frame.frame) - binary.BigEndian.PutUint16(bufTail, uint16(frameLen)) + binary.BigEndian.PutUint16(bufTail, uint16(len(frame))) bufTail = bufTail[2:] - copy(bufTail, frame.frame) - ne.bufTail = bufTail[frameLen:] - ne.buffered += srcLen + dstLen + 2 + frameLen + copy(bufTail, frame) + ne.bufTail = bufTail[len(frame):] + ne.buffered += srcLen + dstLen + 2 + len(frame) } func (ne *NonEncryptor) TotalLen() int { @@ -245,7 +244,7 @@ func (ne *NaClEncryptor) TotalLen() int { // Frame Decryptors -type FrameConsumer func(*LocalConnection, *net.UDPAddr, []byte, []byte, uint16, []byte) +type FrameConsumer func(conn *LocalConnection, sender *net.UDPAddr, src []byte, dst []byte, len uint16, frame []byte) type Decryptor interface { IterateFrames(FrameConsumer, *UDPPacket) error diff --git a/router/forwarder.go b/router/forwarder.go index 28f410e8..f1aeae3a 100644 --- a/router/forwarder.go +++ b/router/forwarder.go @@ -281,11 +281,8 @@ func (fwd *Forwarder) verifyEffectivePMTU(newUnverifiedPMTU int) { } func (fwd *Forwarder) attemptVerifyEffectivePMTU() { - pmtuVerifyFrame := &ForwardedFrame{ - srcPeer: fwd.conn.local, - dstPeer: fwd.conn.remote, - frame: make([]byte, fwd.unverifiedPMTU+EthernetOverhead)} - fwd.enc.AppendFrame(pmtuVerifyFrame) + fwd.enc.AppendFrame(fwd.conn.local.NameByte, fwd.conn.remote.NameByte, + make([]byte, fwd.unverifiedPMTU+EthernetOverhead)) fwd.flush() if fwd.verifyPMTUTick == nil { fwd.verifyPMTUTick = time.After(PMTUVerifyTimeout << (PMTUVerifyAttempts - fwd.pmtuVerifyCount)) @@ -337,7 +334,7 @@ func (fwd *Forwarder) appendFrame(frame *ForwardedFrame) bool { if fwd.enc.TotalLen()+fwd.enc.FrameOverhead()+frameLen > fwd.maxPayload { return false } - fwd.enc.AppendFrame(frame) + fwd.enc.AppendFrame(frame.srcPeer.NameByte, frame.dstPeer.NameByte, frame.frame) return true } From 4a1d3aeb9634d8e639cde794e0a9af7882088680 Mon Sep 17 00:00:00 2001 From: David Wragg Date: Wed, 18 Mar 2015 16:20:32 +0000 Subject: [PATCH 3/5] Remove frameLen parameter to FrameConsumer It's the same as len(frame) --- router/crypto.go | 4 ++-- router/router.go | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index f1f8ae51..7aae7200 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -244,7 +244,7 @@ func (ne *NaClEncryptor) TotalLen() int { // Frame Decryptors -type FrameConsumer func(conn *LocalConnection, sender *net.UDPAddr, src []byte, dst []byte, len uint16, frame []byte) +type FrameConsumer func(conn *LocalConnection, sender *net.UDPAddr, src []byte, dst []byte, frame []byte) type Decryptor interface { IterateFrames(FrameConsumer, *UDPPacket) error @@ -294,7 +294,7 @@ func (nd *NonDecryptor) IterateFrames(consumer FrameConsumer, packet *UDPPacket) } frame := buf[:length] buf = buf[length:] - consumer(nd.conn, packet.Sender, srcNameByte, dstNameByte, length, frame) + consumer(nd.conn, packet.Sender, srcNameByte, dstNameByte, frame) } if len(buf) > 0 { return PacketDecodingError{Desc: fmt.Sprintf("%d octets of trailing garbage", len(buf))} diff --git a/router/router.go b/router/router.go index a78f020f..2f4f0b61 100644 --- a/router/router.go +++ b/router/router.go @@ -268,7 +268,7 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { } func (router *Router) handleUDPPacketFunc(dec *EthernetDecoder, po PacketSink) FrameConsumer { - return func(relayConn *LocalConnection, sender *net.UDPAddr, srcNameByte, dstNameByte []byte, frameLen uint16, frame []byte) { + return func(relayConn *LocalConnection, sender *net.UDPAddr, srcNameByte, dstNameByte []byte, frame []byte) { srcPeer, found := router.Peers.Fetch(PeerNameFromBin(srcNameByte)) if !found { return @@ -293,7 +293,7 @@ func (router *Router) handleUDPPacketFunc(dec *EthernetDecoder, po PacketSink) F // detecting special frames is cheaper post decoding than pre. if decodedLen == 1 && dec.IsSpecial() { if srcPeer == relayConn.Remote() && dstPeer == router.Ourself.Peer { - handleSpecialFrame(relayConn, sender, frameLen, frame) + handleSpecialFrame(relayConn, sender, frame) } } @@ -335,7 +335,8 @@ func (router *Router) handleUDPPacketFunc(dec *EthernetDecoder, po PacketSink) F } } -func handleSpecialFrame(relayConn *LocalConnection, sender *net.UDPAddr, frameLen uint16, frame []byte) { +func handleSpecialFrame(relayConn *LocalConnection, sender *net.UDPAddr, frame []byte) { + frameLen := len(frame) switch { case frameLen == EthernetOverhead+8: relayConn.ReceivedHeartbeat(sender, binary.BigEndian.Uint64(frame[EthernetOverhead:])) From 0d30df165fd382facf1742a9b25a1000ed8f6ccf Mon Sep 17 00:00:00 2001 From: David Wragg Date: Fri, 27 Mar 2015 17:31:23 +0000 Subject: [PATCH 4/5] Eliminate UDPPacket The Name field is not used anywhere, and once that's gone the remaining pairing of the packet bytes and sender is not worth it. --- router/crypto.go | 40 +++++++++++++++++++--------------------- router/router.go | 16 +--------------- 2 files changed, 20 insertions(+), 36 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index 7aae7200..0f788d76 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -247,7 +247,7 @@ func (ne *NaClEncryptor) TotalLen() int { type FrameConsumer func(conn *LocalConnection, sender *net.UDPAddr, src []byte, dst []byte, frame []byte) type Decryptor interface { - IterateFrames(FrameConsumer, *UDPPacket) error + IterateFrames(FrameConsumer, []byte, *net.UDPAddr) error ReceiveNonce([]byte) Shutdown() } @@ -280,24 +280,23 @@ func NewNonDecryptor(conn *LocalConnection) *NonDecryptor { return &NonDecryptor{conn: conn} } -func (nd *NonDecryptor) IterateFrames(consumer FrameConsumer, packet *UDPPacket) error { - buf := packet.Packet - for len(buf) >= (2 + NameSize + NameSize) { - srcNameByte := buf[:NameSize] - buf = buf[NameSize:] - dstNameByte := buf[:NameSize] - buf = buf[NameSize:] - length := binary.BigEndian.Uint16(buf[:2]) - buf = buf[2:] - if len(buf) < int(length) { - return PacketDecodingError{Desc: fmt.Sprintf("too short; expected frame of length %d, got %d", length, len(buf))} +func (nd *NonDecryptor) IterateFrames(consumer FrameConsumer, packet []byte, sender *net.UDPAddr) error { + for len(packet) >= (2 + NameSize + NameSize) { + srcNameByte := packet[:NameSize] + packet = packet[NameSize:] + dstNameByte := packet[:NameSize] + packet = packet[NameSize:] + length := binary.BigEndian.Uint16(packet[:2]) + packet = packet[2:] + if len(packet) < int(length) { + return PacketDecodingError{Desc: fmt.Sprintf("too short; expected frame of length %d, got %d", length, len(packet))} } - frame := buf[:length] - buf = buf[length:] - consumer(nd.conn, packet.Sender, srcNameByte, dstNameByte, frame) + frame := packet[:length] + packet = packet[length:] + consumer(nd.conn, sender, srcNameByte, dstNameByte, frame) } - if len(buf) > 0 { - return PacketDecodingError{Desc: fmt.Sprintf("%d octets of trailing garbage", len(buf))} + if len(packet) > 0 { + return PacketDecodingError{Desc: fmt.Sprintf("%d octets of trailing garbage", len(packet))} } return nil } @@ -344,13 +343,12 @@ func (nd *NaClDecryptor) ReceiveNonce(msg []byte) { } } -func (nd *NaClDecryptor) IterateFrames(consumer FrameConsumer, packet *UDPPacket) error { - buf, err := nd.decrypt(packet.Packet) +func (nd *NaClDecryptor) IterateFrames(consumer FrameConsumer, packet []byte, sender *net.UDPAddr) error { + buf, err := nd.decrypt(packet) if err != nil { return PacketDecodingError{Fatal: true, Desc: fmt.Sprint("decryption failed; ", err)} } - packet.Packet = buf - return nd.NonDecryptor.IterateFrames(consumer, packet) + return nd.NonDecryptor.IterateFrames(consumer, buf, sender) } func (nd *NaClDecryptor) decrypt(buf []byte) ([]byte, error) { diff --git a/router/router.go b/router/router.go index 2f4f0b61..ad3f1899 100644 --- a/router/router.go +++ b/router/router.go @@ -213,16 +213,6 @@ func (router *Router) listenUDP(localPort int, po PacketSink) *net.UDPConn { return conn } -type UDPPacket struct { - Name PeerName - Packet []byte - Sender *net.UDPAddr -} - -func (packet UDPPacket) String() string { - return fmt.Sprintf("UDP Packet\n name: %s\n sender: %v\n payload: % X", packet.Name, packet.Sender, packet.Packet) -} - func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { defer conn.Close() dec := NewEthernetDecoder() @@ -242,10 +232,6 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { name := PeerNameFromBin(buf[:NameSize]) packet := make([]byte, n-NameSize) copy(packet, buf[NameSize:n]) - udpPacket := &UDPPacket{ - Name: name, - Packet: packet, - Sender: sender} peerConn, found := router.Ourself.ConnectionTo(name) if !found { continue @@ -254,7 +240,7 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { if !ok { continue } - err = relayConn.Decryptor.IterateFrames(handleUDPPacket, udpPacket) + err = relayConn.Decryptor.IterateFrames(handleUDPPacket, packet, sender) if pde, ok := err.(PacketDecodingError); ok { if pde.Fatal { relayConn.Shutdown(pde) From 78c70023d89565eaf2c043bad721e8c36e4d364d Mon Sep 17 00:00:00 2001 From: David Wragg Date: Fri, 27 Mar 2015 17:45:56 +0000 Subject: [PATCH 5/5] Don't pass the sender UDPAddr through the crypto layer The crypto layer doesn't need it, and the cost of an additional closure allocation for each incoming UDP packet is modest. Also, make the FrameConsumer the last parameter to IterateFrames. It seems a more natural ordering. --- router/crypto.go | 13 ++++++------- router/router.go | 7 +++---- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index 0f788d76..3d7cad9e 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -11,7 +11,6 @@ import ( "encoding/gob" "fmt" "log" - "net" "sync" ) @@ -244,10 +243,10 @@ func (ne *NaClEncryptor) TotalLen() int { // Frame Decryptors -type FrameConsumer func(conn *LocalConnection, sender *net.UDPAddr, src []byte, dst []byte, frame []byte) +type FrameConsumer func(conn *LocalConnection, src []byte, dst []byte, frame []byte) type Decryptor interface { - IterateFrames(FrameConsumer, []byte, *net.UDPAddr) error + IterateFrames([]byte, FrameConsumer) error ReceiveNonce([]byte) Shutdown() } @@ -280,7 +279,7 @@ func NewNonDecryptor(conn *LocalConnection) *NonDecryptor { return &NonDecryptor{conn: conn} } -func (nd *NonDecryptor) IterateFrames(consumer FrameConsumer, packet []byte, sender *net.UDPAddr) error { +func (nd *NonDecryptor) IterateFrames(packet []byte, consumer FrameConsumer) error { for len(packet) >= (2 + NameSize + NameSize) { srcNameByte := packet[:NameSize] packet = packet[NameSize:] @@ -293,7 +292,7 @@ func (nd *NonDecryptor) IterateFrames(consumer FrameConsumer, packet []byte, sen } frame := packet[:length] packet = packet[length:] - consumer(nd.conn, sender, srcNameByte, dstNameByte, frame) + consumer(nd.conn, srcNameByte, dstNameByte, frame) } if len(packet) > 0 { return PacketDecodingError{Desc: fmt.Sprintf("%d octets of trailing garbage", len(packet))} @@ -343,12 +342,12 @@ func (nd *NaClDecryptor) ReceiveNonce(msg []byte) { } } -func (nd *NaClDecryptor) IterateFrames(consumer FrameConsumer, packet []byte, sender *net.UDPAddr) error { +func (nd *NaClDecryptor) IterateFrames(packet []byte, consumer FrameConsumer) error { buf, err := nd.decrypt(packet) if err != nil { return PacketDecodingError{Fatal: true, Desc: fmt.Sprint("decryption failed; ", err)} } - return nd.NonDecryptor.IterateFrames(consumer, buf, sender) + return nd.NonDecryptor.IterateFrames(buf, consumer) } func (nd *NaClDecryptor) decrypt(buf []byte) ([]byte, error) { diff --git a/router/router.go b/router/router.go index ad3f1899..922c1009 100644 --- a/router/router.go +++ b/router/router.go @@ -216,7 +216,6 @@ func (router *Router) listenUDP(localPort int, po PacketSink) *net.UDPConn { func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { defer conn.Close() dec := NewEthernetDecoder() - handleUDPPacket := router.handleUDPPacketFunc(dec, po) buf := make([]byte, MaxUDPPacketSize) for { n, sender, err := conn.ReadFromUDP(buf) @@ -240,7 +239,7 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { if !ok { continue } - err = relayConn.Decryptor.IterateFrames(handleUDPPacket, packet, sender) + err = relayConn.Decryptor.IterateFrames(packet, router.handleUDPPacketFunc(dec, sender, po)) if pde, ok := err.(PacketDecodingError); ok { if pde.Fatal { relayConn.Shutdown(pde) @@ -253,8 +252,8 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { } } -func (router *Router) handleUDPPacketFunc(dec *EthernetDecoder, po PacketSink) FrameConsumer { - return func(relayConn *LocalConnection, sender *net.UDPAddr, srcNameByte, dstNameByte []byte, frame []byte) { +func (router *Router) handleUDPPacketFunc(dec *EthernetDecoder, sender *net.UDPAddr, po PacketSink) FrameConsumer { + return func(relayConn *LocalConnection, srcNameByte, dstNameByte []byte, frame []byte) { srcPeer, found := router.Peers.Fetch(PeerNameFromBin(srcNameByte)) if !found { return