Skip to content

Commit bd8f033

Browse files
tegefaulkestegefaulkes
committed
fix: properly handling duplicate stream ids if ids are used out of order
1 parent 267e2a1 commit bd8f033

File tree

3 files changed

+273
-17
lines changed

3 files changed

+273
-17
lines changed

‎package-lock.json‎

Lines changed: 61 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/QUICConnection.ts‎

Lines changed: 84 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -107,22 +107,29 @@ class QUICConnection {
107107
/**
108108
* Tracks the highest StreamId that has a created QUICStream for clientBidi
109109
*/
110-
protected streamIdUsedClientBidi = -1 as StreamId;
110+
protected streamIdUsedClientBidi = (0b00 - 4) as StreamId;
111111

112112
/**
113113
* Tracks the highest StreamId that has a created QUICStream for serverBidi
114114
*/
115-
protected streamIdUsedServerBidi = -1 as StreamId;
115+
protected streamIdUsedServerBidi = (0b01 - 4) as StreamId;
116116

117117
/**
118118
* Tracks the highest StreamId that has a created QUICStream for clientUni
119119
*/
120-
protected streamIdUsedClientUni = -1 as StreamId;
120+
protected streamIdUsedClientUni = (0b10 - 4) as StreamId;
121121

122122
/**
123123
* Tracks the highest StreamId that has a created QUICStream for clientUni
124124
*/
125-
protected streamIdUsedServerUni = -1 as StreamId;
125+
protected streamIdUsedServerUni = (0b11 - 4) as StreamId;
126+
127+
/**
128+
* Tracks used ids that have skipped the expected next id for the streamIdUsed counters.
129+
* If the next id in the streamIdUsed sequence is used then we remove IDs from the Set
130+
* up to the next ID gap.
131+
*/
132+
protected streamIdUsedSet: Set<number> = new Set();
126133

127134
/**
128135
* Quiche connection timer. This performs time delayed state transitions.
@@ -988,24 +995,85 @@ class QUICConnection {
988995
}
989996

990997
protected isStreamUsed(streamId: StreamId): boolean {
998+
let nextId: number;
991999
const type = 0b11 & streamId;
9921000
switch (type) {
9931001
case 0b00:
994-
if (streamId <= this.streamIdUsedClientBidi) return true;
995-
this.streamIdUsedClientBidi = streamId;
996-
return false;
1002+
nextId = this.streamIdUsedClientBidi + 4;
1003+
if (
1004+
streamId <= this.streamIdUsedClientBidi ||
1005+
this.streamIdUsedSet.has(streamId)
1006+
) {
1007+
return true;
1008+
} else if (streamId === nextId) {
1009+
// Increase counter and check set in loop.
1010+
do {
1011+
this.streamIdUsedClientBidi = nextId as StreamId;
1012+
this.streamIdUsedSet.delete(nextId);
1013+
nextId += 4;
1014+
} while (this.streamIdUsedSet.has(nextId));
1015+
return false;
1016+
} else {
1017+
this.streamIdUsedSet.add(streamId);
1018+
return false;
1019+
}
9971020
case 0b01:
998-
if (streamId <= this.streamIdUsedServerBidi) return true;
999-
this.streamIdUsedServerBidi = streamId;
1000-
return false;
1021+
nextId = this.streamIdUsedServerBidi + 4;
1022+
if (
1023+
streamId <= this.streamIdUsedServerBidi ||
1024+
this.streamIdUsedSet.has(streamId)
1025+
) {
1026+
return true;
1027+
} else if (streamId === nextId) {
1028+
// Increase counter and check set in loop.
1029+
do {
1030+
this.streamIdUsedServerBidi = nextId as StreamId;
1031+
this.streamIdUsedSet.delete(nextId);
1032+
nextId += 4;
1033+
} while (this.streamIdUsedSet.has(nextId));
1034+
return false;
1035+
} else {
1036+
this.streamIdUsedSet.add(streamId);
1037+
return false;
1038+
}
10011039
case 0b10:
1002-
if (streamId <= this.streamIdUsedClientUni) return true;
1003-
this.streamIdUsedClientUni = streamId;
1004-
return false;
1040+
nextId = this.streamIdUsedClientUni + 4;
1041+
if (
1042+
streamId <= this.streamIdUsedClientUni ||
1043+
this.streamIdUsedSet.has(streamId)
1044+
) {
1045+
return true;
1046+
} else if (streamId === nextId) {
1047+
// Increase counter and check set in loop.
1048+
do {
1049+
this.streamIdUsedClientUni = nextId as StreamId;
1050+
this.streamIdUsedSet.delete(nextId);
1051+
nextId += 4;
1052+
} while (this.streamIdUsedSet.has(nextId));
1053+
return false;
1054+
} else {
1055+
this.streamIdUsedSet.add(streamId);
1056+
return false;
1057+
}
10051058
case 0b11:
1006-
if (streamId <= this.streamIdUsedServerUni) return true;
1007-
this.streamIdUsedServerUni = streamId;
1008-
return false;
1059+
nextId = this.streamIdUsedServerUni + 4;
1060+
if (
1061+
streamId <= this.streamIdUsedServerUni ||
1062+
this.streamIdUsedSet.has(streamId)
1063+
) {
1064+
return true;
1065+
} else if (streamId === nextId) {
1066+
// Increase counter and check set in loop.
1067+
do {
1068+
this.streamIdUsedServerUni = nextId as StreamId;
1069+
this.streamIdUsedSet.delete(nextId);
1070+
nextId += 4;
1071+
} while (this.streamIdUsedSet.has(nextId));
1072+
return false;
1073+
} else {
1074+
this.streamIdUsedSet.add(streamId);
1075+
return false;
1076+
}
10091077
default:
10101078
utils.never('got an unexpected ID type');
10111079
}

‎tests/QUICStream.test.ts‎

Lines changed: 128 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
1-
import type { ClientCryptoOps, QUICConnection, ServerCryptoOps } from '@';
1+
import type {
2+
ClientCryptoOps,
3+
QUICConnection,
4+
ServerCryptoOps,
5+
StreamId,
6+
} from '@';
27
import Logger, { formatting, LogLevel, StreamHandler } from '@matrixai/logger';
38
import { destroyed } from '@matrixai/async-init';
9+
import { test, fc } from '@fast-check/jest';
410
import * as events from '@/events';
511
import * as errors from '@/errors';
612
import * as utils from '@/utils';
@@ -2246,4 +2252,125 @@ describe(QUICStream.name, () => {
22462252
await client.destroy({ force: true });
22472253
await server.stop({ force: true });
22482254
});
2255+
test.prop(
2256+
[
2257+
fc
2258+
.array(fc.integer({ min: 1 }), { minLength: 1000, maxLength: 2000 })
2259+
.noShrink(),
2260+
],
2261+
{ numRuns: 1 },
2262+
)('out of order Ids are handled properly', async (arr) => {
2263+
const size = arr.length;
2264+
const used: Set<number> = new Set();
2265+
const ids: Array<number> = [];
2266+
for (let num of arr) {
2267+
do {
2268+
num = (num + 1) % size;
2269+
} while (used.has(num));
2270+
ids.push(num);
2271+
used.add(num);
2272+
}
2273+
2274+
const connectionEventProm =
2275+
utils.promise<events.EventQUICServerConnection>();
2276+
const tlsConfig = await generateTLSConfig(defaultType);
2277+
const server = new QUICServer({
2278+
crypto: {
2279+
key,
2280+
ops: serverCrypto,
2281+
},
2282+
logger: logger.getChild(QUICServer.name),
2283+
config: {
2284+
key: tlsConfig.leafKeyPairPEM.privateKey,
2285+
cert: tlsConfig.leafCertPEM,
2286+
verifyPeer: false,
2287+
},
2288+
});
2289+
socketCleanMethods.extractSocket(server);
2290+
server.addEventListener(
2291+
events.EventQUICServerConnection.name,
2292+
(e: events.EventQUICServerConnection) => connectionEventProm.resolveP(e),
2293+
);
2294+
await server.start({
2295+
host: localhost,
2296+
});
2297+
const client = await QUICClient.createQUICClient({
2298+
host: localhost,
2299+
port: server.port,
2300+
localHost: localhost,
2301+
crypto: {
2302+
ops: clientCrypto,
2303+
},
2304+
logger: logger.getChild(QUICClient.name),
2305+
config: {
2306+
verifyPeer: false,
2307+
},
2308+
});
2309+
socketCleanMethods.extractSocket(client);
2310+
await connectionEventProm.p;
2311+
2312+
const checkId = (id: StreamId): boolean => {
2313+
// @ts-ignore: Using protected method
2314+
return client.connection.isStreamUsed(id);
2315+
};
2316+
2317+
for (const id of ids) {
2318+
expect(checkId(id as StreamId)).toBeFalse();
2319+
}
2320+
// @ts-ignore: using protected property
2321+
const usedIdSet = client.connection.streamIdUsedSet;
2322+
expect(usedIdSet.size).toBe(0);
2323+
});
2324+
test('out of order Ids are handled properly', async () => {
2325+
const connectionEventProm =
2326+
utils.promise<events.EventQUICServerConnection>();
2327+
const tlsConfig = await generateTLSConfig(defaultType);
2328+
const server = new QUICServer({
2329+
crypto: {
2330+
key,
2331+
ops: serverCrypto,
2332+
},
2333+
logger: logger.getChild(QUICServer.name),
2334+
config: {
2335+
key: tlsConfig.leafKeyPairPEM.privateKey,
2336+
cert: tlsConfig.leafCertPEM,
2337+
verifyPeer: false,
2338+
},
2339+
});
2340+
socketCleanMethods.extractSocket(server);
2341+
server.addEventListener(
2342+
events.EventQUICServerConnection.name,
2343+
(e: events.EventQUICServerConnection) => connectionEventProm.resolveP(e),
2344+
);
2345+
await server.start({
2346+
host: localhost,
2347+
});
2348+
const client = await QUICClient.createQUICClient({
2349+
host: localhost,
2350+
port: server.port,
2351+
localHost: localhost,
2352+
crypto: {
2353+
ops: clientCrypto,
2354+
},
2355+
logger: logger.getChild(QUICClient.name),
2356+
config: {
2357+
verifyPeer: false,
2358+
},
2359+
});
2360+
socketCleanMethods.extractSocket(client);
2361+
await connectionEventProm.p;
2362+
2363+
const checkId = (id: StreamId): boolean => {
2364+
// @ts-ignore: Using protected method
2365+
return client.connection.isStreamUsed(id);
2366+
};
2367+
2368+
expect(checkId(0 as StreamId)).toBeFalse();
2369+
expect(checkId(4 as StreamId)).toBeFalse();
2370+
expect(checkId(8 as StreamId)).toBeFalse();
2371+
expect(checkId(4 as StreamId)).toBeTrue();
2372+
expect(checkId(16 as StreamId)).toBeFalse();
2373+
expect(checkId(0 as StreamId)).toBeTrue();
2374+
expect(checkId(0 as StreamId)).toBeTrue();
2375+
});
22492376
});

0 commit comments

Comments
 (0)