Skip to content

Commit 5fce16d

Browse files
committed
fix: fixed problem where buffers read from the readable stream shared the same memory
This caused a problem where the buffer contents were overwritten by the next message emitted. So if you held onto the buffers their contents would change. This was fixed by using `Buffer.from()` to copy the slice. There is a small performance hit to doing this.
1 parent 42cd89e commit 5fce16d

File tree

2 files changed

+97
-2
lines changed

2 files changed

+97
-2
lines changed

src/QUICStream.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,8 @@ class QUICStream implements ReadableWritablePair<Uint8Array, Uint8Array> {
475475
const [recvLength] = result;
476476
if (recvLength > 0) {
477477
this.readableController.enqueue(
478-
this.readableChunk!.subarray(0, recvLength),
478+
// Making a copy to be enqueued
479+
Buffer.from(this.readableChunk!.slice(0, recvLength)),
479480
);
480481
}
481482
this.readableController.close();
@@ -641,7 +642,8 @@ class QUICStream implements ReadableWritablePair<Uint8Array, Uint8Array> {
641642
const [recvLength, fin] = result;
642643
if (recvLength > 0) {
643644
this.readableController.enqueue(
644-
this.readableChunk!.subarray(0, recvLength),
645+
// Making a copy to be enqueued
646+
Buffer.from(this.readableChunk!.slice(0, recvLength)),
645647
);
646648
}
647649
if (fin) {

tests/QUICStream.test.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,99 @@ describe(QUICStream.name, () => {
270270
await client.destroy({ force: true });
271271
await server.stop({ force: true });
272272
});
273+
test('sent data should be correct and expected', async () => {
274+
const messages = [
275+
'The',
276+
'Quick',
277+
'Brown',
278+
'Fox',
279+
'Jumped',
280+
'Over',
281+
'The',
282+
'Lazy',
283+
'Dog',
284+
].map((v) => Buffer.from(v));
285+
const connectionEventProm =
286+
utils.promise<events.EventQUICServerConnection>();
287+
const tlsConfig = await generateTLSConfig(defaultType);
288+
const server = new QUICServer({
289+
crypto: {
290+
key,
291+
ops: serverCrypto,
292+
},
293+
logger: logger.getChild(QUICServer.name),
294+
config: {
295+
key: tlsConfig.leafKeyPairPEM.privateKey,
296+
cert: tlsConfig.leafCertPEM,
297+
verifyPeer: false,
298+
},
299+
});
300+
socketCleanMethods.extractSocket(server);
301+
server.addEventListener(
302+
events.EventQUICServerConnection.name,
303+
(e: events.EventQUICServerConnection) => connectionEventProm.resolveP(e),
304+
);
305+
await server.start({
306+
host: localhost,
307+
});
308+
const client = await QUICClient.createQUICClient({
309+
host: localhost,
310+
port: server.port,
311+
localHost: localhost,
312+
crypto: {
313+
ops: clientCrypto,
314+
},
315+
logger: logger.getChild(QUICClient.name),
316+
config: {
317+
verifyPeer: false,
318+
},
319+
});
320+
socketCleanMethods.extractSocket(client);
321+
const conn = (await connectionEventProm.p).detail;
322+
// Do the test
323+
const streamProm = utils.promise<QUICStream>();
324+
conn.addEventListener(
325+
events.EventQUICConnectionStream.name,
326+
(streamEvent: events.EventQUICConnectionStream) => {
327+
streamProm.resolveP(streamEvent.detail);
328+
},
329+
{ once: true },
330+
);
331+
332+
// Create a stream
333+
const streamLocal = client.connection.newStream();
334+
const writerLocal = streamLocal.writable.getWriter();
335+
for (const message of messages) {
336+
await writerLocal.write(message);
337+
}
338+
await writerLocal.close();
339+
const streamPeer = await streamProm.p;
340+
const writerPeer = streamPeer.writable.getWriter();
341+
for (const message of messages) {
342+
await writerPeer.write(message);
343+
}
344+
await writerPeer.close();
345+
346+
const readMessagesLocal: Array<any> = [];
347+
const readMessagesPeer: Array<any> = [];
348+
for await (const chunk of streamPeer.readable) {
349+
readMessagesLocal.push(chunk);
350+
}
351+
for await (const chunk of streamLocal.readable) {
352+
readMessagesPeer.push(chunk);
353+
}
354+
355+
const expected = messages.map((v) => Buffer.from(v)).join(' ');
356+
expect(readMessagesLocal.map((v) => Buffer.from(v)).join(' ')).toBe(
357+
expected,
358+
);
359+
expect(readMessagesPeer.map((v) => Buffer.from(v)).join(' ')).toBe(
360+
expected,
361+
);
362+
363+
await client.destroy({ force: true });
364+
await server.stop({ force: true });
365+
});
273366
test('should propagate errors over stream for writable', async () => {
274367
const tlsConfig = await generateTLSConfig(defaultType);
275368
const server = new QUICServer({

0 commit comments

Comments
 (0)