From 9a6dd96cba96155cfbb590cc522b943d6deda499 Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Sun, 16 Dec 2018 09:27:16 -0800 Subject: [PATCH] Revert to using defers in addrbook. (#3025) * Revert to using defers in addrbook. ValidateBasic->Validate since it requires DNS * Update CHANGELOG_PENDING --- CHANGELOG_PENDING.md | 2 ++ node/node.go | 5 +++++ p2p/netaddress.go | 6 ++++++ p2p/node_info.go | 6 +++--- p2p/node_info_test.go | 6 +++--- p2p/pex/addrbook.go | 25 ++++++++++++++++++------- p2p/test_util.go | 2 +- p2p/transport.go | 2 +- 8 files changed, 39 insertions(+), 15 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index e9a1d52f..6742f4e8 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -21,3 +21,5 @@ Special thanks to external contributors on this release: ### IMPROVEMENTS: ### BUG FIXES: + +- [\#3025](https://github.com/tendermint/tendermint/pull/3025) - Revert to using defers in addrbook. Fixes deadlocks in pex and consensus upon invalid ExternalAddr/ListenAddr configuration. diff --git a/node/node.go b/node/node.go index b56a3594..1bb195fb 100644 --- a/node/node.go +++ b/node/node.go @@ -817,6 +817,11 @@ func makeNodeInfo( nodeInfo.ListenAddr = lAddr + err := nodeInfo.Validate() + if err != nil { + panic(err) + } + return nodeInfo } diff --git a/p2p/netaddress.go b/p2p/netaddress.go index f60271bc..5534ded9 100644 --- a/p2p/netaddress.go +++ b/p2p/netaddress.go @@ -175,6 +175,9 @@ func (na *NetAddress) Same(other interface{}) bool { // String representation: @: func (na *NetAddress) String() string { + if na == nil { + return "" + } if na.str == "" { addrStr := na.DialString() if na.ID != "" { @@ -186,6 +189,9 @@ func (na *NetAddress) String() string { } func (na *NetAddress) DialString() string { + if na == nil { + return "" + } return net.JoinHostPort( na.IP.String(), strconv.FormatUint(uint64(na.Port), 10), diff --git a/p2p/node_info.go b/p2p/node_info.go index 99daf7c4..3e02e9c1 100644 --- a/p2p/node_info.go +++ b/p2p/node_info.go @@ -36,7 +36,7 @@ type nodeInfoAddress interface { // nodeInfoTransport validates a nodeInfo and checks // our compatibility with it. It's for use in the handshake. type nodeInfoTransport interface { - ValidateBasic() error + Validate() error CompatibleWith(other NodeInfo) error } @@ -103,7 +103,7 @@ func (info DefaultNodeInfo) ID() ID { return info.ID_ } -// ValidateBasic checks the self-reported DefaultNodeInfo is safe. +// Validate checks the self-reported DefaultNodeInfo is safe. // It returns an error if there // are too many Channels, if there are any duplicate Channels, // if the ListenAddr is malformed, or if the ListenAddr is a host name @@ -116,7 +116,7 @@ func (info DefaultNodeInfo) ID() ID { // International clients could then use punycode (or we could use // url-encoding), and we just need to be careful with how we handle that in our // clients. (e.g. off by default). -func (info DefaultNodeInfo) ValidateBasic() error { +func (info DefaultNodeInfo) Validate() error { // ID is already validated. diff --git a/p2p/node_info_test.go b/p2p/node_info_test.go index c9a72dbc..19567d2b 100644 --- a/p2p/node_info_test.go +++ b/p2p/node_info_test.go @@ -12,7 +12,7 @@ func TestNodeInfoValidate(t *testing.T) { // empty fails ni := DefaultNodeInfo{} - assert.Error(t, ni.ValidateBasic()) + assert.Error(t, ni.Validate()) channels := make([]byte, maxNumChannels) for i := 0; i < maxNumChannels; i++ { @@ -68,13 +68,13 @@ func TestNodeInfoValidate(t *testing.T) { // test case passes ni = testNodeInfo(nodeKey.ID(), name).(DefaultNodeInfo) ni.Channels = channels - assert.NoError(t, ni.ValidateBasic()) + assert.NoError(t, ni.Validate()) for _, tc := range testCases { ni := testNodeInfo(nodeKey.ID(), name).(DefaultNodeInfo) ni.Channels = channels tc.malleateNodeInfo(&ni) - err := ni.ValidateBasic() + err := ni.Validate() if tc.expectErr { assert.Error(t, err, tc.testName) } else { diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index d8954f23..cfeefb34 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -162,26 +162,29 @@ func (a *addrBook) FilePath() string { // AddOurAddress one of our addresses. func (a *addrBook) AddOurAddress(addr *p2p.NetAddress) { - a.Logger.Info("Add our address to book", "addr", addr) a.mtx.Lock() + defer a.mtx.Unlock() + + a.Logger.Info("Add our address to book", "addr", addr) a.ourAddrs[addr.String()] = struct{}{} - a.mtx.Unlock() } // OurAddress returns true if it is our address. func (a *addrBook) OurAddress(addr *p2p.NetAddress) bool { a.mtx.Lock() + defer a.mtx.Unlock() + _, ok := a.ourAddrs[addr.String()] - a.mtx.Unlock() return ok } func (a *addrBook) AddPrivateIDs(IDs []string) { a.mtx.Lock() + defer a.mtx.Unlock() + for _, id := range IDs { a.privateIDs[p2p.ID(id)] = struct{}{} } - a.mtx.Unlock() } // AddAddress implements AddrBook @@ -191,6 +194,7 @@ func (a *addrBook) AddPrivateIDs(IDs []string) { func (a *addrBook) AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error { a.mtx.Lock() defer a.mtx.Unlock() + return a.addAddress(addr, src) } @@ -198,6 +202,7 @@ func (a *addrBook) AddAddress(addr *p2p.NetAddress, src *p2p.NetAddress) error { func (a *addrBook) RemoveAddress(addr *p2p.NetAddress) { a.mtx.Lock() defer a.mtx.Unlock() + ka := a.addrLookup[addr.ID] if ka == nil { return @@ -211,14 +216,16 @@ func (a *addrBook) RemoveAddress(addr *p2p.NetAddress) { func (a *addrBook) IsGood(addr *p2p.NetAddress) bool { a.mtx.Lock() defer a.mtx.Unlock() + return a.addrLookup[addr.ID].isOld() } // HasAddress returns true if the address is in the book. func (a *addrBook) HasAddress(addr *p2p.NetAddress) bool { a.mtx.Lock() + defer a.mtx.Unlock() + ka := a.addrLookup[addr.ID] - a.mtx.Unlock() return ka != nil } @@ -292,6 +299,7 @@ func (a *addrBook) PickAddress(biasTowardsNewAddrs int) *p2p.NetAddress { func (a *addrBook) MarkGood(addr *p2p.NetAddress) { a.mtx.Lock() defer a.mtx.Unlock() + ka := a.addrLookup[addr.ID] if ka == nil { return @@ -306,6 +314,7 @@ func (a *addrBook) MarkGood(addr *p2p.NetAddress) { func (a *addrBook) MarkAttempt(addr *p2p.NetAddress) { a.mtx.Lock() defer a.mtx.Unlock() + ka := a.addrLookup[addr.ID] if ka == nil { return @@ -461,12 +470,13 @@ ADDRS_LOOP: // ListOfKnownAddresses returns the new and old addresses. func (a *addrBook) ListOfKnownAddresses() []*knownAddress { - addrs := []*knownAddress{} a.mtx.Lock() + defer a.mtx.Unlock() + + addrs := []*knownAddress{} for _, addr := range a.addrLookup { addrs = append(addrs, addr.copy()) } - a.mtx.Unlock() return addrs } @@ -476,6 +486,7 @@ func (a *addrBook) ListOfKnownAddresses() []*knownAddress { func (a *addrBook) Size() int { a.mtx.Lock() defer a.mtx.Unlock() + return a.size() } diff --git a/p2p/test_util.go b/p2p/test_util.go index 44f27be4..ea788b79 100644 --- a/p2p/test_util.go +++ b/p2p/test_util.go @@ -24,7 +24,7 @@ type mockNodeInfo struct { func (ni mockNodeInfo) ID() ID { return ni.addr.ID } func (ni mockNodeInfo) NetAddress() *NetAddress { return ni.addr } -func (ni mockNodeInfo) ValidateBasic() error { return nil } +func (ni mockNodeInfo) Validate() error { return nil } func (ni mockNodeInfo) CompatibleWith(other NodeInfo) error { return nil } func AddPeerToSwitch(sw *Switch, peer Peer) { diff --git a/p2p/transport.go b/p2p/transport.go index b16db54d..69fab312 100644 --- a/p2p/transport.go +++ b/p2p/transport.go @@ -350,7 +350,7 @@ func (mt *MultiplexTransport) upgrade( } } - if err := nodeInfo.ValidateBasic(); err != nil { + if err := nodeInfo.Validate(); err != nil { return nil, nil, ErrRejected{ conn: c, err: err,