From 0e1bbed42c96d9d42ab8f04e53f2961093e9032b Mon Sep 17 00:00:00 2001 From: Zakaria El Orche Date: Thu, 13 Aug 2026 14:25:12 +0000 Subject: [PATCH] =?UTF-8?q?feat(core):=20HTTP/2=20Phase=205=20=E2=80=94=20?= =?UTF-8?q?frame=20layer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements HTTP/2 frame reading, validation, and writing: FrameType (the 10 RFC 9113 types + per-type validation descriptor), FrameFlags (with the deliberate END_STREAM/ACK bit collision documented), FrameHeader (a flyweight, never allocated per frame), Http2FrameReader (length-prefixed reader over BufferedByteSource, mirroring RequestParser's buffer/ compaction discipline), FrameValidator (table-driven, specific RFC error code per violation -- not a uniform code per type), Padding (RFC 9113 6.1/6.2), and FrameWriteBuffer (beginFrame/endFrame length back-patching over Phase 4's ByteWriter). All 10 frame types round-trip correctly; every RFC-mandated rejection has its own test asserting the specific error code; the reader is fuzz-tested against 10,000,000 random inputs (~14s). The zero-alloc contract is measured, not asserted: reading + validating + consuming a frame is 0.002 B/op, writing one is ~10^-4 B/op -- both indistinguishable from zero (DEC-21). Found and fixed EX-37 while writing Http2FrameReaderTest: BufferedByteSource's deadline mechanism (EX-07's actual fix) NPE'd against a null socket, which every isolated unit test in this codebase uses -- it had zero dedicated test coverage of its own. Fixed to treat a null socket as "no OS-level timeout to bound" rather than a misuse, and given BufferedByteSourceTest, which did not exist before. 449/449 tests green, both with and without -Pjmh. Co-Authored-By: Claude Sonnet 5 --- flash/docs/http2/DECISIONS.md | 32 +++ flash/docs/http2/FRAMES.md | 148 +++++++++++++ flash/docs/http2/IMPLEMENTATION-PLAN.md | 83 +++++-- .../flash/h2/frame/FrameLayerBenchmark.java | 113 ++++++++++ .../java/dev/relism/flash/h2/Http2Limits.java | 9 + .../dev/relism/flash/h2/frame/FrameFlags.java | 39 ++++ .../relism/flash/h2/frame/FrameHeader.java | 84 +++++++ .../dev/relism/flash/h2/frame/FrameType.java | 88 ++++++++ .../relism/flash/h2/frame/FrameValidator.java | 90 ++++++++ .../flash/h2/frame/FrameWriteBuffer.java | 76 +++++++ .../flash/h2/frame/Http2FrameReader.java | 133 +++++++++++ .../dev/relism/flash/h2/frame/Padding.java | 67 ++++++ .../flash/transport/BufferedByteSource.java | 25 ++- .../flash/h2/frame/FrameValidatorTest.java | 181 +++++++++++++++ .../h2/frame/Http2FrameReaderFuzzTest.java | 59 +++++ .../flash/h2/frame/Http2FrameReaderTest.java | 209 ++++++++++++++++++ .../relism/flash/h2/frame/PaddingTest.java | 88 ++++++++ .../transport/BufferedByteSourceTest.java | 164 ++++++++++++++ 18 files changed, 1665 insertions(+), 23 deletions(-) create mode 100644 flash/docs/http2/FRAMES.md create mode 100644 flash/src/jmh/java/dev/relism/flash/h2/frame/FrameLayerBenchmark.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/FrameFlags.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/FrameHeader.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/FrameType.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/FrameValidator.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/FrameWriteBuffer.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/Http2FrameReader.java create mode 100644 flash/src/main/java/dev/relism/flash/h2/frame/Padding.java create mode 100644 flash/src/test/java/dev/relism/flash/h2/frame/FrameValidatorTest.java create mode 100644 flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderFuzzTest.java create mode 100644 flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderTest.java create mode 100644 flash/src/test/java/dev/relism/flash/h2/frame/PaddingTest.java create mode 100644 flash/src/test/java/dev/relism/flash/transport/BufferedByteSourceTest.java diff --git a/flash/docs/http2/DECISIONS.md b/flash/docs/http2/DECISIONS.md index dbd4207..d547e3a 100644 --- a/flash/docs/http2/DECISIONS.md +++ b/flash/docs/http2/DECISIONS.md @@ -733,3 +733,35 @@ once Phase 6 lands `Request`/`RequestBody` pooling — re-run this exact benchma this entry (or add a new one) with the "after" number, closing the loop Phase 4 opened. --- + +## DEC-21 — Phase 5's zero-alloc contract, measured + +**Context.** Phase 5's plan states: "Reading, validating and discarding a frame: 0 B/op ... +Writing a frame header: 0 B/op." Measured with JMH `-prof gc` (JDK 21.0.11, JMH 1.37, +`FrameLayerBenchmark`, `src/jmh/java`) rather than left as an unverified assertion, per this +project's own standing practice of measuring every stated performance/allocation claim +(`DEC-09`, `DEC-20`). + +**Measurement.** `readValidateAndDiscard` (`Http2FrameReader.readFrame` + +`FrameValidator.validate` + one byte read from the payload + `consumeFrame`, against a warm, +already-grown buffer, matching real keep-alive-connection steady state): 299.846 ± 19.722 ns/op, +**0.002 B/op** — indistinguishable from zero (compare `DEC-20`'s harness-floor discussion: even +this near-zero figure is most plausibly measurement noise around the true 0, not a real +allocation, since nothing in the read/validate/consume path can be shown by inspection to +allocate on the warm path). `writeFrame` (`FrameWriteBuffer.beginFrame` + one `writeBytes` call + +`endFrame`, against an already-grown `ByteWriter`): 14.262 ± 1.084 ns/op, **≈10⁻⁴ B/op** — +likewise indistinguishable from zero. + +**Decision.** Contract verified as stated; no design change required. Both numbers are recorded +here as the baseline Phase 17's eventual CI allocation gate should hold this component to. + +**Consequence.** None beyond the recorded numbers — this entry exists so a future regression +(e.g. a later phase accidentally introducing an allocation on this path while adding HPACK or +stream-state integration) has a concrete "was 0, now isn't" baseline to diff against, per this +project's standing insistence that every non-obvious performance claim trace to an actual number. + +**Revisit when.** Not expected to be revisited; re-measure if `FrameHeader`, `Http2FrameReader`, +or `FrameWriteBuffer` are ever modified in a way that could plausibly affect their allocation +profile. + +--- diff --git a/flash/docs/http2/FRAMES.md b/flash/docs/http2/FRAMES.md new file mode 100644 index 0000000..d84b488 --- /dev/null +++ b/flash/docs/http2/FRAMES.md @@ -0,0 +1,148 @@ +# The Frame Layer (Phase 5) + +Audience: contributors. This is the design record for `dev.relism.flash.h2.frame`'s frame +reading, validation, and writing — the 9-byte header and payload boundary, with no connection +semantics, no streams, and no HPACK above it. + +## Why this is simpler than the h1 parser + +HTTP/1.1 request parsing must scan for `\r\n\r\n` (`RequestParser`, `ByteScan.indexOfCrLfCrLf`) +because nothing in the h1 wire format states the header block's length up front. HTTP/2 states +every frame's payload length in the first three bytes of its 9-byte header — nothing is ever +scanned for. `Http2FrameReader` is a length-prefixed reader and nothing more: read 9 bytes, +decode the length, ensure that many more bytes are available, done. + +## The wire format + +``` ++-----------------------------------------------+ +| Length (24) | ++---------------+---------------+---------------+ +| Type (8) | Flags (8) | ++-+-------------+---------------+-------------------------------+ +|R| Stream Identifier (31) | ++=+=============================================================+ +| Frame Payload (0...) ... ++---------------------------------------------------------------+ +``` + +`R` (RFC 9113 §4.1) is reserved and MUST be ignored on receipt — `FrameHeader.reset` masks it +out of `streamId()` once, so no caller has to remember to. + +## Package layout + +``` +dev.relism.flash.h2.frame +├── FrameType the 10 known types + per-type validation descriptor (min/max length, stream-id rule) +├── FrameFlags END_STREAM/ACK/END_HEADERS/PADDED/PRIORITY bit constants + predicates +├── FrameHeader flyweight over a read buffer: length/type/flags/streamId/payloadOffset +├── Http2FrameReader length-prefixed reader, RequestParser's buffer/compaction discipline +├── FrameValidator table-driven per-type RFC validation, specific error code per rule +├── Padding RFC 9113 §6.1/§6.2 pad-length byte + trailing padding, DATA/HEADERS +├── FrameWriteBuffer beginFrame()/endFrame() length back-patching over a ByteWriter +├── Http2FrameWriter (Phase 3) the connection's single serialized writer — unchanged here +├── WriteIntent (Phase 3) unchanged +└── IntrusiveMpscQueue (Phase 3) unchanged +``` + +## The validation table + +Every rule below is enforced by `FrameValidator.validate(FrameHeader, insideHeaderBlock)`, in +this order: unknown-type handling, `SETTINGS`' modulus-6 special case, the generic +min/max length bounds, the `MAX_FRAME_SIZE_LOCAL` ceiling, the stream-id rule, then +`PUSH_PROMISE`'s always-reject rule. + +| Type | Code | Length | Stream id | Notes / RFC | +|---|---|---|---|---| +| DATA | 0x0 | 0..MAX_FRAME_SIZE | required (≠0) | §6.1. Padding via `Padding.unpad`. | +| HEADERS | 0x1 | 0..MAX_FRAME_SIZE | required (≠0) | §6.2. Padding + PRIORITY fields (Phase 7+ parses the latter). | +| PRIORITY | 0x2 | exactly 5 | required (≠0) | §6.3. Deprecated (§5.3.2) — parsed, discarded, never acted on. | +| RST_STREAM | 0x3 | exactly 4 | required (≠0) | §6.4. The 4 bytes are the error code. | +| SETTINGS | 0x4 | multiple of 6 | forbidden (=0) | §6.5. Modulus checked before the generic bounds. | +| PUSH_PROMISE | 0x5 | ≥4 | required (≠0) | §6.6. Always `PROTOCOL_ERROR` from a client — never sent by Flash. | +| PING | 0x6 | exactly 8 | forbidden (=0) | §6.7. Opaque 8-byte payload, echoed on ACK. | +| GOAWAY | 0x7 | ≥8 | forbidden (=0) | §6.8. Last-stream-id (4) + error code (4) + optional debug data. | +| WINDOW_UPDATE | 0x8 | exactly 4 | either | §6.9. 0 = connection window, ≠0 = one stream's window. | +| CONTINUATION | 0x9 | 0..MAX_FRAME_SIZE | required (≠0) | §6.10. Continues a header block; see the flood guard below. | +| *(unrecognised)* | >0x9 | — | — | §4.1: ignored outside a header block, `PROTOCOL_ERROR` inside one (§6.10). | + +**The error code is not uniform per type** — a `SETTINGS` frame with a bad length is +`FRAME_SIZE_ERROR`; the same frame with a non-zero stream id is `PROTOCOL_ERROR`. Every violation +in the table above carries its own RFC citation and the specific code that citation mandates; +`FrameValidatorTest` has one test per row asserting the exact code, not merely "an exception". + +## Ignore vs. reject policy + +RFC 9113 §4.1 makes unknown frame types part of the protocol's extension mechanism: an endpoint +that does not recognise a type MUST read and discard its payload, never reject the connection for +it. `FrameType.fromCode` returns `null` for anything above `CONTINUATION` (0x9); `FrameHeader` +still exposes the raw `typeCode()` for logging even when `type()` is `null`. + +The one exception (§6.10): if an unrecognised-type frame arrives **between** a HEADERS/ +PUSH_PROMISE frame that lacked `END_HEADERS` and the CONTINUATION that eventually sets it, the +HPACK decoder's state has nowhere to put that frame's bytes without desynchronizing — so this one +case *is* a `PROTOCOL_ERROR`, tracked by `FrameValidator.validate`'s `insideHeaderBlock` +parameter (owned and threaded through by the Phase 8 connection loop, which is the only caller +that knows whether a header block is currently open). + +`PRIORITY` frames are a different kind of "ignore": they are a recognised, well-formed type that +Flash chooses not to act on (RFC 9113 §5.3.2 deprecates priority signalling and permits an +implementation to disregard it) — they are still fully parsed and validated like any other frame, +just never influence scheduling. `PUSH_PROMISE` is the opposite: recognised, but **always** +rejected when received (Flash advertises `SETTINGS_ENABLE_PUSH=0` and never sends one itself), so +receiving one at all can only mean the peer has the client/server roles backwards. + +## Buffer discipline and the frame-size defence + +`Http2FrameReader` never grows its buffer to accommodate a declared length before checking that +length against `Http2Limits.MAX_FRAME_SIZE_LOCAL` — the check happens first, so a hostile 16 MB +declared length is rejected at the cost of reading 9 bytes, not at the cost of a 16 MB +allocation. This mirrors `RequestParser`'s own `EX-08` discipline (bound the request line before +trusting it) applied to the frame layer's own attack surface. + +The buffer itself follows `RequestParser`'s compact-before-grow policy: unconsumed bytes slide to +offset 0 when there is room to do so without growing, and growth only happens when compaction +alone cannot make room — bounded, because the reader's own length check already rejected +anything that would require growing past `9 + MAX_FRAME_SIZE_LOCAL`. + +## Padding + +`Padding.unpad` locates the actual data range within a `PADDED` frame's payload: 1 byte of +pad-length, then data, then that many padding bytes (whose contents carry no meaning — they exist +only to obscure payload size from network observers). A pad length greater than or equal to the +whole payload length is `PROTOCOL_ERROR` (RFC 9113 §6.1), checked before any arithmetic that +could otherwise underflow. Flow-control accounting for padded DATA frames (RFC 9113 §6.9.1: the +*whole* payload counts against the window, not just the data) is Phase 11 scope — `Padding` only +locates the data range, it performs no window bookkeeping itself. + +## Writing: `FrameWriteBuffer`'s back-patching + +A frame's length is rarely known before its payload is serialized (an HPACK-encoded header block, +in particular, has no cheap way to be measured in advance). `FrameWriteBuffer.beginFrame` writes +a 9-byte header with a placeholder length; the caller writes the payload directly through the +same `ByteWriter`; `endFrame` computes the actual length from how far the writer has advanced and +rewrites the three length bytes in place. This is *why* `Http2FrameWriter` (Phase 3) serializes a +complete buffer before ever taking the connection lock, rather than streaming bytes as they are +produced — streaming would need the length upfront, which back-patching deliberately avoids +needing. + +## `EX-37`, found while building this phase's tests + +`BufferedByteSource`'s deadline mechanism (`EX-07`'s actual fix) turned out to have zero dedicated +unit tests and an unconditional `socket.setSoTimeout(...)` call that NPE'd against the `null` +socket every isolated unit test in this codebase uses. Found while writing +`Http2FrameReaderTest`, fixed, and given its own regression suite (`BufferedByteSourceTest`) — +full writeup in the plan's registry, `EX-37`. + +## Testing + +- `Http2FrameReaderTest` — round-trips every frame type, boundary lengths (0, 1, 16383, 16384, + 16385), a frame split across three socket reads, a frame exactly filling the initial buffer, + multiple sequential frames, clean-EOF-vs-mid-frame-EOF, and reserved-bit masking. +- `FrameValidatorTest` — one test per RFC-mandated rejection above, asserting the specific + `Http2ErrorCode`. +- `Http2FrameReaderFuzzTest` — 10 000 000 random-length (0–64 byte), random-content inputs; only + `Http2Exception`, `EOFException`, or `SocketTimeoutException` may escape. Green, ~14s. +- `PaddingTest` — every boundary of the pad-length arithmetic, including the exact + `padLength == payloadLength - 1` (maximum valid) and `padLength >= payloadLength` (rejected) + cases. diff --git a/flash/docs/http2/IMPLEMENTATION-PLAN.md b/flash/docs/http2/IMPLEMENTATION-PLAN.md index adbc011..d74d1c7 100644 --- a/flash/docs/http2/IMPLEMENTATION-PLAN.md +++ b/flash/docs/http2/IMPLEMENTATION-PLAN.md @@ -66,7 +66,7 @@ Status values: `not started` / `in progress` / `blocked` / `done`. | 2 — Transport decomposition | done | `feature/core/http2` | `HttpServer.java` deleted; `transport`/`http1` packages + WS extraction (EX-01/06/11/12/13/14/15/16/32/34) done. Router `ThreadLocal` (EX-06 router half) deliberately deferred to Phase 4 per DEC-15. 311/311 tests green (run 3×). h1 benchmark check deferred — no JMH harness until Phase 3. | | 3 — Serialized frame writer (GO/NO-GO gate) | done | `feature/core/http2` | `Http2FrameWriter`/`WriteIntent`/`IntrusiveMpscQueue` + `Http2FrameWriterTest`/`Http2FrameWriterStressTest` + `FrameWriterBenchmark` (JMH, `-Pjmh`, `src/jmh/java` — moved there from `src/test/java` after it broke default `mvn test`; see `DEC-17`). All 4 gate criteria met: N=1 0 B/op & 42.6 ns overhead (≤50 ns budget); N=64 65.5% throughput retention (≥60%) & 11.8–14.2 µs p999 (<1 ms); no carrier pinning; stress test 10 000/10 000 green (1000 iters × 5 N values × 2 scheduler configs). Full numbers in `WRITER.md`, `DEC-09`. 321/321 non-JMH tests green. | | 4 — Byte-layer foundations | done | `feature/core/http2` | `dev.relism.flash.bytes` package (`ByteScan`+SWAR, `ArrayBackedByteView`, `SegmentedByteView`, `PooledSlice`/`SlicePool`, `ByteWriter`, `Pairs`) built. `EX-04`/`EX-05`/`EX-09`/`EX-19`/`EX-25`/`EX-26`/`EX-33` done, plus `EX-06`'s router half (plan correction, `DEC-19`) removing `FastPathRouterImpl`/`FastPathWsRouterImpl`'s `ThreadLocal`s via an opaque per-connection scratch (`AbstractRouter#newScratch`) instead of extending `ConnectionScratch` (would have created a `routing`→`transport` package cycle). `AbstractRouter`/`AbstractWsRouter.route()` gained a `scratch` param — all call sites updated. Measured (`DEC-20`): SWAR scan 35.4% faster (kept), `EX-04`'s word-path 32.1% faster at the mechanism level (kept; today's router doesn't route through it — `MethodPathByteView` stays non-array-backed by design). Router matching itself is ≈0 B/op including parametric routes. Full h1 pipeline is 120.008 B/op, 100% attributable to `Request`/`RequestBody`/`RequestLine` construction — explicitly Phase 6 scope, not a Phase 4 regression. Two documented (non-hot-path) anonymous-`ByteView` fallbacks remain in `QueryParams`/`PathParams.view`. `BYTES.md` written. 395/395 tests green (both with and without `-Pjmh`). | -| 5 — Frame layer | not started | — | — | +| 5 — Frame layer | done | `feature/core/http2` | `FrameType`/`FrameFlags`/`FrameHeader`/`Http2FrameReader`/`FrameValidator`/`Padding`/`FrameWriteBuffer` built. All 10 frame types read/validated/written; per-type RFC error codes verified individually (`FrameValidatorTest`); fuzz-tested 10M random inputs (~14s, green). Zero-alloc contract measured, not asserted: read+validate+consume 0.002 B/op, write ≈10⁻⁴ B/op (`DEC-21`). Found+fixed `EX-37` (`BufferedByteSource`'s deadline mechanism NPE'd against a `null` socket — zero prior test coverage of `EX-07`'s own fix; added `BufferedByteSourceTest`). `FRAMES.md` written. 449/449 tests green. | | 6 — Request/Response model refactor | not started | — | — | | 7 — HPACK decoder | not started | — | — | | 8 — Connection state machine | not started | — | — | @@ -630,6 +630,30 @@ RFC 9112 §5 gives no such leniency: a header field line without a colon is not **Fix**: `colon == -1` now rejects the request with `400 Bad Request`. **Phase**: 1. +### EX-37 — `BufferedByteSource`'s deadline mechanism NPEs against a `null` socket, so it was never actually testable in isolation +Found while writing `Http2FrameReaderTest` (Phase 5): `BufferedByteSource.clearDeadline()` and +`fillFromUnderlying()` both call `socket.setSoTimeout(...)` unconditionally. Every isolated unit +test in this codebase that constructs a `BufferedByteSource` directly (over a +`ByteArrayInputStream`, to test a parser/reader without a real connection) passes `null` for +`socket` — the codebase's own established idiom, used throughout `RequestParserTest`, +`ChunkedInputStreamTest`, `RequestParserSecurityTest`. That idiom works today only because none +of those tests ever call `setDeadline`/trigger a deadline-bounded read — `RequestParser` itself +never calls `setDeadline` (only `Http1Connection`, which always has a real socket, does). The +moment any code under test (here, `Http2FrameReader`, which correctly uses the deadline exactly +as `EX-07` designed it) sets a deadline and then performs a read against a `null`-socket source, +both methods threw `NullPointerException` instead of the intended `SocketTimeoutException`/ +normal read. `BufferedByteSource` — the class that exists specifically to implement `EX-07`'s +slowloris defence — had **zero** dedicated unit tests (`BufferedByteSourceTest` did not exist); +its deadline mechanism was exercised only indirectly, end-to-end, via real-socket tests +(`HttpServerTimeoutTest`), which never hit this path. +**Fix**: both methods now skip the `socket.setSoTimeout(...)` call when `socket == null` — a +`null` socket means "no OS-level timeout to bound", not a misuse; the deadline-expiry check +itself (`remainingNanos <= 0` → `SocketTimeoutException`) is independent of the socket and keeps +working. Production always supplies a real socket, so no production behavior changes. +`BufferedByteSourceTest.java` added (previously absent) with direct coverage of the deadline +mechanism against a `null` socket, closing the actual test gap this bug lived in. +**Phase**: 5 (found and fixed while building `Http2FrameReaderTest`). + --- # PART III — The phases @@ -1602,36 +1626,57 @@ Created: payload copy at this layer (the payload stays in the read buffer; copies happen above, per the layer that needs to retain it). - Writing a frame header: 0 B/op (writes into the existing scratch). +- [x] **Measured**, not just asserted: `FrameLayerBenchmark` (`-prof gc`) — read+validate+consume + 0.002 B/op, write 10⁻⁴ B/op, both indistinguishable from zero. `DECISIONS.md`, `DEC-21`. ### Safety checks -- [ ] Declared length checked against `SETTINGS_MAX_FRAME_SIZE` **before** any buffer growth -- [ ] Buffer growth bounded and monotonic (never shrink mid-connection; shrink only on release - to the pool if the high-water mark was pathological) -- [ ] Per-type length/stream-id/flag validation table complete for all 10 types -- [ ] Unknown types ignored; unknown types inside a header block rejected -- [ ] Reserved bit masked, not rejected -- [ ] Padding length validated against frame length -- [ ] Frame read is timeout-bounded (reuse `bodyReadTimeoutMs` semantics or add - `Http2Limits.FRAME_READ_TIMEOUT_MS`) +- [x] Declared length checked against `SETTINGS_MAX_FRAME_SIZE` **before** any buffer growth — + `Http2FrameReader.readFrame` checks `declaredLength > MAX_FRAME_SIZE_LOCAL` immediately + after decoding the header, before the payload-sized `ensureAvailable` call that would grow + the buffer. +- [x] Buffer growth bounded and monotonic — grows only to accommodate `9 + declaredLength`, + itself already bounded by the check above; never shrinks (matches `RequestParser`'s own + buffer policy, not yet pool-released — no per-connection buffer pool exists before Phase 13). +- [x] Per-type length/stream-id/flag validation table complete for all 10 types — `FrameType`'s + constants + `FrameValidator`, one `FrameValidatorTest` case per RFC-mandated rejection. +- [x] Unknown types ignored; unknown types inside a header block rejected — + `FrameValidator.validate`'s `insideHeaderBlock` parameter, + `unknownType_outsideHeaderBlock_isIgnoredNotRejected`/`unknownType_insideHeaderBlock_isProtocolError`. +- [x] Reserved bit masked, not rejected — `FrameHeader.reset` masks it out of `streamId()`; + `reservedBitInStreamId_isMaskedNotRejected`. +- [x] Padding length validated against frame length — `Padding.unpad`, `PaddingTest`'s boundary + cases (`padLength == payloadLength - 1` valid, `padLength >= payloadLength` rejected). +- [x] Frame read is timeout-bounded — `Http2Limits.FRAME_READ_TIMEOUT_MS` (new constant, this + phase), enforced via `BufferedByteSource`'s existing deadline mechanism. ### Tests - `Http2FrameReaderTest` — round-trip every frame type; boundary lengths 0, 1, 16383, 16384, - 16385; a frame split across three socket reads; a frame exactly filling the buffer. + 16385; a frame split across three socket reads; a frame exactly filling the buffer; multiple + sequential frames; reserved-bit masking. - `FrameValidatorTest` — one test per RFC-mandated rejection, asserting the **specific** error code, not merely that an error occurred. -- `Http2FrameReaderFuzzTest` — random bytes into the reader; assert only `Http2Exception` or - `Http2StreamException` escapes (never `ArrayIndexOutOfBoundsException`, `NegativeArraySizeException`, - `OutOfMemoryError`, or an infinite loop — enforce with a per-case timeout). +- `Http2FrameReaderFuzzTest` — 10 000 000 random-length, random-content inputs — **plan + correction**: asserts only `Http2Exception`, `EOFException`, or `SocketTimeoutException` + escapes, not `Http2Exception`/`Http2StreamException` as originally written here. + `Http2StreamException` is stream-scoped and this phase has no stream concept yet (Phase 10); + `EOFException`/`SocketTimeoutException` are the correctly-typed outcomes for a fuzz input that + truncates mid-frame or (in principle) times out — both legitimate, expected rejections of + malformed/incomplete input, not bugs. Any other exception type still fails the test. Green, + ~14s. - `PaddingTest`. +- `BufferedByteSourceTest` — new, not originally planned for this phase: regression coverage for + `EX-37`, a `NullPointerException` bug in `BufferedByteSource`'s deadline mechanism found while + writing `Http2FrameReaderTest` (see the registry entry for the full writeup — a plain bug fix, + not a design decision, so no `DECISIONS.md` entry). ### Docs -`flash/docs/http2/FRAMES.md` — the wire format, the validation table (as an actual table, one row per -frame type, with the RFC section for each rule), and the ignore-vs-reject policy. +- [x] `flash/docs/http2/FRAMES.md` — the wire format, the validation table (as an actual table, + one row per frame type, with the RFC section for each rule), and the ignore-vs-reject policy. ### DoD -- [ ] All 10 frame types read, validated, and written. -- [ ] Fuzz test green for 10 million random inputs. -- [ ] `flash/docs/http2/FRAMES.md` complete with the validation table. +- [x] All 10 frame types read, validated, and written — `roundTrip_everyFrameType`. +- [x] Fuzz test green for 10 million random inputs — `Http2FrameReaderFuzzTest`, ~14s. +- [x] `flash/docs/http2/FRAMES.md` complete with the validation table. --- diff --git a/flash/src/jmh/java/dev/relism/flash/h2/frame/FrameLayerBenchmark.java b/flash/src/jmh/java/dev/relism/flash/h2/frame/FrameLayerBenchmark.java new file mode 100644 index 0000000..f49e16e --- /dev/null +++ b/flash/src/jmh/java/dev/relism/flash/h2/frame/FrameLayerBenchmark.java @@ -0,0 +1,113 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.bytes.ByteWriter; +import dev.relism.flash.transport.BufferedByteSource; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +import java.io.IOException; +import java.io.InputStream; +import java.util.concurrent.TimeUnit; + +/** + * Phase 5's zero-alloc contract: "Reading, validating and discarding a frame: 0 B/op ... Writing + * a frame header: 0 B/op." Measured with {@code -prof gc}, not merely asserted — see + * {@code DECISIONS.md}, {@code DEC-21}, for the recorded numbers. + * + *

Uses the same hand-rolled repeating {@link InputStream} technique + * {@code RequestPipelineBenchmark} (Phase 4) established: one {@link BufferedByteSource}/ + * {@link Http2FrameReader} pair created once per trial and reused across every invocation, + * matching how a real connection's demux loop owns exactly one of each for its whole lifetime, + * rather than paying for harness-side (re)construction inside the timed path. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(2) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 5, time = 1) +public class FrameLayerBenchmark { + + private static final class RepeatingByteStream extends InputStream { + private final byte[] template; + private int pos; + + RepeatingByteStream(byte[] template) { + this.template = template; + } + + @Override + public int read() { + byte b = template[pos]; + pos = (pos + 1) % template.length; + return b & 0xFF; + } + + @Override + public int read(byte[] dst, int off, int len) { + for (int i = 0; i < len; i++) { + dst[off + i] = template[pos]; + pos = (pos + 1) % template.length; + } + return len; + } + } + + // ── Read + validate ────────────────────────────────────────────────────── + + private Http2FrameReader reader; + + @Setup(Level.Trial) + public void setupReader() { + FrameWriteBuffer out = new FrameWriteBuffer(new ByteWriter(64)); + out.beginFrame(FrameType.HEADERS, FrameFlags.END_HEADERS, 1); + byte[] payload = new byte[48]; + for (int i = 0; i < payload.length; i++) payload[i] = (byte) i; + out.writer().writeBytes(payload); + out.endFrame(); + byte[] template = new byte[out.writer().length()]; + System.arraycopy(out.writer().array(), 0, template, 0, template.length); + + BufferedByteSource src = new BufferedByteSource(new RepeatingByteStream(template), null); + reader = new Http2FrameReader(src); + } + + @Benchmark + public int readValidateAndDiscard() throws IOException { + FrameHeader header = reader.readFrame(); + FrameValidator.validate(header, false); + int checksum = header.buffer()[header.payloadOffset()]; + reader.consumeFrame(); + return checksum; + } + + // ── Write ──────────────────────────────────────────────────────────────── + + private FrameWriteBuffer writeBuffer; + private byte[] writePayload; + + @Setup(Level.Trial) + public void setupWriter() { + writeBuffer = new FrameWriteBuffer(new ByteWriter(64)); + writePayload = new byte[48]; + for (int i = 0; i < writePayload.length; i++) writePayload[i] = (byte) i; + } + + @Benchmark + public int writeFrame() { + writeBuffer.writer().reset(); + writeBuffer.beginFrame(FrameType.HEADERS, FrameFlags.END_HEADERS, 1); + writeBuffer.writer().writeBytes(writePayload); + writeBuffer.endFrame(); + return writeBuffer.writer().length(); + } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/Http2Limits.java b/flash/src/main/java/dev/relism/flash/h2/Http2Limits.java index 5b950aa..d6d5582 100644 --- a/flash/src/main/java/dev/relism/flash/h2/Http2Limits.java +++ b/flash/src/main/java/dev/relism/flash/h2/Http2Limits.java @@ -156,4 +156,13 @@ public final class Http2Limits { * {@code Socket#setSoTimeout} — that option bounds reads, not writes. */ public static final long WRITE_TIMEOUT_MS = 30_000; + + /** + * Maximum time, in milliseconds, {@code Http2FrameReader} may wait for a single frame's + * header and payload to fully arrive. Bounds the same slowloris-shaped hazard {@code + * BufferedByteSource}'s deadline mechanism already defends h1 against ({@code EX-07}): + * without it, a peer that sends 9 header bytes and then never sends the declared payload + * would hold this connection's frame reader waiting forever. + */ + public static final long FRAME_READ_TIMEOUT_MS = 20_000; } diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/FrameFlags.java b/flash/src/main/java/dev/relism/flash/h2/frame/FrameFlags.java new file mode 100644 index 0000000..c92f35e --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/FrameFlags.java @@ -0,0 +1,39 @@ +package dev.relism.flash.h2.frame; + +/** + * The frame-header flag bits (RFC 9113 §6), as bitwise constants plus predicate helpers. + * + *

The deliberate collision

+ * Bit {@code 0x1} means different things on different frame types: {@link #END_STREAM} on + * {@code DATA}/{@code HEADERS}, {@link #ACK} on {@code SETTINGS}/{@code PING}. They are the same + * bit position because the RFC defines flags per-type, not globally — reusing the numeric value + * is intentional on the wire, not a naming accident here. **Never call {@link #isEndStream} on a + * SETTINGS/PING frame's flags, or {@link #isAck} on a DATA/HEADERS frame's** — each predicate is + * named for the one frame type family it is valid to call it on; mixing them up silently + * misreads an unrelated bit rather than throwing, because the bit pattern is, by construction, + * identical. + * + *

RFC 9113 §4.1: flag bits not defined for a frame's type MUST be ignored on receipt and MUST + * NOT be set when sending. This class only ever tests bits it defines for the type the caller is + * working with; undefined bits are never inspected. + */ +public final class FrameFlags { + private FrameFlags() {} + + /** DATA/HEADERS: no more frames will be sent for this stream in this direction. */ + public static final int END_STREAM = 0x1; + /** SETTINGS/PING: this frame acknowledges the peer's own frame, rather than proposing new values. */ + public static final int ACK = 0x1; + /** HEADERS/PUSH_PROMISE/CONTINUATION: the header block is complete — no CONTINUATION follows. */ + public static final int END_HEADERS = 0x4; + /** DATA/HEADERS/PUSH_PROMISE: a pad-length byte and trailing padding are present — see {@link Padding}. */ + public static final int PADDED = 0x8; + /** HEADERS: deprecated stream-dependency/weight fields are present (RFC 9113 §5.3.2 — parsed and discarded). */ + public static final int PRIORITY = 0x20; + + public static boolean isEndStream(int flags) { return (flags & END_STREAM) != 0; } + public static boolean isAck(int flags) { return (flags & ACK) != 0; } + public static boolean isEndHeaders(int flags) { return (flags & END_HEADERS) != 0; } + public static boolean isPadded(int flags) { return (flags & PADDED) != 0; } + public static boolean hasPriority(int flags) { return (flags & PRIORITY) != 0; } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/FrameHeader.java b/flash/src/main/java/dev/relism/flash/h2/frame/FrameHeader.java new file mode 100644 index 0000000..9220a58 --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/FrameHeader.java @@ -0,0 +1,84 @@ +package dev.relism.flash.h2.frame; + +/** + * A flyweight over one frame's 9-byte header plus its payload location, both still living + * in {@link Http2FrameReader}'s own read buffer. One instance per connection, {@link #reset} + * in place by every {@link Http2FrameReader#readFrame()} call — never allocated per frame + * (mirrors the existing {@code WebSocketFrame} reuse idiom in {@code dev.relism.flash.websocket}). + * + *

Lifetime contract

+ * Valid only until the next {@link Http2FrameReader#readFrame()}/{@code consumeFrame()} call on + * the same reader — same "do not retain past the handler" rule the rest of this codebase's + * buffer-backed flyweights (`HeaderMap`, `WebSocketFrame`) already document. The payload bytes + * are also transient: whatever layer needs to retain a DATA frame's payload past this window + * must copy it out (R3 — the connection read buffer is shared, single-threaded, and reused). + * + *

Reserved bit and unknown types

+ * {@link #streamId()} has already had the wire's reserved high bit (RFC 9113 §4.1: "R: A + * reserved 1-bit field... The semantics of this bit are undefined, and the bit MUST be ignored + * when receiving") masked off during {@link #reset} — callers never see it and never need to + * mask it themselves. {@link #type()} is {@code null} for a type code {@link FrameType} does not + * recognise (i.e. {@code typeCode() > FrameType.maxKnown()}); per RFC 9113 §4.1 such frames must + * be ignored, not rejected — {@link #typeCode()} remains available so the caller can still log + * or count it before skipping the payload. + */ +public final class FrameHeader { + private byte[] buf; + private int length; + private int typeCode; + private FrameType type; + private int flags; + private int streamId; + private int payloadOffset; + + /** Called by {@link Http2FrameReader} only, once the full 9-byte header is available at {@code buf[off]}. */ + void reset(byte[] buf, int off) { + this.buf = buf; + int b0 = buf[off] & 0xFF, b1 = buf[off + 1] & 0xFF, b2 = buf[off + 2] & 0xFF; + this.length = (b0 << 16) | (b1 << 8) | b2; + this.typeCode = buf[off + 3] & 0xFF; + this.type = FrameType.fromCode(typeCode); + this.flags = buf[off + 4] & 0xFF; + // RFC 9113 §4.1: the top bit of byte 5 is reserved and MUST be ignored on receipt — + // masked here, once, rather than requiring every caller to remember to. + int b5 = buf[off + 5] & 0x7F; + int b6 = buf[off + 6] & 0xFF, b7 = buf[off + 7] & 0xFF, b8 = buf[off + 8] & 0xFF; + this.streamId = (b5 << 24) | (b6 << 16) | (b7 << 8) | b8; + this.payloadOffset = off + 9; + } + + /** Payload length in bytes, as declared by the frame header (0..2^24-1 before any limit check). */ + public int length() { + return length; + } + + /** The raw wire type byte, valid even when {@link #type()} is {@code null} (an unrecognised type). */ + public int typeCode() { + return typeCode; + } + + /** The recognised frame type, or {@code null} if {@link #typeCode()} is not one of RFC 9113's 10. */ + public FrameType type() { + return type; + } + + /** The raw flags byte — interpret via {@link FrameFlags}, which is type-specific. */ + public int flags() { + return flags; + } + + /** Stream identifier, reserved bit already masked. {@code 0} means "the connection itself". */ + public int streamId() { + return streamId; + } + + /** The backing buffer — see the class Javadoc's lifetime contract before retaining a reference. */ + public byte[] buffer() { + return buf; + } + + /** Offset of the first payload byte within {@link #buffer()}. Payload spans {@code [payloadOffset(), payloadOffset() + length())}. */ + public int payloadOffset() { + return payloadOffset; + } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/FrameType.java b/flash/src/main/java/dev/relism/flash/h2/frame/FrameType.java new file mode 100644 index 0000000..f2215fd --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/FrameType.java @@ -0,0 +1,88 @@ +package dev.relism.flash.h2.frame; + +/** + * The 10 HTTP/2 frame types (RFC 9113 §6), plus the shared per-type validation rules + * {@link FrameValidator} enforces. Values above {@code 0x9} are not assigned a constant here — + * RFC 9113 §4.1 requires unknown types to be silently ignored (read and discard the payload), + * which {@link Http2FrameReader}'s caller implements by checking {@code type > + * FrameType.maxKnown()} rather than by this enum growing an {@code UNKNOWN} member (an + * {@code UNKNOWN} constant would misleadingly suggest "a recognised category of unrecognised + * frame", when the correct handling is simply "not this table, skip it"). + * + *

Per-type validation, table-driven (R4)

+ * Each constant carries the RFC-mandated payload length bounds, whether a zero stream id is + * required/forbidden/either, and whether the frame counts toward the CONTINUATION-flood guard + * ({@code EX}-style defence, {@code Http2Limits#MAX_CONTINUATION_FRAMES_PER_BLOCK}) — see + * {@link FrameValidator} for how these are applied and the specific RFC citation per rule. + */ +public enum FrameType { + /** RFC 9113 §6.1. Stream body bytes. Stream id required. Length: 0..MAX_FRAME_SIZE. */ + DATA(0x0, 0, Integer.MAX_VALUE, StreamIdRule.REQUIRED), + /** RFC 9113 §6.2. Header block fragment (HPACK). Stream id required. */ + HEADERS(0x1, 0, Integer.MAX_VALUE, StreamIdRule.REQUIRED), + /** RFC 9113 §6.3. Deprecated priority signal — parsed and discarded, never acted on (DEC, Phase 5 task 7). */ + PRIORITY(0x2, 5, 5, StreamIdRule.REQUIRED), + /** RFC 9113 §6.4. Stream-level error. Exactly 4 bytes (the error code). Stream id required. */ + RST_STREAM(0x3, 4, 4, StreamIdRule.REQUIRED), + /** RFC 9113 §6.5. Connection-level parameters. Length must be a multiple of 6. Stream id must be 0. */ + SETTINGS(0x4, 0, Integer.MAX_VALUE, StreamIdRule.FORBIDDEN), + /** RFC 9113 §6.6. Never sent (Flash advertises {@code SETTINGS_ENABLE_PUSH=0}); receiving one from a client is a protocol error. */ + PUSH_PROMISE(0x5, 4, Integer.MAX_VALUE, StreamIdRule.REQUIRED), + /** RFC 9113 §6.7. Connection liveness / RTT probe. Exactly 8 bytes of opaque data. Stream id must be 0. */ + PING(0x6, 8, 8, StreamIdRule.FORBIDDEN), + /** RFC 9113 §6.8. Connection shutdown notice. At least 8 bytes (last-stream-id + error code). Stream id must be 0. */ + GOAWAY(0x7, 8, Integer.MAX_VALUE, StreamIdRule.FORBIDDEN), + /** RFC 9113 §6.9. Flow-control window increment. Exactly 4 bytes. Stream id may be either (0 = connection window). */ + WINDOW_UPDATE(0x8, 4, 4, StreamIdRule.EITHER), + /** RFC 9113 §6.10. Continuation of a header block that did not fit one HEADERS/PUSH_PROMISE frame. Stream id required. */ + CONTINUATION(0x9, 0, Integer.MAX_VALUE, StreamIdRule.REQUIRED); + + /** Whether a frame type requires stream id 0, requires it non-zero, or permits either. */ + public enum StreamIdRule { REQUIRED, FORBIDDEN, EITHER } + + private static final FrameType[] BY_CODE = new FrameType[values().length]; + + static { + for (FrameType t : values()) { + BY_CODE[t.code] = t; + } + } + + private final int code; + private final int minLength; + private final int maxLength; + private final StreamIdRule streamIdRule; + + FrameType(int code, int minLength, int maxLength, StreamIdRule streamIdRule) { + this.code = code; + this.minLength = minLength; + this.maxLength = maxLength; + this.streamIdRule = streamIdRule; + } + + public int code() { + return code; + } + + public int minLength() { + return minLength; + } + + public int maxLength() { + return maxLength; + } + + public StreamIdRule streamIdRule() { + return streamIdRule; + } + + /** The highest type code this enum recognises — anything above must be ignored per RFC 9113 §4.1. */ + public static int maxKnown() { + return CONTINUATION.code; + } + + /** Looks up the constant for a wire type byte, or {@code null} if it is an unrecognised (to-be-ignored) type. */ + public static FrameType fromCode(int code) { + return (code >= 0 && code < BY_CODE.length) ? BY_CODE[code] : null; + } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/FrameValidator.java b/flash/src/main/java/dev/relism/flash/h2/frame/FrameValidator.java new file mode 100644 index 0000000..4a96637 --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/FrameValidator.java @@ -0,0 +1,90 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.h2.Http2ErrorCode; +import dev.relism.flash.h2.Http2Exception; +import dev.relism.flash.h2.Http2Limits; + +/** + * Table-driven RFC 9113 per-frame-type validation: length bounds, the stream-id + * required/forbidden/either rule, and the two special-cased structural rules ({@code SETTINGS}' + * multiple-of-6 length, {@code PUSH_PROMISE} always rejected from a client) that do not fit a + * generic min/max/stream-id table. Table itself lives on {@link FrameType}'s constants (R4); this + * class is the code that reads it. + * + *

The error code is not uniform — read the RFC per violation, not just per type. A + * {@code SETTINGS} frame with a bad length is {@code FRAME_SIZE_ERROR}; the same frame with a + * non-zero stream id is {@code PROTOCOL_ERROR}. This class throws the specific code each + * violation's own RFC citation requires, not a single blanket code per type. + */ +public final class FrameValidator { + private FrameValidator() {} + + /** + * Validates {@code header} against RFC 9113's rules for its type. + * + * @param insideHeaderBlock whether this frame arrived between a HEADERS/PUSH_PROMISE frame + * lacking {@code END_HEADERS} and its terminating CONTINUATION — + * changes the handling of an unrecognised type (§6.10: a + * {@code PROTOCOL_ERROR}, not the usual silent ignore, since an + * in-progress header block cannot tolerate an interloper frame of + * any kind without desynchronizing HPACK's stateful decode) + * @throws Http2Exception on any RFC violation, with the specific error code the violated + * rule mandates + */ + public static void validate(FrameHeader header, boolean insideHeaderBlock) { + FrameType type = header.type(); + + if (type == null) { + // RFC 9113 §4.1: unknown frame types MUST be ignored — except inside an in-progress + // header block (§6.10), where anything other than CONTINUATION desynchronizes HPACK. + if (insideHeaderBlock) { + throw Http2Exception.of(Http2ErrorCode.PROTOCOL_ERROR, + "unrecognised frame type " + header.typeCode() + " received inside a header block"); + } + return; + } + + int length = header.length(); + + // RFC 9113 §6.5: a SETTINGS frame's length MUST be a multiple of 6 (each entry is a + // 2-byte identifier + 4-byte value). Checked before the generic bounds below, since the + // generic table only expresses a min/max range, not a modulus. + if (type == FrameType.SETTINGS && length % 6 != 0) { + throw Http2Exception.FRAME_SIZE_ERROR; + } + + if (length < type.minLength() || length > type.maxLength()) { + throw Http2Exception.FRAME_SIZE_ERROR; + } + + // Redundant with Http2FrameReader's own pre-allocation check for frames it read itself, + // but this method must also be correct for a FrameHeader built any other way (tests, + // and — in later phases — frames reassembled from multiple reads), so the bound is + // re-asserted here rather than trusted from the caller. + if (length > Http2Limits.MAX_FRAME_SIZE_LOCAL) { + throw Http2Exception.FRAME_SIZE_ERROR; + } + + int streamId = header.streamId(); + switch (type.streamIdRule()) { + case REQUIRED -> { + if (streamId == 0) { + throw Http2Exception.of(Http2ErrorCode.PROTOCOL_ERROR, type + " requires a non-zero stream id"); + } + } + case FORBIDDEN -> { + if (streamId != 0) { + throw Http2Exception.of(Http2ErrorCode.PROTOCOL_ERROR, type + " must have stream id 0, got " + streamId); + } + } + case EITHER -> { /* WINDOW_UPDATE: 0 (connection window) or non-zero (stream window) both valid */ } + } + + // RFC 9113 §8.4 / this codebase's DEC-10: PUSH_PROMISE is a server-to-client-only frame + // (Flash advertises SETTINGS_ENABLE_PUSH=0 and never sends one); receiving one at all + // means the peer believes it is talking to a client, which is always a protocol error. + if (type == FrameType.PUSH_PROMISE) { + throw Http2Exception.of(Http2ErrorCode.PROTOCOL_ERROR, "PUSH_PROMISE received from a client"); + } + } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/FrameWriteBuffer.java b/flash/src/main/java/dev/relism/flash/h2/frame/FrameWriteBuffer.java new file mode 100644 index 0000000..221ccb0 --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/FrameWriteBuffer.java @@ -0,0 +1,76 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.bytes.ByteWriter; + +/** + * Serializes HTTP/2 frames into a {@link ByteWriter} scratch buffer with the standard + * length-back-patching technique: {@link #beginFrame} writes a 9-byte header with a placeholder + * length, the caller writes the payload directly through {@link #writer()} (the same + * {@link ByteWriter}), and {@link #endFrame} rewrites the length once it is known — the payload + * size is rarely known before it is serialized (an HPACK-encoded header block, in particular, + * has no cheap way to be measured in advance). + * + *

This is the reason {@link Http2FrameWriter} (Phase 3) serializes a complete buffer and + * issues one bulk {@code write}, rather than streaming bytes as they are produced: streaming + * would require knowing the length before the first byte goes out, which back-patching + * deliberately avoids needing. + * + *

Usage

+ * 
{@code
+ * FrameWriteBuffer out = new FrameWriteBuffer(new ByteWriter(4096));
+ * out.beginFrame(FrameType.SETTINGS, 0, 0);
+ * out.writer().writeUInt16(SETTINGS_MAX_CONCURRENT_STREAMS);
+ * out.writer().writeUInt32(100);
+ * out.endFrame();
+ * // out.writer().array()[0, out.writer().length()) now holds one complete, correctly-lengthed frame
+ * }
+ * + *

Multiple frames, one buffer

+ * {@link #beginFrame}/{@link #endFrame} pairs may be repeated on the same instance without a + * {@link ByteWriter#reset()} between them — each pair appends one more complete frame after + * whatever was already written, which is exactly what {@link Http2FrameWriter#write} wants for a + * single bulk write covering several frames (e.g. HEADERS followed immediately by its first + * DATA frame). + * + *

Thread-safety

+ * Not thread-safe — exactly one writer at a time, the same convention every other per-connection + * scratch object in this codebase follows. + */ +public final class FrameWriteBuffer { + private final ByteWriter writer; + private int headerStart = -1; + + public FrameWriteBuffer(ByteWriter writer) { + this.writer = writer; + } + + /** The underlying {@link ByteWriter} — write the frame's payload directly through this between {@link #beginFrame} and {@link #endFrame}. */ + public ByteWriter writer() { + return writer; + } + + /** Writes a 9-byte frame header with a placeholder length, to be filled in by {@link #endFrame}. */ + public void beginFrame(FrameType type, int flags, int streamId) { + if (headerStart != -1) { + throw new IllegalStateException("beginFrame() called again before the previous frame's endFrame()"); + } + headerStart = writer.length(); + writer.writeUInt24(0); // length placeholder + writer.writeByte((byte) type.code()); + writer.writeByte((byte) flags); + writer.writeUInt31(streamId); + } + + /** Back-patches the length field written by {@link #beginFrame} now that the payload's size is known. */ + public void endFrame() { + if (headerStart == -1) { + throw new IllegalStateException("endFrame() called without a matching beginFrame()"); + } + int payloadLength = writer.length() - (headerStart + 9); + byte[] buf = writer.array(); + buf[headerStart] = (byte) (payloadLength >>> 16); + buf[headerStart + 1] = (byte) (payloadLength >>> 8); + buf[headerStart + 2] = (byte) payloadLength; + headerStart = -1; + } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/Http2FrameReader.java b/flash/src/main/java/dev/relism/flash/h2/frame/Http2FrameReader.java new file mode 100644 index 0000000..305266c --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/Http2FrameReader.java @@ -0,0 +1,133 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.h2.Http2Exception; +import dev.relism.flash.h2.Http2Limits; +import dev.relism.flash.transport.BufferedByteSource; + +import java.io.EOFException; +import java.io.IOException; +import java.util.Arrays; + +/** + * Reads length-prefixed HTTP/2 frames from one connection's {@link BufferedByteSource}. Simpler + * than {@code RequestParser} by construction: HTTP/2 frames declare their length up front (the + * 9-byte header), so nothing is ever scanned for — {@code Http2FrameReader} only ever needs to + * know "do I have N bytes yet", never "where does this end". + * + *

Buffer discipline

+ * One growable {@code byte[]} per connection, reused across every frame — the same + * compact-before-grow discipline {@code RequestParser}'s own buffer uses. A frame's declared + * length is checked against {@link Http2Limits#MAX_FRAME_SIZE_LOCAL} before the buffer + * is ever grown to accommodate it (R8): a hostile 16 MB declared length is rejected at the + * length-check, not after an allocation already paid for it. + * + *

Usage

+ * 
{@code
+ * FrameHeader header = reader.readFrame();
+ * if (header == null) { /* clean EOF between frames — connection closing *\/ }
+ * // ... process header.buffer()[header.payloadOffset(), +header.length()) ...
+ * reader.consumeFrame(); // MUST be called before the next readFrame()
+ * }
+ * + *

Thread-safety

+ * Not thread-safe — exactly one virtual thread (the connection's demux loop) ever calls this, + * the same invariant every other per-connection reader in this codebase assumes. + */ +public final class Http2FrameReader { + private static final int FRAME_HEADER_SIZE = 9; + private static final int INITIAL_BUFFER_SIZE = 16 * 1024; + + private final BufferedByteSource in; + private final FrameHeader header = new FrameHeader(); + private byte[] buffer; + private int base; // offset of the first unconsumed byte + private int totalRead; // count of valid unconsumed bytes at [base, base + totalRead) + + public Http2FrameReader(BufferedByteSource in) { + this(in, INITIAL_BUFFER_SIZE); + } + + public Http2FrameReader(BufferedByteSource in, int initialBufferSize) { + this.in = in; + this.buffer = new byte[Math.max(initialBufferSize, FRAME_HEADER_SIZE)]; + } + + /** + * Reads the next frame's header and payload, bounded by + * {@link Http2Limits#FRAME_READ_TIMEOUT_MS}, and returns the reused {@link FrameHeader} + * flyweight positioned over it — or {@code null} on a clean EOF between frames (the peer + * closed the connection while nothing was in flight; not an error). + * + *

The caller MUST call {@link #consumeFrame()} exactly once after processing this frame + * (or deciding to discard it) and before calling this method again. + * + * @throws Http2Exception if the declared length exceeds {@link Http2Limits#MAX_FRAME_SIZE_LOCAL} + * @throws EOFException if the connection closes after a frame has already started arriving + * @throws java.net.SocketTimeoutException if {@link Http2Limits#FRAME_READ_TIMEOUT_MS} elapses + */ + public FrameHeader readFrame() throws IOException { + in.setDeadline(System.nanoTime() + Http2Limits.FRAME_READ_TIMEOUT_MS * 1_000_000L); + try { + if (!ensureAvailable(FRAME_HEADER_SIZE)) { + return null; // clean EOF: nothing buffered yet, peer closed between frames + } + int declaredLength = decodeLength(buffer, base); + // R8: checked BEFORE any further buffer growth or read — a hostile declared length + // never causes an oversized allocation, only a rejection. + if (declaredLength > Http2Limits.MAX_FRAME_SIZE_LOCAL) { + throw Http2Exception.FRAME_SIZE_ERROR; + } + ensureAvailable(FRAME_HEADER_SIZE + declaredLength); + header.reset(buffer, base); + return header; + } finally { + in.clearDeadline(); + } + } + + /** Advances past the frame last returned by {@link #readFrame()}. Zero-copy, zero-allocation. */ + public void consumeFrame() { + int consumed = FRAME_HEADER_SIZE + header.length(); + base += consumed; + totalRead -= consumed; + if (totalRead == 0) { + base = 0; // nothing buffered — reset to the front rather than drifting forever + } + } + + private static int decodeLength(byte[] buf, int off) { + int b0 = buf[off] & 0xFF, b1 = buf[off + 1] & 0xFF, b2 = buf[off + 2] & 0xFF; + return (b0 << 16) | (b1 << 8) | b2; + } + + /** + * Ensures at least {@code need} bytes are available starting at {@link #base}, growing or + * compacting the buffer as necessary. Returns {@code false} only for a clean EOF with + * nothing at all buffered yet (the between-frames case); an EOF after any bytes of the + * current frame have already arrived is a genuine truncation and throws. + */ + private boolean ensureAvailable(int need) throws IOException { + while (totalRead < need) { + if (base + need > buffer.length) { + if (base > 0) { + // Compact: slide unconsumed bytes to the front — frees room without growing. + System.arraycopy(buffer, base, buffer, 0, totalRead); + base = 0; + } else { + // need <= 9 + MAX_FRAME_SIZE_LOCAL always, by readFrame()'s own check before + // the payload-sized call — grow exactly enough, never unbounded. + int grown = buffer.length; + while (grown < need) grown *= 2; + buffer = Arrays.copyOf(buffer, grown); + } + } + int n = in.read(buffer, base + totalRead, buffer.length - base - totalRead); + if (n < 0) { + if (totalRead == 0) return false; + throw new EOFException("connection closed mid-frame (" + totalRead + "/" + need + " bytes read)"); + } + totalRead += n; + } + return true; + } +} diff --git a/flash/src/main/java/dev/relism/flash/h2/frame/Padding.java b/flash/src/main/java/dev/relism/flash/h2/frame/Padding.java new file mode 100644 index 0000000..9105171 --- /dev/null +++ b/flash/src/main/java/dev/relism/flash/h2/frame/Padding.java @@ -0,0 +1,67 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.bytes.Pairs; +import dev.relism.flash.h2.Http2ErrorCode; +import dev.relism.flash.h2.Http2Exception; + +/** + * RFC 9113 §6.1 (DATA) / §6.2 (HEADERS) padding. When {@link FrameFlags#PADDED} is set, a + * frame's payload is laid out as: 1 pad-length byte, then the actual data (or header-block + * fragment), then that many padding bytes (RFC 9113 gives no meaning to the padding bytes + * themselves — they exist only to obscure payload size from network observers). + * + *

Padding is not optional to support: any client may send it on DATA or HEADERS + * regardless of whether the server ever sends padded frames itself. + * + *

Flow control (forward note, not implemented here)

+ * RFC 9113 §6.9.1: padding bytes count against the DATA flow-control window even though they + * carry no data — the whole frame payload (pad-length byte + data + padding) is what a + * future Phase 11 flow controller must subtract from the window, not just {@link + * #dataLength(long)}. This class only locates the data range within the payload; it performs no + * flow-control accounting itself. + */ +public final class Padding { + private Padding() {} + + /** + * Locates the actual data range within a payload that may or may not be padded. When + * {@code padded} is {@code false}, returns the whole payload unchanged (zero-cost — no + * padding byte to read, no arithmetic beyond the pack). When {@code true}, reads the + * pad-length byte at {@code buf[payloadOffset]}, validates it, and returns the data range + * that follows it. + * + * @return {@code Pairs.pack(dataOffset, dataLength)} — unpack with {@link Pairs#hi}/{@link Pairs#lo} + * @throws Http2Exception ({@code PROTOCOL_ERROR}) if {@code padded} is set but + * {@code payloadLength == 0} (no room for the pad-length byte itself), or if the + * claimed pad length is greater than or equal to the whole payload length (RFC 9113 + * §6.1: "If the length of the padding is the length of the frame payload or + * greater, the recipient MUST treat this as a connection error") + */ + public static long unpad(byte[] buf, int payloadOffset, int payloadLength, boolean padded) { + if (!padded) { + return Pairs.pack(payloadOffset, payloadLength); + } + if (payloadLength == 0) { + throw Http2Exception.of(Http2ErrorCode.PROTOCOL_ERROR, + "PADDED flag set but the frame has no payload for the pad-length byte"); + } + int padLength = buf[payloadOffset] & 0xFF; + if (padLength >= payloadLength) { + throw Http2Exception.of(Http2ErrorCode.PROTOCOL_ERROR, + "pad length " + padLength + " >= frame payload length " + payloadLength); + } + int dataOffset = payloadOffset + 1; + int dataLength = payloadLength - 1 - padLength; + return Pairs.pack(dataOffset, dataLength); + } + + /** Extracts the data offset from a value returned by {@link #unpad}. */ + public static int dataOffset(long unpadded) { + return Pairs.hi(unpadded); + } + + /** Extracts the data length from a value returned by {@link #unpad}. */ + public static int dataLength(long unpadded) { + return Pairs.lo(unpadded); + } +} diff --git a/flash/src/main/java/dev/relism/flash/transport/BufferedByteSource.java b/flash/src/main/java/dev/relism/flash/transport/BufferedByteSource.java index 1396ace..6acc13d 100644 --- a/flash/src/main/java/dev/relism/flash/transport/BufferedByteSource.java +++ b/flash/src/main/java/dev/relism/flash/transport/BufferedByteSource.java @@ -91,10 +91,17 @@ public final class BufferedByteSource extends InputStream { * Removes the deadline and restores the socket to blocking indefinitely * ({@code SO_TIMEOUT = 0}). Must be called before any read the caller wants to be * unbounded (e.g. handing the connection off to a long-lived WebSocket session loop). + * + *

{@code EX-37}: a {@code null} socket (the constructor accepts one — every isolated unit + * test in this codebase that constructs a {@code BufferedByteSource} directly over a + * {@code ByteArrayInputStream} passes {@code null}, since there is no real connection to + * bound) is treated as "no OS-level timeout to clear", not an error — only the deadline + * bookkeeping is reset. Production always supplies a real socket, so this changes no + * production behavior; without it, no test can exercise the deadline mechanism at all. */ public void clearDeadline() throws IOException { this.deadlineActive = false; - socket.setSoTimeout(0); + if (socket != null) socket.setSoTimeout(0); } // ── InputStream ────────────────────────────────────────────────────────── @@ -247,6 +254,14 @@ public final class BufferedByteSource extends InputStream { * active, computes the exact remaining budget and hands it to {@link Socket#setSoTimeout} * before reading, so a {@link SocketTimeoutException} from {@code in.read} unambiguously * means the deadline — not merely one read — has elapsed; see the class Javadoc. + * + *

{@code EX-37}: the expiry check above (throwing once {@code remainingNanos <= 0}) runs + * regardless of whether a real {@link Socket} is present; only the OS-level + * {@code setSoTimeout} call — meaningless without a socket, and previously called + * unconditionally, which NPE'd the instant any deadline-bounded read ran against a + * {@code null}-socket source — is skipped when {@code socket == null}. See + * {@link #clearDeadline()}'s Javadoc for why {@code null} is a legitimate, tested case, not + * a misuse. */ private int fillFromUnderlying(byte[] dst, int off, int len) throws IOException { if (!deadlineActive) { @@ -256,9 +271,11 @@ public final class BufferedByteSource extends InputStream { if (remainingNanos <= 0) { throw new SocketTimeoutException("Read deadline exceeded"); } - long remainingMillis = (remainingNanos + 999_999L) / 1_000_000L; // round up - int timeoutMs = (int) Math.max(1, Math.min(Integer.MAX_VALUE, remainingMillis)); - socket.setSoTimeout(timeoutMs); + if (socket != null) { + long remainingMillis = (remainingNanos + 999_999L) / 1_000_000L; // round up + int timeoutMs = (int) Math.max(1, Math.min(Integer.MAX_VALUE, remainingMillis)); + socket.setSoTimeout(timeoutMs); + } return in.read(dst, off, len); } } diff --git a/flash/src/test/java/dev/relism/flash/h2/frame/FrameValidatorTest.java b/flash/src/test/java/dev/relism/flash/h2/frame/FrameValidatorTest.java new file mode 100644 index 0000000..5c88fdc --- /dev/null +++ b/flash/src/test/java/dev/relism/flash/h2/frame/FrameValidatorTest.java @@ -0,0 +1,181 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.h2.Http2ErrorCode; +import dev.relism.flash.h2.Http2Exception; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +/** One test per RFC-mandated rejection, asserting the specific {@link Http2ErrorCode} — not merely that {@link Http2Exception} was thrown. */ +class FrameValidatorTest { + + private static byte[] rawFrame(int length, int typeCode, int flags, int streamId) { + byte[] buf = new byte[9]; + buf[0] = (byte) (length >>> 16); + buf[1] = (byte) (length >>> 8); + buf[2] = (byte) length; + buf[3] = (byte) typeCode; + buf[4] = (byte) flags; + buf[5] = (byte) (streamId >>> 24); + buf[6] = (byte) (streamId >>> 16); + buf[7] = (byte) (streamId >>> 8); + buf[8] = (byte) streamId; + return buf; + } + + private static FrameHeader headerOf(int length, FrameType type, int flags, int streamId) { + byte[] buf = rawFrame(length, type.code(), flags, streamId); + FrameHeader header = new FrameHeader(); + // reset() is package-private; same package as this test. + header.reset(buf, 0); + return header; + } + + private static Http2ErrorCode codeOf(FrameHeader header, boolean insideHeaderBlock) { + Http2Exception ex = assertThrows(Http2Exception.class, () -> FrameValidator.validate(header, insideHeaderBlock)); + return ex.errorCode(); + } + + // ── Length bounds, per type ────────────────────────────────────────────── + + @Test + void ping_wrongLength_isFrameSizeError() { + FrameHeader h = headerOf(7, FrameType.PING, 0, 0); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } + + @Test + void rstStream_wrongLength_isFrameSizeError() { + FrameHeader h = headerOf(3, FrameType.RST_STREAM, 0, 1); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } + + @Test + void windowUpdate_wrongLength_isFrameSizeError() { + FrameHeader h = headerOf(5, FrameType.WINDOW_UPDATE, 0, 1); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } + + @Test + void priority_wrongLength_isFrameSizeError() { + FrameHeader h = headerOf(4, FrameType.PRIORITY, 0, 1); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } + + @Test + void goaway_tooShort_isFrameSizeError() { + FrameHeader h = headerOf(7, FrameType.GOAWAY, 0, 0); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } + + @Test + void goaway_exactlyEightBytes_isValid() { + FrameHeader h = headerOf(8, FrameType.GOAWAY, 0, 0); + assertDoesNotThrow(() -> FrameValidator.validate(h, false)); + } + + @Test + void settings_notMultipleOfSix_isFrameSizeError() { + FrameHeader h = headerOf(7, FrameType.SETTINGS, 0, 0); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } + + @Test + void settings_multipleOfSix_isValid() { + FrameHeader h = headerOf(12, FrameType.SETTINGS, 0, 0); + assertDoesNotThrow(() -> FrameValidator.validate(h, false)); + } + + @Test + void settings_zeroLength_isValid() { + // An empty SETTINGS frame (0 entries) is legal -- e.g. the initial connection SETTINGS + // with no non-default values, or a SETTINGS ACK. + FrameHeader h = headerOf(0, FrameType.SETTINGS, FrameFlags.ACK, 0); + assertDoesNotThrow(() -> FrameValidator.validate(h, false)); + } + + // ── Stream id rules ────────────────────────────────────────────────────── + + @Test + void settings_nonZeroStreamId_isProtocolError() { + FrameHeader h = headerOf(0, FrameType.SETTINGS, 0, 1); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + @Test + void ping_nonZeroStreamId_isProtocolError() { + FrameHeader h = headerOf(8, FrameType.PING, 0, 3); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + @Test + void goaway_nonZeroStreamId_isProtocolError() { + FrameHeader h = headerOf(8, FrameType.GOAWAY, 0, 5); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + @Test + void data_zeroStreamId_isProtocolError() { + FrameHeader h = headerOf(0, FrameType.DATA, 0, 0); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + @Test + void headers_zeroStreamId_isProtocolError() { + FrameHeader h = headerOf(0, FrameType.HEADERS, 0, 0); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + @Test + void rstStream_zeroStreamId_isProtocolError() { + FrameHeader h = headerOf(4, FrameType.RST_STREAM, 0, 0); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + @Test + void windowUpdate_zeroStreamId_isValid_connectionWindow() { + FrameHeader h = headerOf(4, FrameType.WINDOW_UPDATE, 0, 0); + assertDoesNotThrow(() -> FrameValidator.validate(h, false)); + } + + @Test + void windowUpdate_nonZeroStreamId_isValid_streamWindow() { + FrameHeader h = headerOf(4, FrameType.WINDOW_UPDATE, 0, 9); + assertDoesNotThrow(() -> FrameValidator.validate(h, false)); + } + + // ── PUSH_PROMISE from a client ─────────────────────────────────────────── + + @Test + void pushPromise_fromClient_isAlwaysProtocolError() { + FrameHeader h = headerOf(4, FrameType.PUSH_PROMISE, 0, 1); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, false)); + } + + // ── Unknown frame types ────────────────────────────────────────────────── + + @Test + void unknownType_outsideHeaderBlock_isIgnoredNotRejected() { + byte[] buf = rawFrame(3, 0x20, 0, 1); // 0x20 is not a recognised type + FrameHeader h = new FrameHeader(); + h.reset(buf, 0); + assertNull(h.type()); + assertDoesNotThrow(() -> FrameValidator.validate(h, false)); + } + + @Test + void unknownType_insideHeaderBlock_isProtocolError() { + byte[] buf = rawFrame(3, 0x20, 0, 1); + FrameHeader h = new FrameHeader(); + h.reset(buf, 0); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, codeOf(h, true)); + } + + // ── Frame-size ceiling ──────────────────────────────────────────────────── + + @Test + void declaredLengthAboveMaxFrameSize_isFrameSizeError() { + FrameHeader h = headerOf(dev.relism.flash.h2.Http2Limits.MAX_FRAME_SIZE_LOCAL + 1, FrameType.DATA, 0, 1); + assertEquals(Http2ErrorCode.FRAME_SIZE_ERROR, codeOf(h, false)); + } +} diff --git a/flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderFuzzTest.java b/flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderFuzzTest.java new file mode 100644 index 0000000..7b36774 --- /dev/null +++ b/flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderFuzzTest.java @@ -0,0 +1,59 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.h2.Http2Exception; +import dev.relism.flash.transport.BufferedByteSource; +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayInputStream; +import java.io.EOFException; +import java.io.IOException; +import java.net.SocketTimeoutException; +import java.util.Random; + +import static org.junit.jupiter.api.Assertions.fail; + +/** + * Phase 5's DoD: "Fuzz test green for 10 million random inputs." Throws fully random bytes at + * {@link Http2FrameReader} and asserts that only a typed, expected outcome ever results: a + * {@link Http2Exception} (a declared length exceeding {@code MAX_FRAME_SIZE_LOCAL} — the + * overwhelmingly common outcome, since a random 24-bit length is astronomically likely to + * exceed 16384), an {@link EOFException} (the random input ran out before a full frame arrived + * — the second most common outcome, since fuzz inputs are deliberately small), or a + * {@link SocketTimeoutException} (never actually expected here — no deadline is short enough to + * trip against an in-memory stream — but a legal outcome of the API's own contract). Anything + * else escaping — {@code ArrayIndexOutOfBoundsException}, {@code NegativeArraySizeException}, + * {@code OutOfMemoryError}, or simply never returning — fails the test. + */ +class Http2FrameReaderFuzzTest { + + private static final int TRIALS = 10_000_000; + private static final int MAX_INPUT_LEN = 64; + + @Test + void fuzz_10MillionRandomInputs_onlyTypedOutcomesEscape() { + Random rnd = new Random(0x4855_3244_5F46_5A32L); + byte[] data = new byte[MAX_INPUT_LEN]; + + for (int trial = 0; trial < TRIALS; trial++) { + int len = rnd.nextInt(MAX_INPUT_LEN + 1); + for (int i = 0; i < len; i++) data[i] = (byte) rnd.nextInt(256); + + BufferedByteSource src = new BufferedByteSource( + new ByteArrayInputStream(data, 0, len), null, 128); + Http2FrameReader reader = new Http2FrameReader(src, 128); + + try { + FrameHeader header = reader.readFrame(); + if (header != null) { + reader.consumeFrame(); + } + } catch (Http2Exception | EOFException | SocketTimeoutException expected) { + // any of these three is a correctly-typed rejection of malformed/truncated input + } catch (IOException e) { + fail("unexpected IOException at trial " + trial + " (len=" + len + "): " + e, e); + } catch (RuntimeException e) { + fail("unexpected RuntimeException at trial " + trial + " (len=" + len + "): " + e, e); + } + } + } +} diff --git a/flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderTest.java b/flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderTest.java new file mode 100644 index 0000000..c15855e --- /dev/null +++ b/flash/src/test/java/dev/relism/flash/h2/frame/Http2FrameReaderTest.java @@ -0,0 +1,209 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.bytes.ByteWriter; +import dev.relism.flash.h2.Http2Exception; +import dev.relism.flash.transport.BufferedByteSource; +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayInputStream; +import java.io.EOFException; +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.jupiter.api.Assertions.*; + +class Http2FrameReaderTest { + + private static BufferedByteSource sourceOf(byte[] bytes) { + return new BufferedByteSource(new ByteArrayInputStream(bytes), null); + } + + private static byte[] buildFrame(FrameType type, int flags, int streamId, byte[] payload) { + FrameWriteBuffer out = new FrameWriteBuffer(new ByteWriter(32)); + out.beginFrame(type, flags, streamId); + out.writer().writeBytes(payload); + out.endFrame(); + byte[] result = new byte[out.writer().length()]; + System.arraycopy(out.writer().array(), 0, result, 0, result.length); + return result; + } + + // ── Round trip every frame type ───────────────────────────────────────── + + @Test + void roundTrip_everyFrameType() throws IOException { + for (FrameType type : FrameType.values()) { + int payloadLen = switch (type) { + case PING -> 8; + case RST_STREAM, WINDOW_UPDATE -> 4; + case PRIORITY -> 5; + case GOAWAY -> 8; + default -> 10; + }; + byte[] payload = new byte[payloadLen]; + for (int i = 0; i < payloadLen; i++) payload[i] = (byte) (i + 1); + int streamId = type.streamIdRule() == FrameType.StreamIdRule.FORBIDDEN ? 0 : 7; + + byte[] wire = buildFrame(type, 0x1, streamId, payload); + Http2FrameReader reader = new Http2FrameReader(sourceOf(wire)); + FrameHeader header = reader.readFrame(); + + assertNotNull(header, "type=" + type); + assertEquals(type, header.type()); + assertEquals(type.code(), header.typeCode()); + assertEquals(payloadLen, header.length()); + assertEquals(streamId, header.streamId()); + assertEquals(0x1, header.flags()); + for (int i = 0; i < payloadLen; i++) { + assertEquals(payload[i], header.buffer()[header.payloadOffset() + i], "byte " + i + " of type " + type); + } + reader.consumeFrame(); + } + } + + // ── Boundary lengths ───────────────────────────────────────────────────── + + @Test + void boundaryLengths_0_1_16383_16384_16385() throws IOException { + int[] lengths = {0, 1, 16383, 16384, 16385}; + for (int len : lengths) { + byte[] payload = new byte[len]; + byte[] wire = buildFrame(FrameType.DATA, 0, 1, payload); + Http2FrameReader reader = new Http2FrameReader(sourceOf(wire)); + if (len > dev.relism.flash.h2.Http2Limits.MAX_FRAME_SIZE_LOCAL) { + Http2Exception ex = assertThrows(Http2Exception.class, reader::readFrame); + assertEquals(dev.relism.flash.h2.Http2ErrorCode.FRAME_SIZE_ERROR, ex.errorCode()); + } else { + FrameHeader header = reader.readFrame(); + assertNotNull(header); + assertEquals(len, header.length()); + } + } + } + + // ── A frame split across multiple socket reads ───────────────────────── + + private static final class DribblingInputStream extends InputStream { + private final byte[] data; + private int pos; + private final int chunkSize; + + DribblingInputStream(byte[] data, int chunkSize) { + this.data = data; + this.chunkSize = chunkSize; + } + + @Override + public int read() { + return pos < data.length ? (data[pos++] & 0xFF) : -1; + } + + @Override + public int read(byte[] dst, int off, int len) { + if (pos >= data.length) return -1; + int n = Math.min(chunkSize, Math.min(len, data.length - pos)); + System.arraycopy(data, pos, dst, off, n); + pos += n; + return n; + } + } + + @Test + void frameSplitAcrossThreeSocketReads() throws IOException { + byte[] payload = new byte[300]; + for (int i = 0; i < payload.length; i++) payload[i] = (byte) i; + byte[] wire = buildFrame(FrameType.DATA, 0, 3, payload); + + // 9(header) + 300(payload) = 309 bytes, dribbled in chunks of 103 -> 3 reads. + int chunk = (wire.length + 2) / 3; + BufferedByteSource src = new BufferedByteSource(new DribblingInputStream(wire, chunk), null); + Http2FrameReader reader = new Http2FrameReader(src); + FrameHeader header = reader.readFrame(); + + assertNotNull(header); + assertEquals(300, header.length()); + for (int i = 0; i < 300; i++) { + assertEquals(payload[i], header.buffer()[header.payloadOffset() + i]); + } + } + + // ── A frame exactly filling the initial buffer ────────────────────────── + + @Test + void frameExactlyFillingInitialBuffer() throws IOException { + int bufSize = 64; + byte[] payload = new byte[bufSize - 9]; // header + payload == bufSize exactly + byte[] wire = buildFrame(FrameType.DATA, 0, 1, payload); + assertEquals(bufSize, wire.length); + + Http2FrameReader reader = new Http2FrameReader(sourceOf(wire), bufSize); + FrameHeader header = reader.readFrame(); + assertNotNull(header); + assertEquals(payload.length, header.length()); + } + + // ── Multiple frames on one connection, sequential reads ───────────────── + + @Test + void multipleFramesSequentially() throws IOException { + ByteWriter w = new ByteWriter(64); + FrameWriteBuffer out = new FrameWriteBuffer(w); + out.beginFrame(FrameType.PING, 0, 0); + out.writer().writeBytes(new byte[]{1, 2, 3, 4, 5, 6, 7, 8}); + out.endFrame(); + out.beginFrame(FrameType.PING, dev.relism.flash.h2.frame.FrameFlags.ACK, 0); + out.writer().writeBytes(new byte[]{8, 7, 6, 5, 4, 3, 2, 1}); + out.endFrame(); + byte[] wire = new byte[w.length()]; + System.arraycopy(w.array(), 0, wire, 0, wire.length); + + Http2FrameReader reader = new Http2FrameReader(sourceOf(wire)); + FrameHeader first = reader.readFrame(); + assertEquals(1, first.buffer()[first.payloadOffset()]); + assertEquals(0, first.flags()); + reader.consumeFrame(); + + FrameHeader second = reader.readFrame(); + assertEquals(8, second.buffer()[second.payloadOffset()]); + assertEquals(FrameFlags.ACK, second.flags()); + reader.consumeFrame(); + + assertNull(reader.readFrame()); // clean EOF after both frames consumed + } + + // ── EOF handling ───────────────────────────────────────────────────────── + + @Test + void cleanEofBetweenFrames_returnsNull() throws IOException { + Http2FrameReader reader = new Http2FrameReader(sourceOf(new byte[0])); + assertNull(reader.readFrame()); + } + + @Test + void eofMidFrame_throwsEOFException() { + byte[] wire = buildFrame(FrameType.DATA, 0, 1, new byte[100]); + byte[] truncated = new byte[50]; // header + partial payload + System.arraycopy(wire, 0, truncated, 0, 50); + + Http2FrameReader reader = new Http2FrameReader(sourceOf(truncated)); + assertThrows(EOFException.class, reader::readFrame); + } + + @Test + void eofMidHeader_throwsEOFException() { + byte[] truncated = new byte[5]; // fewer than the 9 header bytes + Http2FrameReader reader = new Http2FrameReader(sourceOf(truncated)); + assertThrows(EOFException.class, reader::readFrame); + } + + // ── Reserved bit masking ───────────────────────────────────────────────── + + @Test + void reservedBitInStreamId_isMaskedNotRejected() throws IOException { + byte[] wire = buildFrame(FrameType.DATA, 0, 5, new byte[]{1, 2, 3}); + wire[5] |= (byte) 0x80; // set the reserved high bit of the stream-id field + Http2FrameReader reader = new Http2FrameReader(sourceOf(wire)); + FrameHeader header = reader.readFrame(); + assertEquals(5, header.streamId(), "reserved bit must be masked, not folded into the stream id"); + } +} diff --git a/flash/src/test/java/dev/relism/flash/h2/frame/PaddingTest.java b/flash/src/test/java/dev/relism/flash/h2/frame/PaddingTest.java new file mode 100644 index 0000000..0b63852 --- /dev/null +++ b/flash/src/test/java/dev/relism/flash/h2/frame/PaddingTest.java @@ -0,0 +1,88 @@ +package dev.relism.flash.h2.frame; + +import dev.relism.flash.h2.Http2ErrorCode; +import dev.relism.flash.h2.Http2Exception; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +class PaddingTest { + + @Test + void notPadded_returnsWholePayloadUnchanged() { + byte[] buf = {1, 2, 3, 4, 5}; + long r = Padding.unpad(buf, 1, 4, false); + assertEquals(1, Padding.dataOffset(r)); + assertEquals(4, Padding.dataLength(r)); + } + + @Test + void padded_zeroPadLength_allBytesAreData() { + // [padLength=0][data...] + byte[] buf = {0, 10, 20, 30}; + long r = Padding.unpad(buf, 0, 4, true); + assertEquals(1, Padding.dataOffset(r)); + assertEquals(3, Padding.dataLength(r)); + assertEquals(10, buf[Padding.dataOffset(r)]); + } + + @Test + void padded_someData_somePadding() { + // [padLength=2][data: 3 bytes][padding: 2 bytes] -> payload length 6 + byte[] buf = {2, 7, 8, 9, 0, 0}; + long r = Padding.unpad(buf, 0, 6, true); + assertEquals(1, Padding.dataOffset(r)); + assertEquals(3, Padding.dataLength(r)); + assertEquals(7, buf[Padding.dataOffset(r)]); + assertEquals(9, buf[Padding.dataOffset(r) + 2]); + } + + @Test + void padded_allPaddingNoData() { + // [padLength=3][padding x3] -> payload length 4, dataLength 0 + byte[] buf = {3, 0, 0, 0}; + long r = Padding.unpad(buf, 0, 4, true); + assertEquals(0, Padding.dataLength(r)); + } + + @Test + void padded_atNonZeroOffset_withinLargerBuffer() { + byte[] buf = {(byte) 0xFF, (byte) 0xFF, 1, 5, 6, 0, (byte) 0xFF}; + // payload starts at index 2, length 4: [padLength=1][data:5,6][padding:1] + long r = Padding.unpad(buf, 2, 4, true); + assertEquals(3, Padding.dataOffset(r)); + assertEquals(2, Padding.dataLength(r)); + assertEquals(5, buf[Padding.dataOffset(r)]); + assertEquals(6, buf[Padding.dataOffset(r) + 1]); + } + + @Test + void padded_zeroPayloadLength_isProtocolError() { + byte[] buf = {}; + Http2Exception ex = assertThrows(Http2Exception.class, () -> Padding.unpad(buf, 0, 0, true)); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, ex.errorCode()); + } + + @Test + void padded_padLengthEqualsPayloadLength_isProtocolError() { + // payloadLength=3, claimed padLength=3 -- leaves -1 bytes for data, invalid. + byte[] buf = {3, 0, 0}; + Http2Exception ex = assertThrows(Http2Exception.class, () -> Padding.unpad(buf, 0, 3, true)); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, ex.errorCode()); + } + + @Test + void padded_padLengthGreaterThanPayloadLength_isProtocolError() { + byte[] buf = {(byte) 255, 0, 0}; + Http2Exception ex = assertThrows(Http2Exception.class, () -> Padding.unpad(buf, 0, 3, true)); + assertEquals(Http2ErrorCode.PROTOCOL_ERROR, ex.errorCode()); + } + + @Test + void padded_maxValidPadLength_leavesZeroData() { + // payloadLength=5: [padLength=4][padding x4] -- valid, dataLength 0. + byte[] buf = {4, 0, 0, 0, 0}; + long r = Padding.unpad(buf, 0, 5, true); + assertEquals(0, Padding.dataLength(r)); + } +} diff --git a/flash/src/test/java/dev/relism/flash/transport/BufferedByteSourceTest.java b/flash/src/test/java/dev/relism/flash/transport/BufferedByteSourceTest.java new file mode 100644 index 0000000..33dbb57 --- /dev/null +++ b/flash/src/test/java/dev/relism/flash/transport/BufferedByteSourceTest.java @@ -0,0 +1,164 @@ +package dev.relism.flash.transport; + +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * {@code EX-37}: this class previously had zero dedicated tests — its deadline mechanism (the + * actual {@code EX-07} slowloris fix) was exercised only indirectly through real-socket, + * end-to-end tests, which never hit the {@code null}-socket path every isolated unit test in + * this codebase actually uses. Found and fixed while building {@code Http2FrameReaderTest} + * (Phase 5); this class closes the gap. + */ +class BufferedByteSourceTest { + + private static BufferedByteSource sourceOf(String s) { + return new BufferedByteSource(new ByteArrayInputStream(s.getBytes(StandardCharsets.US_ASCII)), null); + } + + // ── Plain InputStream passthrough ─────────────────────────────────────── + + @Test + void read_singleByte() throws IOException { + BufferedByteSource src = sourceOf("AB"); + assertEquals('A', src.read()); + assertEquals('B', src.read()); + assertEquals(-1, src.read()); + } + + @Test + void read_intoArray() throws IOException { + BufferedByteSource src = sourceOf("hello world"); + byte[] buf = new byte[5]; + int n = src.read(buf, 0, 5); + assertEquals(5, n); + assertEquals("hello", new String(buf, StandardCharsets.US_ASCII)); + } + + @Test + void read_largerThanInternalBuffer_bypassesBufferCorrectly() throws IOException { + String big = "x".repeat(20_000); + BufferedByteSource src = new BufferedByteSource( + new ByteArrayInputStream(big.getBytes(StandardCharsets.US_ASCII)), null, 4096); + byte[] out = new byte[20_000]; + int total = 0; + while (total < out.length) { + int n = src.read(out, total, out.length - total); + if (n < 0) break; + total += n; + } + assertEquals(20_000, total); + } + + // ── peek / prependOnce ─────────────────────────────────────────────────── + + @Test + void peek_doesNotConsume() throws IOException { + BufferedByteSource src = sourceOf("abcdef"); + byte[] dst = new byte[3]; + int n = src.peek(dst, 0, 3); + assertEquals(3, n); + assertEquals("abc", new String(dst, StandardCharsets.US_ASCII)); + // Still readable from the start — peek must not have advanced the position. + assertEquals('a', src.read()); + assertEquals('b', src.read()); + } + + @Test + void peek_rejectsLengthAboveBufferCapacity() { + BufferedByteSource src = new BufferedByteSource(new ByteArrayInputStream(new byte[0]), null, 16); + assertThrows(IllegalArgumentException.class, () -> src.peek(new byte[20], 0, 20)); + } + + @Test + void prependOnce_servedBeforeUnderlyingBytes() throws IOException { + BufferedByteSource src = sourceOf("world"); + byte[] prefix = "hello ".getBytes(StandardCharsets.US_ASCII); + src.prependOnce(prefix, 0, prefix.length); + + byte[] out = new byte[11]; + int total = 0; + while (total < out.length) { + int n = src.read(out, total, out.length - total); + if (n < 0) break; + total += n; + } + assertEquals("hello world", new String(out, 0, total, StandardCharsets.US_ASCII)); + } + + @Test + void prependOnce_rejectsSecondCallBeforeFirstIsConsumed() { + BufferedByteSource src = sourceOf("x"); + byte[] a = "a".getBytes(StandardCharsets.US_ASCII); + src.prependOnce(a, 0, 1); + assertThrows(IllegalStateException.class, () -> src.prependOnce(a, 0, 1)); + } + + // ── Deadline mechanism, EX-37's actual regression coverage ────────────── + + @Test + void clearDeadline_withNullSocket_doesNotThrow() throws IOException { + BufferedByteSource src = sourceOf("data"); + src.setDeadline(System.nanoTime() + 1_000_000_000L); + assertDoesNotThrow(src::clearDeadline); + } + + @Test + void deadlineAlreadyExpired_throwsSocketTimeoutException_evenWithNullSocket() { + BufferedByteSource src = sourceOf(""); // empty: forces fillFromUnderlying on the next read + src.setDeadline(System.nanoTime() - 1_000_000_000L); // already in the past + assertThrows(SocketTimeoutException.class, () -> src.read(new byte[1], 0, 1)); + } + + @Test + void deadlineNotYetExpired_readsNormally_withNullSocket() throws IOException { + BufferedByteSource src = sourceOf("z"); + src.setDeadline(System.nanoTime() + 30_000_000_000L); // 30s in the future + assertEquals('z', src.read()); + } + + @Test + void bytesAlreadyBuffered_areServedRegardlessOfDeadline() throws IOException { + // peek() fills the internal buffer without a deadline; a since-expired deadline must not + // block already-buffered bytes from being read (only underlying-stream reads are bounded). + BufferedByteSource src = sourceOf("buffered"); + src.peek(new byte[8], 0, 8); + src.setDeadline(System.nanoTime() - 1); // already expired + assertEquals('b', src.read()); // served from the buffer — no underlying read needed + } + + @Test + void clearDeadline_thenRead_neverThrowsTimeoutAfterward() throws IOException { + BufferedByteSource src = sourceOf("ok"); + src.setDeadline(System.nanoTime() - 1); // expired + src.clearDeadline(); + assertEquals('o', src.read()); // deadline cleared — must not time out + } + + // ── available / skip / close ───────────────────────────────────────────── + + @Test + void skip_advancesPastBufferedAndUnderlyingBytes() throws IOException { + BufferedByteSource src = sourceOf("abcdef"); + long skipped = src.skip(3); + assertEquals(3, skipped); + assertEquals('d', src.read()); + } + + @Test + void close_delegatesToUnderlyingStream() { + java.io.InputStream[] closed = new java.io.InputStream[1]; + java.io.InputStream in = new ByteArrayInputStream(new byte[0]) { + @Override public void close() throws IOException { closed[0] = this; super.close(); } + }; + BufferedByteSource src = new BufferedByteSource(in, null); + assertDoesNotThrow(src::close); + assertSame(in, closed[0]); + } +}