a73x

937190ce

refactor: the QUIC layer keeps the constraint, drops the crash report

a73x   2026-08-30 19:49

Commit message
refactor: the QUIC layer keeps the constraint, drops the crash report

50 essays to 9. The ngtcp2 rules that shape this code — payload is not
copied, a blocked stream is not a dead one, both windows extend on
consume — still say what breaks. The paragraphs tracing which source line
in ngtcp2_conn.c faulted, how often, and on which loopback run are gone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XSUFuYHqU9wr4J5NC8EkWV
docscheck.blocks
Old New
@@ -21,8 +21,8 @@ predict.zig 15
21 protocol.zig 13 21 protocol.zig 13
22 proxy.zig 1 22 proxy.zig 1
23 pty.zig 16 23 pty.zig 16
24 quic_server.zig 30 24 quic_server.zig 3
25 quic.zig 20 25 quic.zig 6
26 replica.zig 3 26 replica.zig 3
27 select.zig 4 27 select.zig 4
28 server_agent.zig 8 28 server_agent.zig 8
src/quic.zig
Old New
@@ -1,19 +1,12 @@
1 //! The QUIC vocabulary both ends share: the C import, the pre-shared key, 1 //! The QUIC vocabulary both ends share: the C import, the pre-shared key, the
2 //! the wire constants, and the egress ring's lifetime discipline. 2 //! wire constants, and the egress ring's lifetime discipline. Nothing here
3 //! knows what a LISTENER is — quic_server.zig owns that and imports this. The
4 //! dialling end lives here because the listener's own tests need a real peer
5 //! and the folder rule forbids the server naming a client module.
3 //! 6 //!
4 //! Nothing here knows what a LISTENER is: quic_server.zig owns that, and 7 //! THE OWNERSHIP RULE: this file holds the one and only `@cImport` of the QUIC
5 //! imports this — which is what makes "the two ends agree" a fact about one 8 //! stack. Two blocks over the same headers produce two DISTINCT Zig types, and
6 //! file rather than a convention two files are trusted to keep. The dialling 9 //! the egress ring hands `ngtcp2_vec`s across the seam between them.
7 //! end lives here, below the shared vocabulary, because both the client and
8 //! the listener's own tests need a real peer and the folder rule forbids the
9 //! server naming a client module.
10 //!
11 //! THE OWNERSHIP RULE, because it is load-bearing rather than tidy: this
12 //! file holds the one and only @cImport of the QUIC stack. Two blocks over
13 //! the same headers produce two *distinct* Zig types, so an `ngtcp2_vec`
14 //! minted under one would not be an `ngtcp2_vec` to the other — and the
15 //! egress ring hands exactly those across the seam between listener and
16 //! client. A new QUIC file therefore imports this one; it never cImports.
17 const std = @import("std"); 10 const std = @import("std");
18 11
19 /// The C view of the QUIC stack. Exported because the client transport 12 /// The C view of the QUIC stack. Exported because the client transport
@@ -36,20 +29,11 @@ pub const c = @cImport({
36 /// binaries already import. 29 /// binaries already import.
37 pub const default_port: u16 = 4433; 30 pub const default_port: u16 = 4433;
38 31
39 /// How long a QUIC connection tolerates silence before declaring the peer 32 /// How long a QUIC connection tolerates silence before declaring the peer gone.
40 /// gone. Long enough that a quiet terminal is not a suspicious one, short 33 /// Keepalives run at a third of it, so an idle session never trips it.
41 /// enough that a client which has genuinely vanished stops being served 34 /// `--quic-idle-ms` tunes it wherever a human dials or listens, because the
42 /// within a few seconds of keepalives failing — keepalives run at a third 35 /// reconnect tests need death declared on a schedule they can wait for; `mux a`
43 /// of it, so an idle session is never the thing that trips it. 36 /// has no such flag, since `--timeout` already bounds an agent's wait.
44 /// `--quic-idle-ms` tunes it on the three binaries that dial or listen for
45 /// a human — every mode of `mux` — because the reconnect tests need
46 /// death declared on a schedule they can wait for. `mux a` deliberately has
47 /// no such flag and always takes this default: an agent's wait is bounded
48 /// by `--timeout` already, and a second knob over the same wait is one
49 /// more thing for a driver to get wrong.
50 ///
51 /// Here for the same reason as `default_port`: two copies of a number both
52 /// binaries default to are two numbers, and they drift in silence.
53 pub const default_idle_ms: u32 = 15_000; 37 pub const default_idle_ms: u32 = 15_000;
54 38
55 /// Zero means NO idle timeout to ngtcp2, the inverse of what anyone typing 39 /// Zero means NO idle timeout to ngtcp2, the inverse of what anyone typing
@@ -83,13 +67,10 @@ pub fn parseAddr(alloc: std.mem.Allocator, host_port: []const u8) !std.net.Addre
83 return resolveHost(alloc, hp.host, hp.port); 67 return resolveHost(alloc, hp.host, hp.port);
84 } 68 }
85 69
86 /// The grammar half, with nothing resolved: `HOST[:PORT]` as a client 70 /// The grammar half, nothing resolved: `HOST[:PORT]` as a client types it,
87 /// types it, where an omitted port means `default_port` and an empty one 71 /// where an omitted port means `default_port` and an empty one is a mistake.
88 /// (`127.0.0.1:`) is still a mistake. `mux quic://HOST:PORT`, 72 /// Every spelling of `--quic` reads this — an agent, a human and the box they
89 /// `mux a --quic HOST:PORT` and the daemon's own `--quic` bind address all 73 /// point at must type the same thing, and two grammars are two dialects.
90 /// read exactly these spellings — an agent, a human and the box they point
91 /// at must be able to type the same thing, and two copies of a grammar
92 /// drift into two dialects of one flag.
93 pub fn splitHostPort(s: []const u8) !struct { host: []const u8, port: u16 } { 74 pub fn splitHostPort(s: []const u8) !struct { host: []const u8, port: u16 } {
94 if (s.len > 0 and s[0] == '[') { 75 if (s.len > 0 and s[0] == '[') {
95 // Bracketed: the brackets say where the address stops, so the port 76 // Bracketed: the brackets say where the address stops, so the port
@@ -102,12 +83,9 @@ pub fn splitHostPort(s: []const u8) !struct { host: []const u8, port: u16 } {
102 } 83 }
103 const colon = std.mem.lastIndexOfScalar(u8, s, ':') orelse 84 const colon = std.mem.lastIndexOfScalar(u8, s, ':') orelse
104 return .{ .host = s, .port = default_port }; 85 return .{ .host = s, .port = default_port };
105 // An unbracketed IPv6 literal carries colons of its own, and splitting 86 // An unbracketed IPv6 literal carries its own colons: `fe80::1:4433` reads
106 // on the last one would quietly take its final group as a port: 87 // equally well as a host with a port and as a host without one. Brackets
107 // `fe80::1:4433` reads equally well as host `fe80::1` port 4433 and as 88 // spell the ambiguity out, so without them it is refused, not guessed.
108 // host `fe80::1:4433` with the port left off. Brackets are how that
109 // ambiguity is spelled out, so without them it is refused rather than
110 // guessed at.
111 if (std.mem.indexOfScalar(u8, s[0..colon], ':') != null) return error.MalformedAddress; 89 if (std.mem.indexOfScalar(u8, s[0..colon], ':') != null) return error.MalformedAddress;
112 return .{ .host = s[0..colon], .port = try parsePort(s[colon + 1 ..]) }; 90 return .{ .host = s[0..colon], .port = try parsePort(s[colon + 1 ..]) };
113 } 91 }
@@ -141,14 +119,9 @@ test "splitHostPort: literal addresses, bracketed and not" {
141 try std.testing.expectEqualStrings("::", any6.host); 119 try std.testing.expectEqualStrings("::", any6.host);
142 try std.testing.expectEqual(@as(u16, 1), any6.port); 120 try std.testing.expectEqual(@as(u16, 1), any6.port);
143 121
144 // No port names the default. 4433 is mux's convention; an explicit 122 // No port names the default. Spelled out rather than written
145 // port always wins. 123 // `quic.default_port`: comparing the parse's answer against the constant the
146 // 124 // parse reads holds for ANY value, and 4433 is what both ends must agree on.
147 // The number is spelled out rather than written `quic.default_port`:
148 // comparing the parse's answer against the same constant the parse
149 // reads holds for ANY value, so it would pin the wiring and say
150 // nothing about the port — and 4433 is the half both ends of a
151 // connection have to agree on.
152 const dflt = try splitHostPort("127.0.0.1"); 125 const dflt = try splitHostPort("127.0.0.1");
153 try std.testing.expectEqualStrings("127.0.0.1", dflt.host); 126 try std.testing.expectEqualStrings("127.0.0.1", dflt.host);
154 try std.testing.expectEqual(@as(u16, 4433), dflt.port); 127 try std.testing.expectEqual(@as(u16, 4433), dflt.port);
@@ -178,10 +151,8 @@ test "parseAddr: literals, brackets, and the spellings that are refused" {
178 try std.testing.expectEqual(@as(u16, 9), (try parseAddr(alloc, "127.0.0.1:9")).getPort()); 151 try std.testing.expectEqual(@as(u16, 9), (try parseAddr(alloc, "127.0.0.1:9")).getPort());
179 152
180 // An omitted port means mux's own. Spelled out rather than written 153 // An omitted port means mux's own. Spelled out rather than written
181 // `default_port`, because comparing the parse's answer against the 154 // `default_port`, because comparing against the constant the parse reads
182 // constant the parse reads would hold for any value and say nothing 155 // would hold for any value — and this is the number the daemon must agree on.
183 // about the port — and this is the number the daemon at the other end
184 // has to agree on.
185 try std.testing.expectEqual(@as(u16, 4433), (try parseAddr(alloc, "10.0.0.2")).getPort()); 156 try std.testing.expectEqual(@as(u16, 4433), (try parseAddr(alloc, "10.0.0.2")).getPort());
186 157
187 const six = try parseAddr(alloc, "[::1]:9999"); 158 const six = try parseAddr(alloc, "[::1]:9999");
@@ -207,12 +178,9 @@ test "parseAddr: literals, brackets, and the spellings that are refused" {
207 178
208 pub const key_len = 32; 179 pub const key_len = 32;
209 180
210 /// The pre-shared key, and the rules for getting one off disk. 181 /// The pre-shared key, and the rules for getting one off disk. A key file is as
211 /// 182 /// sensitive as an ssh private key and held to the same standard: readable by
212 /// A key file is exactly as sensitive as an ssh private key, so it is held 183 /// nobody but its owner. A daemon that starts anyway has authenticated nothing.
213 /// to the same standard: readable by nobody but its owner. Refusing is the
214 /// whole point — a daemon that starts anyway with a world-readable key has
215 /// authenticated nothing, and would do it silently.
216 pub const Key = struct { 184 pub const Key = struct {
217 bytes: [key_len]u8, 185 bytes: [key_len]u8,
218 186
@@ -240,13 +208,9 @@ pub const Key = struct {
240 var buf: [128]u8 = undefined; 208 var buf: [128]u8 = undefined;
241 const n = try file.readAll(&buf); 209 const n = try file.readAll(&buf);
242 210
243 // A file of exactly 32 bytes is a raw key, taken WITHOUT trimming. 211 // A file of exactly 32 bytes is a raw key, taken WITHOUT trimming: a
244 // Trimming first looks harmless and is not: a random key ends in one 212 // random key ends in one of " \t\r\n" about one time in 64, and trimming
245 // of " \t\r\n" about one time in 64, and such keys were silently 213 // shortens those to 31 bytes and rejects them as malformed.
246 // shortened to 31 bytes and rejected as malformed — so
247 // `head -c 32 /dev/urandom > k` failed for a few percent of the keys
248 // it generated. Rare, random, and looking like the user's fault,
249 // which is the worst shape a bug can have. Found by review probe.
250 if (n == key_len) { 214 if (n == key_len) {
251 var k: Key = undefined; 215 var k: Key = undefined;
252 @memcpy(&k.bytes, buf[0..key_len]); 216 @memcpy(&k.bytes, buf[0..key_len]);
@@ -270,28 +234,16 @@ pub const Key = struct {
270 } 234 }
271 }; 235 };
272 236
273 /// A buffer for one refusal body: PATH_MAX for the path, and 128 for 237 /// A buffer for one refusal body: PATH_MAX for the path, 128 for the rest. The
274 /// everything else in the line. 238 /// 128 is chosen, not derived, because the catch-all appends an `@errorName`
275 /// 239 /// whose only bound is the longest error name in the binary. `keyRefusalBody`'s
276 /// The 128 is chosen, not derived, and the honest reason is the catch-all. 240 /// clipping is what makes choosing rather than deriving safe.
277 /// The longest fixed text is 53 bytes (` is not a key: want 32 raw bytes
278 /// or 64 hex characters`), which would size itself — but the catch-all
279 /// also appends an `@errorName`, and the only bound on that is the longest
280 /// error name in the binary, a number no source line here can name. So
281 /// this is `open_err_len`'s kind of number, picked to put truncation out
282 /// of reach, and it is `keyRefusalBody`'s clipping that makes choosing
283 /// rather than deriving safe.
284 pub const key_refusal_len = std.fs.max_path_bytes + 128; 241 pub const key_refusal_len = std.fs.max_path_bytes + 128;
285 242
286 /// The middle sentence of every key refusal, in every binary — one owner so 243 /// The middle sentence of every key refusal, in every binary — one owner so the
287 /// the catch-alls cannot drift apart. Callers add their own prefix and suffix. 244 /// catch-alls cannot drift. `err` is `anyerror` rather than `Key.LoadError`
288 /// 245 /// because `load` widens past its own set. Truncating rather than failing:
289 /// `err` is `anyerror` rather than `Key.LoadError` because `load` widens past 246 /// this line is the user's only account of the refusal.
290 /// its own set: a stat or a read that fails arrives here as a plain posix
291 /// error, and the catch-all is what those are for.
292 ///
293 /// Truncating rather than failing: this line is the user's only account of the
294 /// refusal, so a clipped one beats none.
295 pub fn keyRefusalBody(buf: []u8, err: anyerror, path: []const u8) []const u8 { 247 pub fn keyRefusalBody(buf: []u8, err: anyerror, path: []const u8) []const u8 {
296 var w: std.Io.Writer = .fixed(buf); 248 var w: std.Io.Writer = .fixed(buf);
297 switch (err) { 249 switch (err) {
@@ -417,18 +369,9 @@ test "Key.load: refuses a permissive mode, a missing file, and a bad length" {
417 } 369 }
418 370
419 test "keyRefusalBody: the words three binaries print, byte for byte" { 371 test "keyRefusalBody: the words three binaries print, byte for byte" {
420 // These bytes ARE the contract. Every key refusal any binary prints is 372 // These bytes ARE the contract: every key refusal any binary prints is a
421 // a prefix, this body, and at most a suffix: 373 // prefix, this body, and at most a suffix, so a change here changes every
422 // 374 // caller at once.
423 // mux d: <body> (mux d start --quic)
424 // mux d endpoint: <body>; staying on ssh
425 // mux d: endpoint_req: <body> (the daemon's lazy bind)
426 // mux: <body> (the client's dial)
427 // mux a: the JSON detail after "quic: unusable key"
428 //
429 // so a change here is a change to every caller at once — which is what
430 // the four literal copies this replaced could never guarantee, and did
431 // not: one of their catch-alls had drifted to a different verb.
432 var buf: [key_refusal_len]u8 = undefined; 375 var buf: [key_refusal_len]u8 = undefined;
433 try std.testing.expectEqualStrings( 376 try std.testing.expectEqualStrings(
434 "no such key file: /etc/mux/key", 377 "no such key file: /etc/mux/key",
@@ -464,20 +407,15 @@ pub const egress_cap = 256 * 1024;
464 /// Outbound stream bytes, in a ring that never moves a byte once written. 407 /// Outbound stream bytes, in a ring that never moves a byte once written.
465 /// 408 ///
466 /// A ring rather than a growable buffer, because ngtcp2 does NOT copy stream 409 /// A ring rather than a growable buffer, because ngtcp2 does NOT copy stream
467 /// payload: `ngtcp2_conn_writev_stream` stores the *vector* it is handed — 410 /// payload: `ngtcp2_conn_writev_stream` stores the VECTOR it is handed and
468 /// `ngtcp2_vec_copy` is a memcpy of base+len, not of the bytes — in its 411 /// re-reads those bytes if the packet is lost. A buffer that reallocates on
469 /// retransmission queue, and re-reads those bytes if the packet is lost. A 412 /// append, or clears once the last byte is handed over, leaves ngtcp2 holding a
470 /// buffer that reallocates on append, or that clears once the last byte has 413 /// recycled pointer and it dies inside `ngtcp2_pkt_encode_stream_frame`.
471 /// been handed over, therefore leaves ngtcp2 holding a freed or recycled
472 /// pointer, and it dies inside `ngtcp2_pkt_encode_stream_frame`. It did:
473 /// rarely, only under loss, and only in the one test big enough to overflow
474 /// a socket buffer — which is the worst shape a bug can have.
475 /// 414 ///
476 /// So the invariant, and everything here exists to hold it: **a byte handed 415 /// The invariant everything here exists to hold: a byte handed to ngtcp2 does
477 /// to ngtcp2 does not move or get overwritten until the peer acknowledges 416 /// not move or get overwritten until the peer acknowledges it. `head` advances
478 /// it.** `head` advances only from `acked_stream_data_offset`; a write can 417 /// only from `acked_stream_data_offset`. Fixed in size is the other half — a
479 /// only land in the free space that leaves. Being fixed in size is the other 418 /// full ring is the backpressure signal.
480 /// half of the point — a full ring is the backpressure signal.
481 pub const Egress = struct { 419 pub const Egress = struct {
482 buf: []u8, 420 buf: []u8,
483 /// The oldest byte the peer has not acknowledged. 421 /// The oldest byte the peer has not acknowledged.
@@ -566,11 +504,10 @@ pub fn accountWrite(out: *Egress, wrote: c.ngtcp2_ssize, n: c.ngtcp2_ssize) Writ
566 } 504 }
567 505
568 // --------------------------------------------------------------------------- 506 // ---------------------------------------------------------------------------
569 // The per-connection core. `Client` below and `Listener.Conn` across the seam 507 // The per-connection core. `Client` and `Listener.Conn` are different objects —
570 // are different objects — one dials, one accepts; one has a connected socket 508 // one dials and dies of a refusal, the other accepts and cannot be told — but
571 // and dies of a refusal, the other has an unconnected one and cannot be told 509 // the ngtcp2 they drive is the same library run the same way. What follows is
572 // — but the ngtcp2 they drive is the same library run the same way. What 510 // the parts where a difference would be a BUG, not a design.
573 // follows is that: the parts where a difference would be a BUG, not a design.
574 // --------------------------------------------------------------------------- 511 // ---------------------------------------------------------------------------
575 512
576 /// Milliseconds until ngtcp2 next wants servicing on `conn`, capped. A 513 /// Milliseconds until ngtcp2 next wants servicing on `conn`, capped. A
@@ -645,12 +582,9 @@ pub fn drainConn(
645 timestampNs(), 582 timestampNs(),
646 ); 583 );
647 if (n == c.NGTCP2_ERR_STREAM_DATA_BLOCKED or n == c.NGTCP2_ERR_STREAM_SHUT_WR) { 584 if (n == c.NGTCP2_ERR_STREAM_DATA_BLOCKED or n == c.NGTCP2_ERR_STREAM_SHUT_WR) {
648 // A documented return, not a failure: the peer has no window 585 // A documented return, not a failure: the peer has no window left.
649 // left for this stream. Treating it as fatal abandoned the whole 586 // Treating it as fatal abandons the ACKs and flow-control updates
650 // egress loop — including the ACKs and the flow-control updates 587 // that reopen that window, turning backpressure into a stall.
651 // that are how the peer's window reopens, which turns a moment
652 // of backpressure into a stall that never resolves. Retry the
653 // same iteration carrying no stream data.
654 stream_blocked = true; 588 stream_blocked = true;
655 continue; 589 continue;
656 } 590 }
@@ -724,11 +658,10 @@ test "accountWrite: bytes ngtcp2 committed are accounted even when the call fail
724 _ = e.push("abcdefgh"); 658 _ = e.push("abcdefgh");
725 try std.testing.expectEqual(@as(usize, 8), e.unsent); 659 try std.testing.expectEqual(@as(usize, 8), e.unsent);
726 660
727 // The case that has no fault injection available and would otherwise go 661 // ngtcp2 committed four bytes of stream data — its offset has moved — and
728 // untested: ngtcp2 committed four bytes of stream data — its offset has 662 // THEN returned an error. Accounting after the check leaves those four
729 // moved — and THEN returned an error. Accounting after the check would 663 // counted as unsent, and the next drain offers them at an offset the peer
730 // leave those four counted as unsent, so the next drain would offer them 664 // is already past.
731 // again at an offset the peer is already past.
732 try std.testing.expectEqual(WriteAction.stop, accountWrite(&e, 4, -1)); 665 try std.testing.expectEqual(WriteAction.stop, accountWrite(&e, 4, -1));
733 try std.testing.expectEqual(@as(usize, 4), e.unsent); 666 try std.testing.expectEqual(@as(usize, 4), e.unsent);
734 667
@@ -744,11 +677,9 @@ test "Egress: an ack against a torn-down ring is ignored, not a division by zero
744 _ = e.push("abcd"); 677 _ = e.push("abcd");
745 e.took(4); 678 e.took(4);
746 679
747 // Teardown leaves a zero-length buffer behind. No live path acks it 680 // Teardown leaves a zero-length buffer behind. No live path acks it today,
748 // today — both owners delete the ngtcp2 conn before freeing the ring, 681 // but the modulo in `ack` divides by `buf.len`, so a teardown reordering
749 // and ngtcp2 fires this callback only on ACK receipt — but the modulo 682 // would be a division by zero on a path nobody would look at.
750 // in `ack` divides by buf.len, so a teardown reordering would be a
751 // division by zero on a path nobody would think to look at.
752 e.deinit(alloc); 683 e.deinit(alloc);
753 try std.testing.expectEqual(@as(usize, 0), e.buf.len); 684 try std.testing.expectEqual(@as(usize, 0), e.buf.len);
754 e.ack(4); 685 e.ack(4);
@@ -800,11 +731,9 @@ pub fn getNewCidCb(
800 } 731 }
801 732
802 // --------------------------------------------------------------------------- 733 // ---------------------------------------------------------------------------
803 // The client side of the handshake, kept separate from `Client` below 734 // The client side of the handshake, kept separate from `Client` so the
804 // because the listener's tests once dialled with a second copy of it: a 735 // listener's tests can dial with it rather than a second copy: a drift between
805 // drift between the two failed the handshake with nothing to read but a 736 // two copies fails the handshake with nothing to read but a TLS alert.
806 // TLS alert. `Client` sets its own key, stream handlers and idle timeout
807 // on what these hand back.
808 // --------------------------------------------------------------------------- 737 // ---------------------------------------------------------------------------
809 738
810 /// Identity "mux", the key's bytes, our one ciphersuite: none is a choice. 739 /// Identity "mux", the key's bytes, our one ciphersuite: none is a choice.
@@ -903,37 +832,23 @@ pub fn pathFrom(
903 832
904 // --------------------------------------------------------------------------- 833 // ---------------------------------------------------------------------------
905 // The client transport: one UDP socket, one connection, one bidirectional 834 // The client transport: one UDP socket, one connection, one bidirectional
906 // stream of opaque bytes. It knows NOTHING about the frame protocol it 835 // stream of opaque bytes. It knows NOTHING about the frame protocol it carries.
907 // carries, which is why `protocol` is not imported here.
908 //
909 // Three ngtcp2 constraints shape it, and the listener across the seam obeys
910 // the same three:
911 // 836 //
912 // 1. **ngtcp2 does not copy stream payload.** It keeps the vector it is 837 // Three ngtcp2 constraints shape it, and the listener obeys the same three:
913 // handed and re-reads those bytes to retransmit, so outbound bytes 838 // 1. ngtcp2 does not copy stream payload — outbound bytes must not move or
914 // must not move or be reused until the peer acknowledges them. 839 // be reused until the peer acknowledges them.
915 // 2. **A blocked stream is not a dead one.** NGTCP2_ERR_STREAM_DATA_BLOCKED 840 // 2. A blocked stream is not a dead one: abandoning the egress loop on
916 // is a documented return; abandoning the egress loop on it also 841 // STREAM_DATA_BLOCKED also abandons the ACKs that reopen the window.
917 // abandons the ACKs that would reopen the window. 842 // 3. BOTH flow-control windows get extended on consume, or the connection
918 // 3. **Both flow-control windows get extended on consume.** Extending 843 // window closes and the daemon stalls a few hundred kilobytes in.
919 // only the stream's leaves the connection window to close, and the
920 // DAEMON stalls a few hundred kilobytes in — a freeze with no error
921 // on either side.
922 // --------------------------------------------------------------------------- 844 // ---------------------------------------------------------------------------
923 845
924 /// wolfSSL's PSK callback carries no user pointer, so the key has to be 846 /// wolfSSL's PSK callback carries no user pointer, so the key must be reachable
925 /// reachable without one. Process-global, and a mux client process is NOT 847 /// without one. Process-global, and a mux client is NOT one dial at a time:
926 /// one dial at a time: every wall host poller and every tile pump dials on 848 /// every poller and tile pump dials on its own thread, so two concurrent dials
927 /// its own thread, each with the key its own handoff announced, so two 849 /// with different keys race between `connect` writing this and the ClientHello
928 /// concurrent dials with two different keys race on this — `connect` writes 850 /// its `drain()` builds. The symptom is a handshake that never completes.
929 /// it, then builds the ClientHello inside its own `drain()`, and the other 851 /// Fix tracked as issue 92d0a0e2: a per-connection key via `wolfSSL_set_app_data`.
930 /// thread may write between those two. The symptom is a handshake that
931 /// never completes: one `unreachable` poll the wall rides out, with no log
932 /// line. The fix is a per-connection key — wolfSSL_set_app_data(ssl, self)
933 /// in `startTls`, wolfSSL_get_app_data(ssl) in `pskClientCb`, and this
934 /// global deleted — tracked as issue 92d0a0e2, and left out of the comment-only
935 /// change that wrote this paragraph because it is a code path with its own
936 /// pin to earn.
937 var g_key: ?Key = null; 852 var g_key: ?Key = null;
938 853
939 fn pskClientCb( 854 fn pskClientCb(
@@ -1033,13 +948,10 @@ pub const Client = struct {
1033 /// timeout, a protocol error, a peer that stopped answering. The caller 948 /// timeout, a protocol error, a peer that stopped answering. The caller
1034 /// reads it as "the transport is gone" and reconnects. 949 /// reads it as "the transport is gone" and reconnects.
1035 dead: bool = false, 950 dead: bool = false,
1036 /// Depth of nesting inside ngtcp2, mirroring the listener's. This side 951 /// Depth of nesting inside ngtcp2, mirroring the listener's. This side has
1037 /// has no live re-entrancy today — its callbacks only set flags and 952 /// no live re-entrancy — its callbacks only set flags and append to `in` —
1038 /// append to `in`, none of them reaches `send` — so `send` may still 953 /// so `send` may still drain, which keeps a keystroke off the next poll
1039 /// drain, which is what keeps a keystroke from waiting for the next poll 954 /// cycle. The assert in `drain` is what makes that checked.
1040 /// cycle. The counter and the assert in `drain` are what make that a
1041 /// checked invariant rather than a property someone has to re-verify
1042 /// every time a callback grows a line.
1043 ngtcp2_depth: u8 = 0, 955 ngtcp2_depth: u8 = 0,
1044 /// Outbound bytes, in the ring that does not move them until they are 956 /// Outbound bytes, in the ring that does not move them until they are
1045 /// acknowledged. See the module comment for why that matters. 957 /// acknowledged. See the module comment for why that matters.
@@ -1203,11 +1115,9 @@ pub const Client = struct {
1203 } 1115 }
1204 } 1116 }
1205 1117
1206 /// Read whatever has arrived, WITHOUT transmitting — `pump` is the pass 1118 /// Read whatever has arrived, WITHOUT transmitting — `pump` is the pass that
1207 /// that also acknowledges. The daemon's tests need the two apart: a 1119 /// also acknowledges. The daemon's tests need the two apart: a client that
1208 /// client that acks is a client the listener gets an inbound packet 1120 /// acks sends the listener a packet, which carries the drain they rule out.
1209 /// from, and an inbound packet carries a drain the test was trying to
1210 /// rule out.
1211 pub fn readable(self: *Client) void { 1121 pub fn readable(self: *Client) void {
1212 var buf: [65536]u8 = undefined; 1122 var buf: [65536]u8 = undefined;
1213 while (true) { 1123 while (true) {
src/server/quic_server.zig
Old New
@@ -1,18 +1,11 @@
1 //! The daemon's QUIC listener: one UDP socket, N authenticated connections, each 1 //! The daemon's QUIC listener: one UDP socket, N authenticated connections,
2 //! carrying exactly one bidirectional stream of opaque bytes. The vocabulary 2 //! each carrying one bidirectional stream of opaque bytes. The vocabulary both
3 //! both ends share — the C import, the key, the egress ring — is quic.zig's, 3 //! ends share is quic.zig's, and the tests below dial with the shipping
4 //! and this file imports it. 4 //! `quic.Client`.
5 //! 5 //!
6 //! The peer the tests below dial with is quic.Client — the shipping one, not 6 //! It follows proxy.zig's discipline and knows NOTHING about the frame
7 //! a second copy of it living here. 7 //! protocol it carries: a `proto.` import here is a mistake. Authentication is
8 //! 8 //! TLS 1.3 external PSK — both ends hold the same 32-byte key, no certificate.
9 //! This file follows proxy.zig's discipline: it knows NOTHING about the
10 //! frame protocol it carries. If a `proto.` import ever appears here,
11 //! something has gone wrong.
12 //!
13 //! Authentication is TLS 1.3 external PSK: both ends hold the same 32-byte
14 //! key and nobody holds a certificate. See spike/quic/README.md for why, and
15 //! for the integration assessment this implements.
16 const std = @import("std"); 9 const std = @import("std");
17 10
18 const quic = @import("quic"); 11 const quic = @import("quic");
@@ -37,25 +30,17 @@ pub const Handler = struct {
37 onOpen: *const fn (ctx: *anyopaque, id: u64) void, 30 onOpen: *const fn (ctx: *anyopaque, id: u64) void,
38 /// Stream bytes arrived. Arbitrary chunking: the owner reassembles. 31 /// Stream bytes arrived. Arbitrary chunking: the owner reassembles.
39 onData: *const fn (ctx: *anyopaque, id: u64, bytes: []const u8) void, 32 onData: *const fn (ctx: *anyopaque, id: u64, bytes: []const u8) void,
40 /// A connection the LISTENER gave up on: an idle timeout, a protocol 33 /// A connection the LISTENER gave up on: an idle timeout, a protocol error,
41 /// error, a peer that went away. 34 /// a peer that went away. It does NOT pair with `onOpen`: a handshake that
42 /// 35 /// never completes produces neither, and a close the owner asked for via
43 /// These two do not pair, and an owner that assumes they do will be 36 /// `closeConn` produces no callback. This means "ended without you asking".
44 /// wrong in both directions. A handshake that never completes — a wrong
45 /// key, a client that vanishes mid-flight — produces neither: the
46 /// connection is torn down having never been announced, because there
47 /// was never anything to announce. And a close the owner itself asked
48 /// for via `closeConn` produces no callback either. `onClose` means
49 /// "this ended without you asking", nothing more.
50 onClose: *const fn (ctx: *anyopaque, id: u64) void, 37 onClose: *const fn (ctx: *anyopaque, id: u64) void,
51 }; 38 };
52 39
53 /// Connections the listener will hold at once. Deliberately larger than the 40 /// Connections the listener will hold at once. Larger than the daemon's
54 /// daemon's `max_clients`: a connection exists from the moment its handshake 41 /// `max_clients` on purpose: a connection exists from the moment its handshake
55 /// completes, and only then asks for a client slot — so the extra room is 42 /// completes and only then asks for a slot, so the room is for handshakes in
56 /// headroom for handshakes in flight, not for sessions. A connection that 43 /// flight. One that finds no slot is answered and closed.
57 /// finds no slot is answered and closed (see the daemon's onOpen), which is
58 /// why the two numbers do not need to agree.
59 const max_conns = 16; 44 const max_conns = 16;
60 45
61 /// Source Connection IDs cached per connection. ngtcp2 caps its own pool at 46 /// Source Connection IDs cached per connection. ngtcp2 caps its own pool at
@@ -104,11 +89,9 @@ const Conn = struct {
104 local_storage: std.posix.sockaddr.storage = undefined, 89 local_storage: std.posix.sockaddr.storage = undefined,
105 local_len: std.posix.socklen_t = 0, 90 local_len: std.posix.socklen_t = 0,
106 scid: c.ngtcp2_cid = undefined, 91 scid: c.ngtcp2_cid = undefined,
107 /// Every Connection ID this endpoint has advertised and not retired. 92 /// Every Connection ID this endpoint has advertised and not retired. A peer
108 /// A peer is entitled to address us by any of them — that is what makes 93 /// may address us by any of them — that is what makes migration work — and
109 /// connection migration work — so matching only `scid` meant a client 94 /// matching only `scid` drops a migrated client's packets as noise.
110 /// that switched CID had its packets fall through to `accept`, where
111 /// they were dropped as noise.
112 cids: [max_cids]c.ngtcp2_cid = undefined, 95 cids: [max_cids]c.ngtcp2_cid = undefined,
113 ncids: usize = 0, 96 ncids: usize = 0,
114 cids_dirty: bool = true, 97 cids_dirty: bool = true,
@@ -144,10 +127,8 @@ const Conn = struct {
144 } 127 }
145 128
146 pub fn deinit(self: *Conn, alloc: std.mem.Allocator) void { 129 pub fn deinit(self: *Conn, alloc: std.mem.Allocator) void {
147 // ngtcp2 first, egress second. The ring is not just this struct's 130 // ngtcp2 first, egress second: it holds vectors into the ring until the
148 // memory: bytes handed to ngtcp2 stay exactly where they are and it 131 // peer acknowledges them, so freeing the ring first leaves ngtcp2
149 // holds vectors into them until they are acknowledged, so freeing
150 // the ring before the connection that points into it leaves ngtcp2
151 // reading freed memory for the length of its own teardown. 132 // reading freed memory for the length of its own teardown.
152 if (self.conn) |cn| c.ngtcp2_conn_del(cn); 133 if (self.conn) |cn| c.ngtcp2_conn_del(cn);
153 if (self.ssl) |s| c.wolfSSL_free(s); 134 if (self.ssl) |s| c.wolfSSL_free(s);
@@ -241,23 +222,16 @@ fn recvStreamDataCb(
241 ) callconv(.c) c_int { 222 ) callconv(.c) c_int {
242 const cn: *Conn = @ptrCast(@alignCast(user_data.?)); 223 const cn: *Conn = @ptrCast(@alignCast(user_data.?));
243 224
244 // MANDATORY, and the reason the spike's C echo pair is a reference and 225 // MANDATORY: ngtcp2 never extends its own windows. Consume without these
245 // not a source: ngtcp2 never extends its own windows. Consume without 226 // two and the peer stops the moment it has sent the initial window, with no
246 // these two calls and the peer stops the moment it has sent the initial 227 // error on either side — it presents as a freeze.
247 // window — on the order of 64KB, which a session of snapshots and deltas
248 // reaches in seconds — with no error on either side. It presents as a
249 // freeze, which is why the test below sends more than a window.
250 _ = c.ngtcp2_conn_extend_max_stream_offset(conn, stream_id, datalen); 228 _ = c.ngtcp2_conn_extend_max_stream_offset(conn, stream_id, datalen);
251 _ = c.ngtcp2_conn_extend_max_offset(conn, datalen); 229 _ = c.ngtcp2_conn_extend_max_offset(conn, datalen);
252 230
253 // Note the order: the window is extended BEFORE the owner has done 231 // The window is extended BEFORE the owner has touched these bytes, so the
254 // anything with these bytes, so the credit is granted against a consume 232 // credit is granted against a consume that has not happened. Safe while
255 // that has not happened yet. That is the right trade here and not an 233 // `onData` is synchronous and bounded by the daemon's own cap; it stops
256 // oversight — the owner's `onData` is synchronous and bounded (it 234 // being safe the moment an owner can hold bytes indefinitely.
257 // appends to a queue the daemon's own cap governs), so there is no
258 // second buffer to overflow by being generous. It would stop being the
259 // right trade the moment an owner could hold bytes indefinitely, which
260 // is exactly what the egress ring now refuses to let it do.
261 if (datalen > 0 and cn.close_state == .open) { 235 if (datalen > 0 and cn.close_state == .open) {
262 cn.listener.handler.onData(cn.listener.handler.ctx, cn.id, data[0..datalen]); 236 cn.listener.handler.onData(cn.listener.handler.ctx, cn.id, data[0..datalen]);
263 } 237 }
@@ -276,30 +250,17 @@ pub const Listener = struct {
276 /// previous run must not validate against this one. 250 /// previous run must not validate against this one.
277 retry_secret: [32]u8, 251 retry_secret: [32]u8,
278 idle_ms: u64, 252 idle_ms: u64,
279 /// True while ngtcp2 owns the stack — i.e. inside `ngtcp2_conn_read_pkt`. 253 /// Nonzero while ngtcp2 owns the stack — inside `ngtcp2_conn_read_pkt`.
280 /// 254 ///
281 /// A connection MUST NOT be freed during that window. ngtcp2 calls our 255 /// A connection MUST NOT be freed in that window: after our receive
282 /// receive callback and then goes on using `conn`: after 256 /// callback ngtcp2 calls `conn_emit_pending_stream_data`, whose FIRST
283 /// conn_call_recv_stream_data it calls conn_emit_pending_stream_data, 257 /// statement dereferences the connection. Our callback reaches the daemon,
284 /// which dereferences the connection again. Our callback reaches the 258 /// which may decide the client is finished — a `.detach` frame is exactly
285 /// daemon, and the daemon may well decide the client is finished — a 259 /// that — so freeing there is a use-after-free on every receive.
286 /// `.detach` frame is exactly that — so `closeConn` used to free the
287 /// Conn underneath ngtcp2 and it segfaulted on the next dereference.
288 /// 260 ///
289 /// It happened on loopback too, and that is the part worth remembering. 261 /// A DEPTH rather than a flag: as a bool it is balanced only by there being
290 /// conn_emit_pending_stream_data is called unconditionally, and its 262 /// no early return between the set and the clear, and one added `return`
291 /// FIRST statement dereferences the connection — 263 /// would leave the listener permanently believing ngtcp2 owns the stack.
292 /// conn_is_tls_handshake_completed(conn), ngtcp2_conn.c:7220 — before
293 /// the `if (!strm->rx.rob)` early-out two lines later. So the freed
294 /// connection was read on every single receive, everywhere. Reordering
295 /// decided only whether that read landed on memory the allocator had
296 /// already taken back, and therefore whether it faulted. A real network
297 /// did not create this bug; it just stopped it reading successfully.
298 /// A DEPTH rather than a flag. As a bool it was balanced only by the
299 /// absence of any early return between the set and the clear — true
300 /// today, and a single `return` added inside that window would have left
301 /// the listener permanently believing ngtcp2 owned the stack, which
302 /// silently disables every teardown that consults it.
303 ngtcp2_depth: u8 = 0, 264 ngtcp2_depth: u8 = 0,
304 265
305 /// Bound BEFORE the daemon's socket: a refused UDP port must not cost a 266 /// Bound BEFORE the daemon's socket: a refused UDP port must not cost a
@@ -342,14 +303,9 @@ pub const Listener = struct {
342 0, 303 0,
343 ); 304 );
344 errdefer std.posix.close(fd); 305 errdefer std.posix.close(fd);
345 // Deliberately NO SO_REUSEADDR. On UDP it lets a second daemon bind 306 // Deliberately NO SO_REUSEADDR: on UDP it lets a second daemon bind the
346 // the same address, and the kernel then hands each datagram to one 307 // same address, and the kernel then splits datagrams between them —
347 // of them: two sessions silently splitting one port, with packets 308 // two sessions silently sharing one port. Fail the bind, loudly.
348 // going to whichever process the kernel picked. The unix socket has
349 // a whole story for "a daemon is already running" precisely because
350 // taking over another daemon's endpoint by accident is unacceptable;
351 // this is that story's QUIC edition, and the answer is the same —
352 // fail the bind, loudly, and let the operator decide.
353 try std.posix.bind(fd, &bind_addr.any, bind_addr.getOsSockLen()); 309 try std.posix.bind(fd, &bind_addr.any, bind_addr.getOsSockLen());
354 310
355 return finishInit(alloc, fd, key, handler, idle_ms); 311 return finishInit(alloc, fd, key, handler, idle_ms);
@@ -483,24 +439,17 @@ pub const Listener = struct {
483 return cn.out.held; 439 return cn.out.held;
484 } 440 }
485 441
486 /// Close ONE connection and nothing else — never the socket it shares. 442 /// Close ONE connection and never the socket it shares: every peer is
487 /// Every QUIC peer is multiplexed over one UDP socket, so closing "the 443 /// multiplexed over one UDP socket. It does NOT invoke `onClose` — calling
488 /// client's transport" would take down every other session. 444 /// back mid-teardown is how re-entrancy bugs start — and does not send
489 /// 445 /// CONNECTION_CLOSE, so the peer finds out when its idle timer expires.
490 /// Two things it deliberately does NOT do. It does not invoke `onClose`:
491 /// calling back into a handler mid-teardown is how re-entrancy bugs start,
492 /// and only `kill` calls back. And it does not send CONNECTION_CLOSE, so
493 /// the peer finds out when its idle timer expires; that cost is accepted,
494 /// and a graceful close belongs with the client transport.
495 pub fn closeConn(self: *Listener, id: u64) void { 446 pub fn closeConn(self: *Listener, id: u64) void {
496 for (&self.conns) |*slot| { 447 for (&self.conns) |*slot| {
497 if (slot.*) |cn| { 448 if (slot.*) |cn| {
498 if (cn.id != id) continue; 449 if (cn.id != id) continue;
499 // Deferred if ngtcp2 is mid-call on this connection: it will 450 // Deferred if ngtcp2 is mid-call: it dereferences `conn` again
500 // dereference `conn` again after our callback returns, and 451 // after our callback returns. `feed` reaps on the way out,
501 // freeing here is a use-after-free. `feed` reaps on the way 452 // quietly — a close the owner asked for gets no callback.
502 // out. Quietly, per this function's contract — a close the
503 // owner asked for gets no callback telling it so.
504 if (self.inNgtcp2()) { 453 if (self.inNgtcp2()) {
505 cn.close_state = .closing_quiet; 454 cn.close_state = .closing_quiet;
506 return; 455 return;
@@ -560,11 +509,9 @@ pub const Listener = struct {
560 if (cn.close_state == .open) continue; 509 if (cn.close_state == .open) continue;
561 const notify = cn.close_state == .closing_notify and cn.opened; 510 const notify = cn.close_state == .closing_notify and cn.opened;
562 const id = cn.id; 511 const id = cn.id;
563 // One last push before it goes. Anything the owner queued while 512 // One last push before it goes: anything the owner queued while
564 // deciding to close it — the session-full refusal is exactly 513 // deciding to close — the session-full refusal — was only queued,
565 // that — was only queued, since send no longer drains, and this 514 // and this is its one chance to reach the wire.
566 // is its only chance to reach the wire. Safe here and nowhere
567 // earlier: the depth is back to zero by the time reaping runs.
568 self.drain(cn); 515 self.drain(cn);
569 cn.deinit(self.alloc); 516 cn.deinit(self.alloc);
570 self.alloc.destroy(cn); 517 self.alloc.destroy(cn);
@@ -584,12 +531,10 @@ pub const Listener = struct {
584 var hd: c.ngtcp2_pkt_hd = undefined; 531 var hd: c.ngtcp2_pkt_hd = undefined;
585 if (c.ngtcp2_accept(&hd, pkt.ptr, pkt.len) != 0) return; 532 if (c.ngtcp2_accept(&hd, pkt.ptr, pkt.len) != 0) return;
586 533
587 // No token: answer with a Retry and nothing else. Until the peer 534 // No token: answer with a Retry and nothing else. Until the peer echoes
588 // echoes a token we minted for its address, we have no evidence the 535 // a token we minted for its address there is no evidence the address is
589 // address is real, and a server that hands out handshake bytes on 536 // real, and handing out handshake bytes anyway is an amplification
590 // that evidence is an amplification reflector. One round trip is 537 // reflector. Sent before any conn state, so a flood costs no memory.
591 // the price; it is also why a Retry is sent before any conn state
592 // is allocated, so a flood costs us no memory either.
593 if (hd.tokenlen == 0) { 538 if (hd.tokenlen == 0) {
594 self.sendRetry(&hd, from, from_len); 539 self.sendRetry(&hd, from, from_len);
595 return; 540 return;
@@ -633,11 +578,9 @@ pub const Listener = struct {
633 return; 578 return;
634 }; 579 };
635 580
636 // The connection's source CID must be the one the client is already 581 // The source CID must be the one the client is already addressing — the
637 // addressing — i.e. the SCID we put in the Retry, which is exactly 582 // SCID we put in the Retry, i.e. this Initial's DCID. A fresh one leaves
638 // the DCID this token-bearing Initial arrived on. Minting a fresh 583 // ngtcp2 owning a CID nobody sends to, and the handshake stalls.
639 // one here instead leaves ngtcp2 owning a CID nobody is sending to,
640 // and the handshake stalls with no error on either side.
641 cn.scid = hd.dcid; 584 cn.scid = hd.dcid;
642 585
643 const ssl = c.wolfSSL_new(self.ssl_ctx) orelse { 586 const ssl = c.wolfSSL_new(self.ssl_ctx) orelse {
@@ -712,13 +655,10 @@ pub const Listener = struct {
712 } 655 }
713 cn.conn = conn; 656 cn.conn = conn;
714 c.ngtcp2_conn_set_tls_native_handle(conn, ssl); 657 c.ngtcp2_conn_set_tls_native_handle(conn, ssl);
715 // Without this, a terminal nobody is typing into is 658 // Without this, a terminal nobody is typing into is indistinguishable
716 // indistinguishable from a peer that has gone away, and the 659 // from a peer that has gone away, and a session dies while its owner
717 // connection is dropped after idle_ms of quiet — which for a 660 // reads the screen. The keep-alive PING is ack-eliciting, so a peer that
718 // default of 15s means a session dies while its owner reads the 661 // has genuinely vanished still times out on schedule.
719 // screen. The keep-alive PING is ack-eliciting, so its ACK restarts
720 // the idle timer at both ends; a peer that has genuinely vanished
721 // answers nothing and still times out on schedule.
722 c.ngtcp2_conn_set_keep_alive_timeout(conn, quic.keepAliveNs(self.idle_ms)); 662 c.ngtcp2_conn_set_keep_alive_timeout(conn, quic.keepAliveNs(self.idle_ms));
723 slot.* = cn; 663 slot.* = cn;
724 664
@@ -829,13 +769,10 @@ pub const Listener = struct {
829 const rv = c.ngtcp2_pkt_decode_version_cid(&vc, pkt.ptr, pkt.len, 8); 769 const rv = c.ngtcp2_pkt_decode_version_cid(&vc, pkt.ptr, pkt.len, 8);
830 if (rv != 0) return; 770 if (rv != 0) return;
831 771
832 // Known connection? Match on ANY Connection ID this endpoint has 772 // Match on ANY Connection ID this endpoint has advertised, not just the
833 // advertised, not merely the one the connection was created with. 773 // one the connection was created with: a migrating client switches to
834 // A client that migrates — a new network, a NAT rebind, the exact 774 // another CID we gave it (RFC 9000 §9.5), and matching only the first
835 // case QUIC exists to survive — switches to another CID we gave it 775 // sends those packets to `accept`, which drops them.
836 // (RFC 9000 section 9.5). Matching only the first one sent those
837 // packets to `accept`, which has no token for them and drops them:
838 // the migration presents as a dead connection.
839 for (&self.conns) |*slot| { 776 for (&self.conns) |*slot| {
840 const cn = slot.* orelse continue; 777 const cn = slot.* orelse continue;
841 if (!cn.matches(vc.dcid[0..vc.dcidlen])) continue; 778 if (!cn.matches(vc.dcid[0..vc.dcidlen])) continue;
@@ -860,13 +797,10 @@ pub const Listener = struct {
860 // Reaped before anything else touches the table: a connection the 797 // Reaped before anything else touches the table: a connection the
861 // handler closed mid-callback is gone from here on. 798 // handler closed mid-callback is gone from here on.
862 self.reapClosing(); 799 self.reapClosing();
863 // Recomputed AFTER the reap, and re-read from the slot rather than 800 // Recomputed AFTER the reap and re-read from the SLOT, not the `cn`
864 // trusting the `cn` captured above. reapClosing calls onClose with 801 // above: `reapClosing` calls `onClose` at depth zero, so a handler that
865 // the depth back at zero, so a handler that closes some OTHER 802 // closes another connection frees it immediately — and that other one
866 // connection frees it immediately — and if that other one happened 803 // may be this one.
867 // to be this one, every use of `cn` past this point is a read of
868 // freed memory. Asking the slot again is the only answer that
869 // cannot go stale.
870 const closed = slot.* == null or slot.*.?.close_state != .open; 804 const closed = slot.* == null or slot.*.?.close_state != .open;
871 if (rv != 0) { 805 if (rv != 0) {
872 if (!closed) self.kill(slot); 806 if (!closed) self.kill(slot);
@@ -885,11 +819,9 @@ pub const Listener = struct {
885 // one run in a few hundred somewhere else entirely. 819 // one run in a few hundred somewhere else entirely.
886 std.debug.assert(self.ngtcp2_depth == 0); 820 std.debug.assert(self.ngtcp2_depth == 0);
887 const conn = cn.conn orelse return; 821 const conn = cn.conn orelse return;
888 // No refusal check, unlike quic.Client's: this socket is 822 // No refusal check, unlike `quic.Client`'s: this socket is UNCONNECTED,
889 // UNCONNECTED, so the kernel has no peer to attribute an ICMP 823 // so the kernel has no peer to attribute an ICMP unreachable to. The
890 // unreachable to and never delivers one. The asymmetry with the 824 // asymmetry is the sockets', which is why `sendPkt` is the seam.
891 // client is the sockets', not drift — which is why `sendPkt` is the
892 // seam the shared loop is cut at.
893 const To = struct { l: *Listener, cn: *Conn }; 825 const To = struct { l: *Listener, cn: *Conn };
894 const sendPkt = struct { 826 const sendPkt = struct {
895 fn f(ctx: To, pkt: []const u8) bool { 827 fn f(ctx: To, pkt: []const u8) bool {
@@ -926,22 +858,15 @@ pub const Listener = struct {
926 }; 858 };
927 859
928 // --------------------------------------------------------------------------- 860 // ---------------------------------------------------------------------------
929 // Tests: a real handshake against a real client, in one process. 861 // Tests: a real handshake against a real client, in one process. A QUIC
930 // 862 // handshake either completes against a real peer or it does not, so these dial
931 // The listener cannot be tested by inspection — a QUIC handshake either 863 // the SHIPPING client, wrapped for the two conveniences a test wants.
932 // completes against a real peer or it does not — so the tests below dial it
933 // with the shipping client, wrapped for the two conveniences a test wants.
934 // --------------------------------------------------------------------------- 864 // ---------------------------------------------------------------------------
935 865
936 /// The shipping `quic.Client`, plus the two things only a test wants: a 866 /// The shipping `quic.Client`, plus the two things only a test wants: a backlog
937 /// backlog it re-offers as the ring drains — `send` takes what fits, and a 867 /// it re-offers as the ring drains — `send` takes what fits, and a payload over
938 /// payload larger than `egress_cap` needs somebody to hold the rest — and a 868 /// `egress_cap` needs somebody to hold the rest — and a value a stack `defer`
939 /// value a stack `defer` can deinit. 869 /// can deinit.
940 ///
941 /// It replaced a second hand-written QUIC client that lived here. That copy
942 /// existed because the folder rule forbids the server naming a client
943 /// module; `quic.Client` moving down to quic.zig, which both ends already
944 /// import, dissolved the reason rather than the rule.
945 pub const TestPeer = struct { 870 pub const TestPeer = struct {
946 cl: *quic.Client, 871 cl: *quic.Client,
947 out: []const u8 = &.{}, 872 out: []const u8 = &.{},
@@ -1098,24 +1023,16 @@ test "Listener: PSK handshake, Retry, and a payload larger than the initial wind
1098 // address validation having taken place. 1023 // address validation having taken place.
1099 try std.testing.expect(setup.l.conns[0] != null); 1024 try std.testing.expect(setup.l.conns[0] != null);
1100 1025
1101 // The freeze-shaped case. The size is not arbitrary: it must exceed the 1026 // The freeze-shaped case. The size must exceed the windows this listener
1102 // windows this listener actually advertises (256KB per stream, 1MB per 1027 // advertises (256KB per stream, 1MB per connection), because those are what
1103 // connection), because those are what the peer stops at when nobody 1028 // the peer stops at when nobody extends them — 200KB fits inside both and
1104 // extends them. An earlier version of this test sent 200KB, which fits 1029 // passes with flow control disabled. Without the extends this does not
1105 // inside both — it passed with flow control deliberately disabled, and 1030 // fail, it STOPS, which is the shape of the bug in production.
1106 // was therefore testing nothing. 3MB forces both the stream-level and
1107 // connection-level extends to do real work.
1108 //
1109 // Without them this does not fail, it STOPS: the pump runs out its
1110 // deadline with the transfer half done. That is the shape of the bug in
1111 // production too, which is why it is worth a test rather than a comment.
1112 const payload = try alloc.alloc(u8, 3 * 1024 * 1024); 1031 const payload = try alloc.alloc(u8, 3 * 1024 * 1024);
1113 defer alloc.free(payload); 1032 defer alloc.free(payload);
1114 // A repeating byte pattern, compared byte for byte, not merely counted. 1033 // A repeating pattern, compared byte for byte and not merely counted:
1115 // Counting alone cannot tell a working transport from one that delivers 1034 // counting cannot tell a working transport from one delivering the right
1116 // the right NUMBER of the wrong bytes — reordered, duplicated, or read 1035 // NUMBER of the wrong bytes, out of a buffer recycled underneath it.
1117 // back out of a buffer that had been recycled underneath it, which is
1118 // precisely the failure this file has already had once.
1119 for (payload, 0..) |*b, i| b.* = @truncate(i); 1036 for (payload, 0..) |*b, i| b.* = @truncate(i);
1120 cl.offer(payload); 1037 cl.offer(payload);
1121 cl.drain(); 1038 cl.drain();
@@ -1135,11 +1052,9 @@ test "Listener: PSK handshake, Retry, and a payload larger than the initial wind
1135 try std.testing.expectEqual(@as(usize, 3 * 1024 * 1024), owner.received); 1052 try std.testing.expectEqual(@as(usize, 3 * 1024 * 1024), owner.received);
1136 try std.testing.expectEqualSlices(u8, payload, cl.cl.in.items); 1053 try std.testing.expectEqualSlices(u8, payload, cl.cl.in.items);
1137 1054
1138 // Piggybacking on the one place a LIVE connection with a finite expiry 1055 // The one place a LIVE connection with a finite expiry exists: negative is
1139 // exists: negative is poll(2)'s "wait forever", and the timeout fold 1056 // poll(2)'s "wait forever" and the fold must hand it back rather than
1140 // must hand it back rather than try to @intCast it. Without the guard 1057 // `@intCast` it. It needs a real connection, or the loop never reaches the cast.
1141 // this line panics — and it needs a real connection, because the loop
1142 // skips connections with no expiry and would never reach the cast.
1143 try std.testing.expectEqual(@as(i32, -1), setup.l.timeoutMs(-1)); 1058 try std.testing.expectEqual(@as(i32, -1), setup.l.timeoutMs(-1));
1144 // The ordinary case still folds as before. 1059 // The ordinary case still folds as before.
1145 try std.testing.expect(setup.l.timeoutMs(100) <= 100); 1060 try std.testing.expect(setup.l.timeoutMs(100) <= 100);
@@ -1170,14 +1085,10 @@ test "Listener: a client holding the wrong key never completes a handshake" {
1170 try std.testing.expectEqual(@as(usize, 0), owner.opened); 1085 try std.testing.expectEqual(@as(usize, 0), owner.opened);
1171 try std.testing.expectEqual(@as(usize, 0), owner.received); 1086 try std.testing.expectEqual(@as(usize, 0), owner.received);
1172 1087
1173 // ...and it failed on the KEY, which is a stronger claim than "it 1088 // ...and it failed on the KEY, which is stronger than "it failed": a client
1174 // failed". A client refused earlier — a Retry token that never 1089 // refused earlier would satisfy every assertion above while proving nothing
1175 // validated, an Initial the server would not parse — would satisfy 1090 // about authentication. An id is only allocated once a token-bearing Initial
1176 // every assertion above while proving nothing about authentication. 1091 // is accepted, so one having been handed out is the witness.
1177 // The listener only allocates a connection id once it has accepted a
1178 // token-bearing Initial, so an id having been handed out is the witness
1179 // that this client got all the way to the point where the PSK is the
1180 // only thing left to disagree about.
1181 try std.testing.expect(setup.l.next_id > 1); 1092 try std.testing.expect(setup.l.next_id > 1);
1182 } 1093 }
1183 1094
@@ -1224,12 +1135,9 @@ test "Listener: keepalive carries an idle connection past its idle timeout" {
1224 try std.testing.expectEqual(@as(usize, 0), owner.closed); 1135 try std.testing.expectEqual(@as(usize, 0), owner.closed);
1225 } 1136 }
1226 1137
1227 /// Get the peer's stream on the record. The server learns a stream id only 1138 /// Get the peer's stream on the record. The server learns a stream id only when
1228 /// when data arrives on it — the client opening one locally tells the server 1139 /// data ARRIVES on it, so a test that wants the server to send first must make
1229 /// nothing — so a test that wants the server to SEND first has to make the 1140 /// the client speak first. Waits for the ack, so the ring is empty again.
1230 /// client say something first. Waits until the round trip is acknowledged,
1231 /// so the egress ring is empty again and the next assertion is about the
1232 /// test's own bytes.
1233 fn openStream(l: *Listener, cl: *TestPeer, owner: *EchoOwner) !void { 1141 fn openStream(l: *Listener, cl: *TestPeer, owner: *EchoOwner) !void {
1234 cl.offer("hi"); 1142 cl.offer("hi");
1235 cl.drain(); 1143 cl.drain();
@@ -1288,14 +1196,10 @@ test "Listener: bytes survive retransmission, which is what the buffer is for" {
1288 var cl = try TestPeer.init(setup.addr, key); 1196 var cl = try TestPeer.init(setup.addr, key);
1289 defer cl.deinit(); 1197 defer cl.deinit();
1290 1198
1291 // A deliberately tiny receive buffer, which is how loopback is made to 1199 // A deliberately tiny receive buffer, which is how loopback is made to lose
1292 // lose packets on purpose. Loss is the ONLY way into ngtcp2's 1200 // packets. Loss is the ONLY way into ngtcp2's retransmission path, which
1293 // retransmission path, and that path re-reads the original bytes 1201 // re-reads bytes through a pointer handed over packets ago — so it is the
1294 // through a pointer the listener handed over packets ago — so it is 1202 // only way to catch a buffer that recycled them underneath it.
1295 // also the only way to catch a buffer that recycled them underneath it.
1296 // Without the loss this test is just another echo; the 3MB test above
1297 // reached the same path by accident, about one run in twenty, and
1298 // segfaulted when it did.
1299 try std.posix.setsockopt( 1203 try std.posix.setsockopt(
1300 cl.cl.fd, 1204 cl.cl.fd,
1301 std.posix.SOL.SOCKET, 1205 std.posix.SOL.SOCKET,
@@ -1358,21 +1262,17 @@ test "Listener.send: takes what fits, refuses when full, and recovers on acks" {
1358 defer alloc.free(big); 1262 defer alloc.free(big);
1359 @memset(big, 0x5A); 1263 @memset(big, 0x5A);
1360 1264
1361 // Nobody is pumping the client from here, so nothing is acknowledged 1265 // Nobody is pumping the client here, so nothing is acknowledged or released:
1362 // and nothing is released. The first send fills the ring exactly and 1266 // the first send fills the ring exactly and reports the short count, and the
1363 // reports the short count; the daemon keeps the remainder, which is the 1267 // daemon keeps the remainder where `pending_cap` can see it.
1364 // whole point — an accept-everything send moved that backlog down here
1365 // where pending_cap could never see it.
1366 const first = try setup.l.send(owner.id, big); 1268 const first = try setup.l.send(owner.id, big);
1367 try std.testing.expectEqual(quic.egress_cap, first); 1269 try std.testing.expectEqual(quic.egress_cap, first);
1368 try std.testing.expectEqual(@as(usize, 0), try setup.l.send(owner.id, "x")); 1270 try std.testing.expectEqual(@as(usize, 0), try setup.l.send(owner.id, "x"));
1369 try std.testing.expectEqual(quic.egress_cap, setup.l.pendingBytes(owner.id)); 1271 try std.testing.expectEqual(quic.egress_cap, setup.l.pendingBytes(owner.id));
1370 1272
1371 // Now let the client read and acknowledge: space comes back, and it 1273 // Space comes back from ACKS, not from having handed bytes to ngtcp2. The
1372 // comes back from acks rather than from having handed bytes to ngtcp2. 1274 // last byte reaching the client is not the same event as the ring being free
1373 // Waiting on the ACK, not on the arrival: the last byte reaching the 1275 // to reuse it, and conflating them is what this ring exists to stop.
1374 // client is not the same event as the ring being free to reuse it, and
1375 // conflating them is what this buffer exists to stop anyone doing.
1376 try std.testing.expect(pumpUntil(setup.l, &cl, &owner, 20_000, struct { 1276 try std.testing.expect(pumpUntil(setup.l, &cl, &owner, 20_000, struct {
1377 fn f(o: *EchoOwner, _: *TestPeer) bool { 1277 fn f(o: *EchoOwner, _: *TestPeer) bool {
1378 return o.listener.pendingBytes(o.id) == 0; 1278 return o.listener.pendingBytes(o.id) == 0;
@@ -1472,12 +1372,10 @@ test "Listener: a packet addressed to any advertised CID reaches its connection"
1472 const stranger_cid = [_]u8{0xEE} ** 8; 1372 const stranger_cid = [_]u8{0xEE} ** 8;
1473 try std.testing.expect(!cn.matches(&stranger_cid)); 1373 try std.testing.expect(!cn.matches(&stranger_cid));
1474 1374
1475 // Now the routing itself. The observable is what happens to a packet 1375 // The observable is what happens to a packet that MISSES: it falls through
1476 // that MISSES: it falls through to `accept`, which answers a token-less 1376 // to `accept`, which answers a token-less Initial with a Retry. A probe that
1477 // Initial with a Retry. So a probe socket that receives nothing is proof 1377 // receives nothing is proof of delivery to the connection; the same probe
1478 // the packet was delivered to the connection instead — and the same 1378 // receiving a Retry for an unknown CID proves the packet was well-formed.
1479 // probe receiving a Retry for an unknown CID proves the packet was
1480 // well-formed enough for `accept` to have answered it.
1481 const probe = try std.posix.socket( 1379 const probe = try std.posix.socket(
1482 std.posix.AF.INET, 1380 std.posix.AF.INET,
1483 std.posix.SOCK.DGRAM | std.posix.SOCK.NONBLOCK | std.posix.SOCK.CLOEXEC, 1381 std.posix.SOCK.DGRAM | std.posix.SOCK.NONBLOCK | std.posix.SOCK.CLOEXEC,
@@ -1541,17 +1439,10 @@ test "Listener: a reply queued just before a close still reaches the peer" {
1541 const alloc = std.testing.allocator; 1439 const alloc = std.testing.allocator;
1542 const key: quic.Key = .{ .bytes = [_]u8{0x5C} ** quic.key_len }; 1440 const key: quic.Key = .{ .bytes = [_]u8{0x5C} ** quic.key_len };
1543 1441
1544 // The session-full refusal, in miniature: the owner answers and then 1442 // The session-full refusal in miniature: the owner answers and then shuts
1545 // shuts the connection, both from inside one callback. Since `send` only 1443 // the connection from inside one callback. Since `send` only QUEUES, those
1546 // QUEUES — draining from inside an ngtcp2 callback is the defect that 1444 // bytes have one chance to leave — the drain `reapClosing` does before it
1547 // change removed — those bytes have exactly one chance to leave, in the 1445 // frees the connection. Bounded by the peer's window: a refusal always fits.
1548 // drain reapClosing does before it frees the connection. Delete that
1549 // drain and a refused client learns nothing and waits out its idle
1550 // timeout instead.
1551 //
1552 // The guarantee is bounded by the peer's flow-control window at that
1553 // instant. A refusal is a handful of bytes and always fits; a large
1554 // queued payload would not, and this test does not claim otherwise.
1555 const AnswerThenClose = struct { 1446 const AnswerThenClose = struct {
1556 listener: *Listener = undefined, 1447 listener: *Listener = undefined,
1557 id: u64 = 0, 1448 id: u64 = 0,
@@ -1625,14 +1516,10 @@ test "Listener: closing a connection from inside a receive callback is deferred"
1625 const alloc = std.testing.allocator; 1516 const alloc = std.testing.allocator;
1626 const key: quic.Key = .{ .bytes = [_]u8{0x7E} ** quic.key_len }; 1517 const key: quic.Key = .{ .bytes = [_]u8{0x7E} ** quic.key_len };
1627 1518
1628 // An owner that does what the daemon does when a client says goodbye: 1519 // An owner that closes the connection from inside `onData`, as the daemon
1629 // closes the connection from inside onData. ngtcp2 is on the stack at 1520 // does on goodbye. ngtcp2 is on the stack and uses the connection after the
1630 // that moment and goes on using the connection after the callback 1521 // callback returns, so freeing there is a use-after-free. This pins the
1631 // returns, so freeing it there is a use-after-free — one that loopback 1522 // DEFERRAL: still valid when the callback returns, gone by the next pump.
1632 // cannot show, because the second dereference only happens when there
1633 // is buffered out-of-order data to flush. This test pins the deferral
1634 // rather than the crash: the connection must still be a valid object
1635 // when the callback returns, and gone by the time the pump comes back.
1636 const CloseOnData = struct { 1523 const CloseOnData = struct {
1637 listener: *Listener = undefined, 1524 listener: *Listener = undefined,
1638 id: u64 = 0, 1525 id: u64 = 0,