quic: crash when incoming unidirectional stream has no onstream handler #64030
Description
Activity
I turned it into a test:
// Flags: --experimental-quic --no-warnings // Test: client-initiated unidirectional stream with no onstream // The client creates a uni stream with no onstream. // Verify, that this causes no crash import { hasQuic, skip, mustCall } from '../common/index.mjs'; import {readFileSync} from 'node:fs'; import {createPrivateKey} from 'node:crypto'; import * as fixtures from '../common/fixtures.mjs'; const { readKey } = fixtures; if (!hasQuic) { skip('QUIC is not enabled'); } const { listen, connect } = await import('../common/quic.mjs'); const serverKey = createPrivateKey(readKey('agent1-key.pem')); const serverCert = readKey('agent1-cert.pem'); const serverEndpoint = await listen(mustCall(async (session) => { await session.opened; console.log('Server session opened'); // Intentionally do not install .onstream }), { sni: {'*': {keys: [serverKey], certs: [serverCert]}}, alpn: ['repro'], }); const clientSession = await connect(serverEndpoint.address, { servername: 'localhost', alpn: 'repro', ca: serverCert, }); await clientSession.opened; console.log('Client session opened'); await clientSession.createUnidirectionalStream({ body: 'something something darkside' }); console.log('Unidirectional stream created');and tried to reproduce it on windows, but no luck. So I expect that it is a race condition or uninitialized memory, which may be system depent. However, my local main is still on martenrichter@a17d2b8 from yesterday, so may be it is a very recent commit. I try to update and test again.
Does not crash either... on current main.
My best bet is that something in
Stream::Destroy:
Line 1641 in 665daeb
crashes, where something is not protected against a unidirectional stream that has no write side.
So as you can reproduce it locally, add some printf in this function to narrow down the crash. As it does not crash on my install I can not help with the debugging part.My best bet is that something in
Stream::Destroy:
...
So as you can reproduce it locally, add some printf in this function to narrow down the crash. As it does not crash on my install I can not help with the debugging part.Thanks for the input!
I've been debugging this over the last few days, as far as I see this is caused by JS synchronously destroying the stream and native code trying to access it. Weird you couldn't reproduce it though, it is 100% reproducable for me.
Thanks for the input!
I've been debugging this over the last few days, as far as I see this is caused by JS synchronously destroying the stream and native code trying to access it. Weird you couldn't reproduce it though, it is 100% reproducable for me.
So that means something on the native side is not holding a reference to JS long enough.
(You may want to look at my old PR #60237, the code basis change a bit since then, what I had fixed there many bugs like this, so if one of the fix is close to the parts you are debugging, it may be a match).And where are the crashes happening?
And it is not so weird, that I can not repoduce it. These type of race condition triggered crashes depend on the timing, and also the timing of network packets. And Windows uses a different compiler, so memory is differently initialized. So it is often machine and OS dependent.
[Edit: I see you already made a PR, was not clear to me when writing the above]
[Edit: I see you already made a PR, was not clear to me when writing the above]
Yeah, I have a PR open, but I appreciate the input. I'm trying to get familiar with QUIC and it's implementation in Node.js, so any extra guidance helps :)
When a QUIC server accepts a session without
.onstream, and the client connects with a unidirectional stream with a body, Node crashes withSIGSEGVIf I use a bidirectional stream instead, it works. If I create the unidirectional stream without a body, it also does not crash.
Expected behaviour would be to safely destroy or at least not crash.
Repro:
Outputs