From 9b2581564111d0d2c62346f5ea6867bf36004a3c Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 15 Apr 2015 20:04:40 +0100 Subject: [PATCH 1/4] don't send heartbeats on new but ultimately invalid connections It is wasteful and can trip up crypto at the other end. See comment for why. To accomplish this we move the responsibility for shutting down the new, invalid connection from LocalPeer.AddConnection to its caller, i.e. LocalConnection.run. So rather than always firing up all the connection machinery and getting told asynchronously by AddConnection to shut it all down again, we now only fire it up if AddConnection succeeded. --- router/connection.go | 31 +++++++++++++++++++++++-------- router/local_peer.go | 23 ++++++++++------------- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/router/connection.go b/router/connection.go index 240abddd..fdadfac9 100644 --- a/router/connection.go +++ b/router/connection.go @@ -298,11 +298,13 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch } conn.Log("completed handshake") - if err = conn.initHeartbeats(); err != nil { // [1] + if err = conn.Router.Ourself.AddConnection(conn); err != nil { // [1] return } - conn.Router.Ourself.AddConnection(conn) // [1] + if err = conn.initHeartbeats(); err != nil { // [1] + return + } go conn.receiveTCP(dec) // [1] @@ -311,11 +313,14 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch // [1] Ordering constraints: // -// (a) initHeartbeats should precede AddConnection. That way we can -// guarantee that a) the forwarder gets initialised before the router -// can discover the connection and attempt to invoke Forward, and b) -// Heartbeats are the first thing that get sent on connections. -// Neither of these are essential, but they are desirable. +// (a) AddConnections must precede initHeartbeats. It is only after +// the former completes that we know the connection is valid, in +// particular is not a duplicate connection to the same peer. Sending +// heartbeats on a duplicate connection can trip up crypto at the +// other end, since the associated UDP packets may get decoded by the +// other connection. It is also generally wasteful to engage in any +// interaction with the remote on a connection that turns out to be +// invald. // // (b) AddConnection must precede receiveTCP. In the absence of any // indirect connectivity to the remote peer, the first we hear about @@ -332,7 +337,17 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch // precede shutdown, since that invokes DeleteConnection and is // invoked on termination of this entire function. Essentially this // boils down to a prohibition on running AddConnection in a separate -// goroutine, at least not without some synchronisation. +// goroutine, at least not without some synchronisation. Which in turn +// requires us the launching of the receiveTCP goroutine to precede +// actorLoop. +// +// (d) AddConnection should precede receiveTCP. There is no point +// starting the latter if the former fails. +// +// (e) initHeartbeats should precede actorLoop. The former is setting +// LocalConnection fields accessed by the latter. Since the latter +// runs in a separate goroutine, we'd have to add some synchronisation +// if initHeartbeats isn't run first. func (conn *LocalConnection) initHeartbeats() error { conn.heartbeatTCP = time.NewTicker(TCPHeartbeat) diff --git a/router/local_peer.go b/router/local_peer.go index 64f16c8f..69d71481 100644 --- a/router/local_peer.go +++ b/router/local_peer.go @@ -115,13 +115,12 @@ func (peer *LocalPeer) CreateConnection(peerAddr string, acceptNewPeer bool) err // ACTOR client API // Sync. -func (peer *LocalPeer) AddConnection(conn *LocalConnection) { - resultChan := make(chan interface{}) +func (peer *LocalPeer) AddConnection(conn *LocalConnection) error { + resultChan := make(chan error) peer.actionChan <- func() { - peer.handleAddConnection(conn) - resultChan <- nil + resultChan <- peer.handleAddConnection(conn) } - <-resultChan + return <-resultChan } // Async. @@ -155,7 +154,7 @@ func (peer *LocalPeer) actorLoop(actionChan <-chan LocalPeerAction) { } } -func (peer *LocalPeer) handleAddConnection(conn Connection) { +func (peer *LocalPeer) handleAddConnection(conn Connection) error { if peer.Peer != conn.Local() { log.Fatal("Attempt made to add connection to peer where peer is not the source of connection") } @@ -167,26 +166,23 @@ func (peer *LocalPeer) handleAddConnection(conn Connection) { // deliberately non symmetrical if dupConn, found := peer.connections[toName]; found { if dupConn == conn { - return + return nil } switch conn.BreakTie(dupConn) { case TieBreakWon: dupConn.Shutdown(dupErr) peer.handleDeleteConnection(dupConn) case TieBreakLost: - conn.Shutdown(dupErr) - return + return dupErr case TieBreakTied: // oh good grief. Sod it, just kill both of them. - conn.Shutdown(dupErr) dupConn.Shutdown(dupErr) peer.handleDeleteConnection(dupConn) - return + return dupErr } } if err := peer.checkConnectionLimit(); err != nil { - conn.Shutdown(err) - return + return err } _, isConnectedPeer := peer.router.Routes.Unicast(toName) peer.addConnection(conn) @@ -197,6 +193,7 @@ func (peer *LocalPeer) handleAddConnection(conn Connection) { peer.router.SendAllGossipDown(conn) } peer.broadcastPeerUpdate(conn.Remote()) + return nil } func (peer *LocalPeer) handleConnectionEstablished(conn Connection) { From 57b7276ceb7eb008dc585cd8f6707a61d10cbadc Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 15 Apr 2015 20:55:42 +0100 Subject: [PATCH 2/4] create forwarders before AddConnection, if we can This is beneficial for reasons explained in the comments. And we used to do that previously, when initHeartbeats was called before AddConnection. --- router/connection.go | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/router/connection.go b/router/connection.go index fdadfac9..32678453 100644 --- a/router/connection.go +++ b/router/connection.go @@ -298,17 +298,21 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch } conn.Log("completed handshake") - if err = conn.Router.Ourself.AddConnection(conn); err != nil { // [1] + // The ordering of the following is very important. [1] + + if conn.remoteUDPAddr != nil { + if err = conn.ensureForwarders(); err != nil { + return + } + } + if err = conn.Router.Ourself.AddConnection(conn); err != nil { return } - - if err = conn.initHeartbeats(); err != nil { // [1] + if err = conn.initHeartbeats(); err != nil { return } - - go conn.receiveTCP(dec) // [1] - - err = conn.actorLoop(actionChan) // [1] + go conn.receiveTCP(dec) + err = conn.actorLoop(actionChan) } // [1] Ordering constraints: @@ -348,6 +352,13 @@ func (conn *LocalConnection) run(actionChan <-chan ConnectionAction, finished ch // LocalConnection fields accessed by the latter. Since the latter // runs in a separate goroutine, we'd have to add some synchronisation // if initHeartbeats isn't run first. +// +// (f) ensureForwarders should precede AddConnection. As soon as a +// connection has been added to LocalPeer by the latter, it becomes +// visible to the packet routing logic, which will end up dropping +// packets if the forwarders haven't been created yet. We cannot +// prevent that completely, since, for example, forwarder can only be +// created when we know the remote UDP address, but it helps to try. func (conn *LocalConnection) initHeartbeats() error { conn.heartbeatTCP = time.NewTicker(TCPHeartbeat) From f4ba0f7287abff919ca786c5717ecaf07c5fc267 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 15 Apr 2015 21:10:22 +0100 Subject: [PATCH 3/4] log udp packet processing errors against the connection --- router/router.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/router/router.go b/router/router.go index 450216ef..797dccf6 100644 --- a/router/router.go +++ b/router/router.go @@ -257,8 +257,10 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { } else { relayConn.Log(pde.Error()) } - } else { - checkWarn(err) + continue + } + if err != nil { + relayConn.Log(err) } } } From a5a91ebb331c3f04465c6b89a5c58e5a909720d7 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Wed, 15 Apr 2015 21:52:56 +0100 Subject: [PATCH 4/4] do not shut down connections on PacketDecodingError --- router/crypto.go | 5 ++--- router/router.go | 22 ++++++++++++---------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/router/crypto.go b/router/crypto.go index 08645df1..7c2c0b4f 100644 --- a/router/crypto.go +++ b/router/crypto.go @@ -271,8 +271,7 @@ type NaClDecryptorInstance struct { } type PacketDecodingError struct { - Fatal bool - Desc string + Desc string } func NewNonDecryptor(conn *LocalConnection) *NonDecryptor { @@ -335,7 +334,7 @@ func (nd *NaClDecryptor) ReceiveNonce(msg []byte) { 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 PacketDecodingError{Desc: fmt.Sprint("decryption failed; ", err)} } return nd.NonDecryptor.IterateFrames(buf, consumer) } diff --git a/router/router.go b/router/router.go index 797dccf6..092a3842 100644 --- a/router/router.go +++ b/router/router.go @@ -250,16 +250,18 @@ func (router *Router) udpReader(conn *net.UDPConn, po PacketSink) { if !ok { continue } - err = relayConn.Decryptor.IterateFrames(packet, router.handleUDPPacketFunc(dec, sender, po)) - if pde, ok := err.(PacketDecodingError); ok { - if pde.Fatal { - relayConn.Shutdown(pde) - } else { - relayConn.Log(pde.Error()) - } - continue - } - if err != nil { + if err := relayConn.Decryptor.IterateFrames(packet, router.handleUDPPacketFunc(dec, sender, po)); err != nil { + // Errors during UDP packet decoding / processing are + // non-fatal. One common cause is that we receive and + // attempt to decrypt a "stray" packet. This can actually + // happen quite easily if there is some connection churn + // between two peers. After all, UDP isn't a + // connection-oriented protocol, yet we pretend it is. + // + // If anything really is seriously, unrecoverably amiss + // with a connection, that will typically result in missed + // heartbeats and the connection getting shut down because + // of that. relayConn.Log(err) } }