From bd8f0338e87af224696c78563987dd9ec28c9afb Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Thu, 20 Feb 2025 15:54:40 +1100 Subject: [PATCH 1/2] fix: properly handling duplicate stream ids if ids are used out of order --- package-lock.json | 61 ++++++++++++++++++ src/QUICConnection.ts | 100 +++++++++++++++++++++++++----- tests/QUICStream.test.ts | 129 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 273 insertions(+), 17 deletions(-) diff --git a/package-lock.json b/package-lock.json index 76f2f859..1e5ecfa7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1469,6 +1469,67 @@ "resolved": "https://registry.npmjs.org/@matrixai/logger/-/logger-3.1.2.tgz", "integrity": "sha512-nNliLCnbg6hGS2+gGtQfeeyVNJzOmvqz90AbrQsHPNiE08l3YsENL2JQt9d454SorD1Ud51ymZdDCkeMLWY93A==" }, + "node_modules/@matrixai/quic-darwin-arm64": { + "version": "1.3.12", + "resolved": "https://registry.npmjs.org/@matrixai/quic-darwin-arm64/-/quic-darwin-arm64-1.3.12.tgz", + "integrity": "sha512-GBLJWYHT4XXA7ZuToZGLHhTYqeKC2KvKfHjWFeO9Fz3aDcFKBTUdN+QiQZFA3N6YtDme6+xBb5qxGvrNPJgHng==", + "cpu": [ + "arm64" + ], + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@matrixai/quic-darwin-universal": { + "version": "1.3.12", + "resolved": "https://registry.npmjs.org/@matrixai/quic-darwin-universal/-/quic-darwin-universal-1.3.12.tgz", + "integrity": "sha512-Tq2Tjp3Pkg293B0Ib7Owds2wzEfYcbkIr4VfrhZ3yCVVWXmC/DSQ23Bd+l9MIGo03/NYUQeZwJ0KuexthERqXA==", + "cpu": [ + "x64", + "arm64" + ], + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@matrixai/quic-darwin-x64": { + "version": "1.3.12", + "resolved": "https://registry.npmjs.org/@matrixai/quic-darwin-x64/-/quic-darwin-x64-1.3.12.tgz", + "integrity": "sha512-uw/jesb4oJoVcsqBVewvu47XVLdffOUGnHPV+m1XNWHaClUKzYJXHRCtwsnlyf2DltzHPBf2Vu+jbkLZcuovnQ==", + "cpu": [ + "x64" + ], + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@matrixai/quic-linux-x64": { + "version": "1.3.12", + "resolved": "https://registry.npmjs.org/@matrixai/quic-linux-x64/-/quic-linux-x64-1.3.12.tgz", + "integrity": "sha512-u+qXouRfZ+h4iLksj/fD3DkWFGFyVl8lQbfKQs0W0mv0U0P9+gYIBNvMIqDn65da7Z8X8jGpFWcATgsBAbVLRQ==", + "cpu": [ + "x64" + ], + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@matrixai/quic-win32-x64": { + "version": "1.3.12", + "resolved": "https://registry.npmjs.org/@matrixai/quic-win32-x64/-/quic-win32-x64-1.3.12.tgz", + "integrity": "sha512-c+x0ZItcrZOUE1ZgGuhiN9AH5KWJCIsmGQbFfkVko6NVI0bDiTIwkqmUA6JbSBCH8JD4jQqnaggMRc4kZmlMsA==", + "cpu": [ + "x64" + ], + "optional": true, + "os": [ + "win32" + ] + }, "node_modules/@matrixai/resources": { "version": "1.1.5", "resolved": "https://registry.npmjs.org/@matrixai/resources/-/resources-1.1.5.tgz", diff --git a/src/QUICConnection.ts b/src/QUICConnection.ts index 9136c5ce..6c0c2024 100644 --- a/src/QUICConnection.ts +++ b/src/QUICConnection.ts @@ -107,22 +107,29 @@ class QUICConnection { /** * Tracks the highest StreamId that has a created QUICStream for clientBidi */ - protected streamIdUsedClientBidi = -1 as StreamId; + protected streamIdUsedClientBidi = (0b00 - 4) as StreamId; /** * Tracks the highest StreamId that has a created QUICStream for serverBidi */ - protected streamIdUsedServerBidi = -1 as StreamId; + protected streamIdUsedServerBidi = (0b01 - 4) as StreamId; /** * Tracks the highest StreamId that has a created QUICStream for clientUni */ - protected streamIdUsedClientUni = -1 as StreamId; + protected streamIdUsedClientUni = (0b10 - 4) as StreamId; /** * Tracks the highest StreamId that has a created QUICStream for clientUni */ - protected streamIdUsedServerUni = -1 as StreamId; + protected streamIdUsedServerUni = (0b11 - 4) as StreamId; + + /** + * Tracks used ids that have skipped the expected next id for the streamIdUsed counters. + * If the next id in the streamIdUsed sequence is used then we remove IDs from the Set + * up to the next ID gap. + */ + protected streamIdUsedSet: Set = new Set(); /** * Quiche connection timer. This performs time delayed state transitions. @@ -988,24 +995,85 @@ class QUICConnection { } protected isStreamUsed(streamId: StreamId): boolean { + let nextId: number; const type = 0b11 & streamId; switch (type) { case 0b00: - if (streamId <= this.streamIdUsedClientBidi) return true; - this.streamIdUsedClientBidi = streamId; - return false; + nextId = this.streamIdUsedClientBidi + 4; + if ( + streamId <= this.streamIdUsedClientBidi || + this.streamIdUsedSet.has(streamId) + ) { + return true; + } else if (streamId === nextId) { + // Increase counter and check set in loop. + do { + this.streamIdUsedClientBidi = nextId as StreamId; + this.streamIdUsedSet.delete(nextId); + nextId += 4; + } while (this.streamIdUsedSet.has(nextId)); + return false; + } else { + this.streamIdUsedSet.add(streamId); + return false; + } case 0b01: - if (streamId <= this.streamIdUsedServerBidi) return true; - this.streamIdUsedServerBidi = streamId; - return false; + nextId = this.streamIdUsedServerBidi + 4; + if ( + streamId <= this.streamIdUsedServerBidi || + this.streamIdUsedSet.has(streamId) + ) { + return true; + } else if (streamId === nextId) { + // Increase counter and check set in loop. + do { + this.streamIdUsedServerBidi = nextId as StreamId; + this.streamIdUsedSet.delete(nextId); + nextId += 4; + } while (this.streamIdUsedSet.has(nextId)); + return false; + } else { + this.streamIdUsedSet.add(streamId); + return false; + } case 0b10: - if (streamId <= this.streamIdUsedClientUni) return true; - this.streamIdUsedClientUni = streamId; - return false; + nextId = this.streamIdUsedClientUni + 4; + if ( + streamId <= this.streamIdUsedClientUni || + this.streamIdUsedSet.has(streamId) + ) { + return true; + } else if (streamId === nextId) { + // Increase counter and check set in loop. + do { + this.streamIdUsedClientUni = nextId as StreamId; + this.streamIdUsedSet.delete(nextId); + nextId += 4; + } while (this.streamIdUsedSet.has(nextId)); + return false; + } else { + this.streamIdUsedSet.add(streamId); + return false; + } case 0b11: - if (streamId <= this.streamIdUsedServerUni) return true; - this.streamIdUsedServerUni = streamId; - return false; + nextId = this.streamIdUsedServerUni + 4; + if ( + streamId <= this.streamIdUsedServerUni || + this.streamIdUsedSet.has(streamId) + ) { + return true; + } else if (streamId === nextId) { + // Increase counter and check set in loop. + do { + this.streamIdUsedServerUni = nextId as StreamId; + this.streamIdUsedSet.delete(nextId); + nextId += 4; + } while (this.streamIdUsedSet.has(nextId)); + return false; + } else { + this.streamIdUsedSet.add(streamId); + return false; + } default: utils.never('got an unexpected ID type'); } diff --git a/tests/QUICStream.test.ts b/tests/QUICStream.test.ts index 1c3f151e..57478633 100644 --- a/tests/QUICStream.test.ts +++ b/tests/QUICStream.test.ts @@ -1,6 +1,12 @@ -import type { ClientCryptoOps, QUICConnection, ServerCryptoOps } from '@'; +import type { + ClientCryptoOps, + QUICConnection, + ServerCryptoOps, + StreamId, +} from '@'; import Logger, { formatting, LogLevel, StreamHandler } from '@matrixai/logger'; import { destroyed } from '@matrixai/async-init'; +import { test, fc } from '@fast-check/jest'; import * as events from '@/events'; import * as errors from '@/errors'; import * as utils from '@/utils'; @@ -2246,4 +2252,125 @@ describe(QUICStream.name, () => { await client.destroy({ force: true }); await server.stop({ force: true }); }); + test.prop( + [ + fc + .array(fc.integer({ min: 1 }), { minLength: 1000, maxLength: 2000 }) + .noShrink(), + ], + { numRuns: 1 }, + )('out of order Ids are handled properly', async (arr) => { + const size = arr.length; + const used: Set = new Set(); + const ids: Array = []; + for (let num of arr) { + do { + num = (num + 1) % size; + } while (used.has(num)); + ids.push(num); + used.add(num); + } + + const connectionEventProm = + utils.promise(); + const tlsConfig = await generateTLSConfig(defaultType); + const server = new QUICServer({ + crypto: { + key, + ops: serverCrypto, + }, + logger: logger.getChild(QUICServer.name), + config: { + key: tlsConfig.leafKeyPairPEM.privateKey, + cert: tlsConfig.leafCertPEM, + verifyPeer: false, + }, + }); + socketCleanMethods.extractSocket(server); + server.addEventListener( + events.EventQUICServerConnection.name, + (e: events.EventQUICServerConnection) => connectionEventProm.resolveP(e), + ); + await server.start({ + host: localhost, + }); + const client = await QUICClient.createQUICClient({ + host: localhost, + port: server.port, + localHost: localhost, + crypto: { + ops: clientCrypto, + }, + logger: logger.getChild(QUICClient.name), + config: { + verifyPeer: false, + }, + }); + socketCleanMethods.extractSocket(client); + await connectionEventProm.p; + + const checkId = (id: StreamId): boolean => { + // @ts-ignore: Using protected method + return client.connection.isStreamUsed(id); + }; + + for (const id of ids) { + expect(checkId(id as StreamId)).toBeFalse(); + } + // @ts-ignore: using protected property + const usedIdSet = client.connection.streamIdUsedSet; + expect(usedIdSet.size).toBe(0); + }); + test('out of order Ids are handled properly', async () => { + const connectionEventProm = + utils.promise(); + const tlsConfig = await generateTLSConfig(defaultType); + const server = new QUICServer({ + crypto: { + key, + ops: serverCrypto, + }, + logger: logger.getChild(QUICServer.name), + config: { + key: tlsConfig.leafKeyPairPEM.privateKey, + cert: tlsConfig.leafCertPEM, + verifyPeer: false, + }, + }); + socketCleanMethods.extractSocket(server); + server.addEventListener( + events.EventQUICServerConnection.name, + (e: events.EventQUICServerConnection) => connectionEventProm.resolveP(e), + ); + await server.start({ + host: localhost, + }); + const client = await QUICClient.createQUICClient({ + host: localhost, + port: server.port, + localHost: localhost, + crypto: { + ops: clientCrypto, + }, + logger: logger.getChild(QUICClient.name), + config: { + verifyPeer: false, + }, + }); + socketCleanMethods.extractSocket(client); + await connectionEventProm.p; + + const checkId = (id: StreamId): boolean => { + // @ts-ignore: Using protected method + return client.connection.isStreamUsed(id); + }; + + expect(checkId(0 as StreamId)).toBeFalse(); + expect(checkId(4 as StreamId)).toBeFalse(); + expect(checkId(8 as StreamId)).toBeFalse(); + expect(checkId(4 as StreamId)).toBeTrue(); + expect(checkId(16 as StreamId)).toBeFalse(); + expect(checkId(0 as StreamId)).toBeTrue(); + expect(checkId(0 as StreamId)).toBeTrue(); + }); }); From a2bc562311a11d8970a2560e7271abf67cf6caf9 Mon Sep 17 00:00:00 2001 From: Brian Botha Date: Thu, 20 Feb 2025 16:15:25 +1100 Subject: [PATCH 2/2] 1.3.13 --- Cargo.lock | 2 +- Cargo.toml | 2 +- package-lock.json | 75 +++++------------------------------------------ package.json | 12 ++++---- 4 files changed, 15 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3580ff96..52957cf4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -388,7 +388,7 @@ dependencies = [ [[package]] name = "quic" -version = "1.3.12" +version = "1.3.13" dependencies = [ "boring", "napi", diff --git a/Cargo.toml b/Cargo.toml index c3895ce1..f736dc1b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "quic" -version = "1.3.12" +version = "1.3.13" authors = ["Roger Qiu "] license-file = "LICENSE" edition = "2021" diff --git a/package-lock.json b/package-lock.json index 1e5ecfa7..906b4123 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@matrixai/quic", - "version": "1.3.12", + "version": "1.3.13", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@matrixai/quic", - "version": "1.3.12", + "version": "1.3.13", "license": "Apache-2.0", "dependencies": { "@matrixai/async-cancellable": "^1.1.1", @@ -57,11 +57,11 @@ "typescript": "^5.1.6" }, "optionalDependencies": { - "@matrixai/quic-darwin-arm64": "1.3.12", - "@matrixai/quic-darwin-universal": "1.3.12", - "@matrixai/quic-darwin-x64": "1.3.12", - "@matrixai/quic-linux-x64": "1.3.12", - "@matrixai/quic-win32-x64": "1.3.12" + "@matrixai/quic-darwin-arm64": "1.3.13", + "@matrixai/quic-darwin-universal": "1.3.13", + "@matrixai/quic-darwin-x64": "1.3.13", + "@matrixai/quic-linux-x64": "1.3.13", + "@matrixai/quic-win32-x64": "1.3.13" } }, "node_modules/@aashutoshrathi/word-wrap": { @@ -1469,67 +1469,6 @@ "resolved": "https://registry.npmjs.org/@matrixai/logger/-/logger-3.1.2.tgz", "integrity": "sha512-nNliLCnbg6hGS2+gGtQfeeyVNJzOmvqz90AbrQsHPNiE08l3YsENL2JQt9d454SorD1Ud51ymZdDCkeMLWY93A==" }, - "node_modules/@matrixai/quic-darwin-arm64": { - "version": "1.3.12", - "resolved": "https://registry.npmjs.org/@matrixai/quic-darwin-arm64/-/quic-darwin-arm64-1.3.12.tgz", - "integrity": "sha512-GBLJWYHT4XXA7ZuToZGLHhTYqeKC2KvKfHjWFeO9Fz3aDcFKBTUdN+QiQZFA3N6YtDme6+xBb5qxGvrNPJgHng==", - "cpu": [ - "arm64" - ], - "optional": true, - "os": [ - "darwin" - ] - }, - "node_modules/@matrixai/quic-darwin-universal": { - "version": "1.3.12", - "resolved": "https://registry.npmjs.org/@matrixai/quic-darwin-universal/-/quic-darwin-universal-1.3.12.tgz", - "integrity": "sha512-Tq2Tjp3Pkg293B0Ib7Owds2wzEfYcbkIr4VfrhZ3yCVVWXmC/DSQ23Bd+l9MIGo03/NYUQeZwJ0KuexthERqXA==", - "cpu": [ - "x64", - "arm64" - ], - "optional": true, - "os": [ - "darwin" - ] - }, - "node_modules/@matrixai/quic-darwin-x64": { - "version": "1.3.12", - "resolved": "https://registry.npmjs.org/@matrixai/quic-darwin-x64/-/quic-darwin-x64-1.3.12.tgz", - "integrity": "sha512-uw/jesb4oJoVcsqBVewvu47XVLdffOUGnHPV+m1XNWHaClUKzYJXHRCtwsnlyf2DltzHPBf2Vu+jbkLZcuovnQ==", - "cpu": [ - "x64" - ], - "optional": true, - "os": [ - "darwin" - ] - }, - "node_modules/@matrixai/quic-linux-x64": { - "version": "1.3.12", - "resolved": "https://registry.npmjs.org/@matrixai/quic-linux-x64/-/quic-linux-x64-1.3.12.tgz", - "integrity": "sha512-u+qXouRfZ+h4iLksj/fD3DkWFGFyVl8lQbfKQs0W0mv0U0P9+gYIBNvMIqDn65da7Z8X8jGpFWcATgsBAbVLRQ==", - "cpu": [ - "x64" - ], - "optional": true, - "os": [ - "linux" - ] - }, - "node_modules/@matrixai/quic-win32-x64": { - "version": "1.3.12", - "resolved": "https://registry.npmjs.org/@matrixai/quic-win32-x64/-/quic-win32-x64-1.3.12.tgz", - "integrity": "sha512-c+x0ZItcrZOUE1ZgGuhiN9AH5KWJCIsmGQbFfkVko6NVI0bDiTIwkqmUA6JbSBCH8JD4jQqnaggMRc4kZmlMsA==", - "cpu": [ - "x64" - ], - "optional": true, - "os": [ - "win32" - ] - }, "node_modules/@matrixai/resources": { "version": "1.1.5", "resolved": "https://registry.npmjs.org/@matrixai/resources/-/resources-1.1.5.tgz", diff --git a/package.json b/package.json index e12210af..d3b7b806 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@matrixai/quic", - "version": "1.3.12", + "version": "1.3.13", "author": "Matrix AI", "contributors": [ { @@ -48,11 +48,11 @@ "ip-num": "^1.5.0" }, "optionalDependencies": { - "@matrixai/quic-darwin-arm64": "1.3.12", - "@matrixai/quic-darwin-universal": "1.3.12", - "@matrixai/quic-darwin-x64": "1.3.12", - "@matrixai/quic-linux-x64": "1.3.12", - "@matrixai/quic-win32-x64": "1.3.12" + "@matrixai/quic-darwin-arm64": "1.3.13", + "@matrixai/quic-darwin-universal": "1.3.13", + "@matrixai/quic-darwin-x64": "1.3.13", + "@matrixai/quic-linux-x64": "1.3.13", + "@matrixai/quic-win32-x64": "1.3.13" }, "devDependencies": { "@fast-check/jest": "^1.1.0",