From 67fe7657b34695efc4f5d70f0f5df03504b1a29b Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 2 Feb 2022 19:17:01 +0200 Subject: [PATCH 1/4] always set the peer if the peer scope is null Removes the very ugly contract of "if you know the peer you have set it" and makes the code robust against upstream bugz. --- p2p/net/upgrader/upgrader.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/p2p/net/upgrader/upgrader.go b/p2p/net/upgrader/upgrader.go index 9a3ec0f8ba..3ea71bf777 100644 --- a/p2p/net/upgrader/upgrader.go +++ b/p2p/net/upgrader/upgrader.go @@ -157,9 +157,9 @@ func (u *upgrader) upgrade(ctx context.Context, t transport.Transport, maconn ma return nil, fmt.Errorf("gater rejected connection with peer %s and addr %s with direction %d", sconn.RemotePeer().Pretty(), maconn.RemoteMultiaddr(), dir) } - // Only call SetPeer if we didn't know the peer ID in advance. - // Otherwise, the caller will already have called it before calling Upgrade. - if p == "" { + // Only call SetPeer if it hasn't already been set -- this can happen when we don't know + // the peer in advance and in some bug scenarios. + if connScope.PeerScope() == nil { if err := connScope.SetPeer(sconn.RemotePeer()); err != nil { log.Debugw("resource manager blocked connection for peer", "peer", sconn.RemotePeer(), "addr", conn.RemoteAddr(), "error", err) if err := maconn.Close(); err != nil { From 191278d558ba6baecb073e84b3906943797c9e29 Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 2 Feb 2022 19:17:32 +0200 Subject: [PATCH 2/4] fix tests --- p2p/net/upgrader/listener_test.go | 1 + p2p/net/upgrader/upgrader_test.go | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/p2p/net/upgrader/listener_test.go b/p2p/net/upgrader/listener_test.go index c8daef187a..464e4f6bec 100644 --- a/p2p/net/upgrader/listener_test.go +++ b/p2p/net/upgrader/listener_test.go @@ -363,6 +363,7 @@ func TestListenerResourceManagement(t *testing.T) { connScope := mocknetwork.NewMockConnManagementScope(ctrl) gomock.InOrder( rcmgr.EXPECT().OpenConnection(network.DirInbound, true).Return(connScope, nil), + connScope.EXPECT().PeerScope(), connScope.EXPECT().SetPeer(id), connScope.EXPECT().PeerScope(), ) diff --git a/p2p/net/upgrader/upgrader_test.go b/p2p/net/upgrader/upgrader_test.go index bf40438d09..38bd6c74a3 100644 --- a/p2p/net/upgrader/upgrader_test.go +++ b/p2p/net/upgrader/upgrader_test.go @@ -153,7 +153,11 @@ func TestOutboundResourceManagement(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() connScope := mocknetwork.NewMockConnManagementScope(ctrl) - connScope.EXPECT().PeerScope().Return(network.NullScope) + gomock.InOrder( + connScope.EXPECT().PeerScope(), + connScope.EXPECT().SetPeer(id), + connScope.EXPECT().PeerScope().Return(network.NullScope), + ) _, dialUpgrader := createUpgrader(t) conn, err := dial(t, dialUpgrader, ln.Multiaddr(), id, connScope) require.NoError(t, err) @@ -171,6 +175,8 @@ func TestOutboundResourceManagement(t *testing.T) { defer ctrl.Finish() connScope := mocknetwork.NewMockConnManagementScope(ctrl) gomock.InOrder( + connScope.EXPECT().PeerScope(), + connScope.EXPECT().SetPeer(id), connScope.EXPECT().PeerScope().Return(network.NullScope), connScope.EXPECT().Done(), ) From f75dfe12ff7b8f35baa0acc1e7ab6abd57ffc9de Mon Sep 17 00:00:00 2001 From: vyzo Date: Wed, 2 Feb 2022 19:28:36 +0200 Subject: [PATCH 3/4] more robust nil check --- p2p/net/upgrader/upgrader.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/p2p/net/upgrader/upgrader.go b/p2p/net/upgrader/upgrader.go index 3ea71bf777..8f15317324 100644 --- a/p2p/net/upgrader/upgrader.go +++ b/p2p/net/upgrader/upgrader.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net" + "reflect" "time" "github.com/libp2p/go-libp2p-core/connmgr" @@ -159,7 +160,7 @@ func (u *upgrader) upgrade(ctx context.Context, t transport.Transport, maconn ma } // Only call SetPeer if it hasn't already been set -- this can happen when we don't know // the peer in advance and in some bug scenarios. - if connScope.PeerScope() == nil { + if ps := connScope.PeerScope(); ps == nil || reflect.ValueOf(ps).IsNil() { if err := connScope.SetPeer(sconn.RemotePeer()); err != nil { log.Debugw("resource manager blocked connection for peer", "peer", sconn.RemotePeer(), "addr", conn.RemoteAddr(), "error", err) if err := maconn.Close(); err != nil { From 8350e75f17964c2e2d42b19853aee14de4c757e8 Mon Sep 17 00:00:00 2001 From: vyzo Date: Thu, 3 Feb 2022 08:56:03 +0200 Subject: [PATCH 4/4] avoid reflection --- p2p/net/upgrader/upgrader.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/p2p/net/upgrader/upgrader.go b/p2p/net/upgrader/upgrader.go index 8f15317324..3ea71bf777 100644 --- a/p2p/net/upgrader/upgrader.go +++ b/p2p/net/upgrader/upgrader.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "net" - "reflect" "time" "github.com/libp2p/go-libp2p-core/connmgr" @@ -160,7 +159,7 @@ func (u *upgrader) upgrade(ctx context.Context, t transport.Transport, maconn ma } // Only call SetPeer if it hasn't already been set -- this can happen when we don't know // the peer in advance and in some bug scenarios. - if ps := connScope.PeerScope(); ps == nil || reflect.ValueOf(ps).IsNil() { + if connScope.PeerScope() == nil { if err := connScope.SetPeer(sconn.RemotePeer()); err != nil { log.Debugw("resource manager blocked connection for peer", "peer", sconn.RemotePeer(), "addr", conn.RemoteAddr(), "error", err) if err := maconn.Close(); err != nil {