From 7b0fa6c889b26c0a9ce528bba9ec2cc23b699e08 Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Tue, 14 Nov 2017 17:39:32 -0700 Subject: [PATCH] p2p: peer should respect errors from SetDeadline Noticed while auditing the code that we aren't respecting (*net.Conn) SetDeadline errors which return after a connection has been killed and is simultaneously being used. For example given program, without SetDeadline error checks ```go package main import ( "log" "net" "time" ) func main() { conn, err := net.Dial("tcp", "tendermint.com:443") if err != nil { log.Fatal(err) } go func() { <-time.After(400 * time.Millisecond) conn.Close() }() for i := 0; i < 5; i++ { if err := conn.SetDeadline(time.Now().Add(time.Duration(10 * time.Second))); err != nil { log.Fatalf("set deadline #%d, err: %v", i, err) } log.Printf("Successfully set deadline #%d", i) <-time.After(150 * time.Millisecond) } } ``` erraneously gives ```shell 2017/11/14 17:46:28 Successfully set deadline #0 2017/11/14 17:46:29 Successfully set deadline #1 2017/11/14 17:46:29 Successfully set deadline #2 2017/11/14 17:46:29 Successfully set deadline #3 2017/11/14 17:46:29 Successfully set deadline #4 ``` However, if we properly fix it to respect that error with ```diff --- wild.go 2017-11-14 17:44:38.000000000 -0700 +++ main.go 2017-11-14 17:45:40.000000000 -0700 @@ -16,7 +16,9 @@ conn.Close() }() for i := 0; i < 5; i++ { - conn.SetDeadline(time.Now().Add(time.Duration(10 * time.Second))) + if err := conn.SetDeadline(time.Now().Add(time.Duration(10 * time.Second))); err != nil { + log.Fatalf("set deadline #%d, err: %v", i, err) + } log.Printf("Successfully set deadline #%d", i) <-time.After(150 * time.Millisecond) } ``` properly catches any problems and gives ```shell $ go run main.go 2017/11/14 17:43:44 Successfully set deadline #0 2017/11/14 17:43:45 Successfully set deadline #1 2017/11/14 17:43:45 Successfully set deadline #2 2017/11/14 17:43:45 set deadline #3, err: set tcp 10.182.253.51:57395: use of closed network connection exit status 1 ``` --- p2p/peer.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/p2p/peer.go b/p2p/peer.go index 9ee1c0e32..ec8349551 100644 --- a/p2p/peer.go +++ b/p2p/peer.go @@ -113,7 +113,9 @@ func newPeerFromConnAndConfig(rawConn net.Conn, outbound bool, reactorsByCh map[ // Encrypt connection if config.AuthEnc { - conn.SetDeadline(time.Now().Add(config.HandshakeTimeout * time.Second)) + if err := conn.SetDeadline(time.Now().Add(config.HandshakeTimeout * time.Second)); err != nil { + return nil, errors.Wrap(err, "Error setting deadline while encrypting connection") + } var err error conn, err = MakeSecretConnection(conn, ourNodePrivKey) @@ -165,7 +167,9 @@ func (p *peer) IsPersistent() bool { // NOTE: blocking func (p *peer) HandshakeTimeout(ourNodeInfo *NodeInfo, timeout time.Duration) error { // Set deadline for handshake so we don't block forever on conn.ReadFull - p.conn.SetDeadline(time.Now().Add(timeout)) + if err := p.conn.SetDeadline(time.Now().Add(timeout)); err != nil { + return errors.Wrap(err, "Error setting deadline") + } var peerNodeInfo = new(NodeInfo) var err1 error @@ -196,7 +200,9 @@ func (p *peer) HandshakeTimeout(ourNodeInfo *NodeInfo, timeout time.Duration) er } // Remove deadline - p.conn.SetDeadline(time.Time{}) + if err := p.conn.SetDeadline(time.Time{}); err != nil { + return errors.Wrap(err, "Error removing deadline") + } peerNodeInfo.RemoteAddr = p.Addr().String()