From c441230993307985f973c0f300a88f17a2118b82 Mon Sep 17 00:00:00 2001 From: tabcat Date: Fri, 11 Oct 2024 18:50:10 +0700 Subject: [PATCH 1/2] fix(@libp2p/tcp): race condition in onSocket This fixes a race condition in the onSocket listener method. onSocket gets a net.Socket parameter that needs to be closed later before the tcp server can close. The net.Socket is used to create a MultiaddrConnection but the connection is not tracked with this.connections until after it has been upgraded. If the tcp listener.close is called while a the MultiaddrConnection is waiting to be upgraded, then the MultiaddrConnection socket cannot be closed as it does not exist in this.connections. Instead of adding the MultiaddrConnection to this.connections on creation I decided to create a separate property named this.maConnections. I decided to track the non-upgraded connections separately because I was unsure how this.connections must be used with other things like metrics. --- packages/transport-tcp/src/listener.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/transport-tcp/src/listener.ts b/packages/transport-tcp/src/listener.ts index b83b35d843..8df45f5138 100644 --- a/packages/transport-tcp/src/listener.ts +++ b/packages/transport-tcp/src/listener.ts @@ -69,6 +69,7 @@ export class TCPListener extends TypedEventEmitter implements Li private readonly server: net.Server /** Keep track of open connections to destroy in case of timeout */ private readonly connections = new Set() + private readonly maConnections = new Set() private status: Status = { code: TCPListenerStatusCode.INACTIVE } private metrics?: TCPListenerMetrics private addr: string @@ -195,11 +196,13 @@ export class TCPListener extends TypedEventEmitter implements Li } this.log('new inbound connection %s', maConn.remoteAddr) + this.maConnections.add(maConn) this.context.upgrader.upgradeInbound(maConn) .then((conn) => { this.log('inbound connection upgraded %s', maConn.remoteAddr) this.connections.add(maConn) + this.maConnections.delete(maConn) socket.once('close', () => { this.connections.delete(maConn) @@ -307,6 +310,11 @@ export class TCPListener extends TypedEventEmitter implements Li conn.abort(err) }) + // cleanup connections that have not been upgraded + this.maConnections.forEach(conn => { + conn.abort(err) + }) + // shut down the server socket, permanently await this.pause(true) } From f3cf0196df603794d51bd0bda8ec80560b0f0fa0 Mon Sep 17 00:00:00 2001 From: tabcat Date: Fri, 11 Oct 2024 23:49:56 +0700 Subject: [PATCH 2/2] remove maConn from this.maConnections on upgrade fail --- packages/transport-tcp/src/listener.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/transport-tcp/src/listener.ts b/packages/transport-tcp/src/listener.ts index 8df45f5138..5a168ec540 100644 --- a/packages/transport-tcp/src/listener.ts +++ b/packages/transport-tcp/src/listener.ts @@ -242,6 +242,7 @@ export class TCPListener extends TypedEventEmitter implements Li .catch(async err => { this.log.error('inbound connection upgrade failed', err) this.metrics?.errors.increment({ [`${this.addr} inbound_upgrade`]: true }) + this.maConnections.delete(maConn) maConn.abort(err) }) }