Fix heap allocation leak in Socket.getValue - #1
Closed
ianegordon wants to merge 31 commits into
Closed
Conversation
Per RFC 6455 §5.2, extended payload lengths must be interpreted as unsigned integers in network byte order (Big-Endian). Previously, the 64-bit decoding path (used when length0 is 127) was incorrectly implemented as Little-Endian, causing decoding failures for frames larger than 65,535 bytes. This change: - Corrects the 8-byte decoding path to use Big-Endian shifts. - Normalizes WSFrameEncoderTests to use Big-Endian expectations. - Adds edge case coverage for 0, 126, and 65,536 byte boundaries. - Adds tests for truncated data in both 16-bit and 64-bit paths. - Ensures mask bit presence is correctly handled during decoding.
HTTPRequest.shouldKeepAlive now treats HTTP/1.1 as persistent unless `Connection: close` is sent, and HTTP/1.0 as closed unless `keep-alive` is sent. Splits the Connection header on commas so multi-token values like `Keep-Alive, Upgrade` are recognized (RFC 9110 §7.6.1). HTTPServer.handleRequest no longer overwrites a Connection header that the handler explicitly set (e.g. WebSocket's `Upgrade`); it only echoes the request's Connection token when the response has none. Adds nine HTTPRequest.shouldKeepAlive cases covering HTTP/1.1 default, multi-token, and HTTP/1.0 semantics. Updates the keep-alive iteration test to terminate via `Connection: close` instead of relying on the old (buggy) behavior. Closes TVT-288 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
HTTPDecoder.readBody now honors Transfer-Encoding: chunked (RFC 9112 §7.1) by routing the body through a new HTTPChunkedTransferDecoder, the read-side mirror of HTTPChunkedTransferEncoder. Trailer fields are consumed and discarded. readBody also throws HTTPDecoder.Error on framing violations: - both Content-Length and Transfer-Encoding present (§6.1) - non-numeric or negative Content-Length (§6.3 swhitty#5) - Transfer-Encoding whose final coding is not `chunked` (§6.1) Throwing surfaces as a connection-close (RFC-permitted for unrecoverable framing errors); HTTPServer.handleConnection has a TODO to upgrade this to an explicit 400 Bad Request response in a future change. readBody's signature changes from (from:length:) to (from:contentLength:transferEncoding:). Both decodeRequest and decodeResponse pass headers[.contentLength] and headers[.transferEncoding]. Closes TVT-287 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Six additional HTTPDecoderTests targeting the uncovered branches in HTTPChunkedTransferDecoder and HTTPDecoder.readBody: - chunkExt_IsIgnored (RFC 9112 §7.1: chunk-ext after `;` is parsed but ignored) - invalidChunkSize_ThrowsError (non-hex chunk-size) - missingCRLFAfterChunkData_ThrowsError (chunk-data not followed by CRLF) - truncatedChunkSize_ThrowsError / truncatedChunkData_ThrowsError (stream ends mid-line / mid-chunk -> SocketError.disconnected) - unsupportedTransferEncoding_ThrowsError (e.g. `gzip`) Lifts HTTPChunkedDecodedSequence.swift to 86.79%/88.73% region/line coverage and HTTPDecoder.swift's TVT-287 additions to 100%. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The `MessageFrameWSHandler.start` implementation previously lacked explicit termination for the `messagesIn` stream, which could lead to message handlers hanging indefinitely if they relied on the stream ending naturally. Additionally, concurrent tasks could prematurely finish the `framesOut` stream with a `CancellationError` when one task completed before the other. This change: - Uses `defer` blocks to guarantee that both `messagesIn` and `framesOut` continuations are finished when the handler exits. - Filters out `CancellationError` during task cleanup to prevent reporting spurious errors to the client during a clean shutdown. - Centralizes stream termination logic, removing redundant `.finish()` calls from individual tasks. - Adds an exhaustive catch block to ensure the task group closures remain non-throwing as required by the Swift concurrency model.
…p-alive-default Honor HTTP/1.1 persistent-connection default per RFC 9112 §9.3
…quest-bodies Decode chunked request bodies; reject invalid framing per RFC 9112
Existing chunked-body coverage runs through HTTPDecoder. These tests exercise the decoder directly to pin behavior that integration tests don't cover: that consumption stops at the trailer terminator (so keep-alive pipelining is safe), that nextBuffer(suggested:) honors the suggested cap, that uppercase HEXDIG is accepted per RFC 5234 §2.3, and that chunk-sizes exceeding Int.max are rejected as a framing error.
nginx (ngx_http_set_etag) and Apache HTTPD's default FileETag MTime Size both derive a static-file ETag from the same metadata. Doing the same here lets FileHTTPHandler and DirectoryHTTPHandler skip the per-request Data(contentsOf:) + SHA-256 load. Closes TVT-290 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add direct unit tests for HTTPChunkedTransferDecoder
…ndler-loads-whole-file-into-memory-to-compute-etag Compute ETag from (mtime, size) to skip whole-file load per request
Matches FileHTTPHandler so a multi-gigabyte asset under a directory handler no longer loads fully into memory per request. Existence-check moves into the HTTPBodySequence(file:) throw, mirroring FileHTTPHandler's do/catch → 404 pattern. Closes TVT-286 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Control frames (ping/pong/close) MUST have payload length ≤ 125 bytes. Without this guard, a peer could send a 1 MB ping and have it echoed back verbatim as a pong by WSHandler.makeResponseFrames. Closes TVT-306 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds tests for Cache-Control/Date/Last-Modified/ETag header emission on 200 responses, plus 304 round-trips and 200 fallthrough for both If-Modified-Since and If-None-Match. Exercises lines in DirectoryHTTPHandler.handleRequest that previously had no coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ttphandler-loads-entire-file-into-memory-no
…idator-reject-control-frames-with-payload-gt125
HTTPDecoder pulls one byte per syscall while parsing the status line and headers: `bytes.lines.takeNext()` and `readHeaders(from:)` both end up in CollectUntil.next() calling iterator.next(), which on AsyncSocketReadSequence does an unbuffered `socket.read()` per byte. For a typical request with 200–500 bytes of status line + headers that's 200–500 single-byte read(2) syscalls and a corresponding suspendSocket cycle whenever a TCP segment boundary lands mid-header. Adding an internal buffer to AsyncSocketReadSequence.next() would lose bytes between iterators, because HTTPDecoder constructs a fresh iterator for the body reader and HTTPRequestSequence creates a fresh iterator per request on a keepalive connection. Any bytes buffered-but-unconsumed when one iterator is dropped would be unreachable to the next. Add AsyncBufferingSequence<Base>: a reference-typed wrapper backed by an actor that owns one iterator into Base and a shared in-memory buffer. Iterators created from the same wrapper consume from the shared backing buffer, so bytes pulled from Base are never lost between successive iterators. Uses the same Transferring idiom that AsyncSharedReplaySequence already uses to call mutating async functions on a value-type iterator across actor isolation. Wrap socket.bytes once per HTTPConnection and thread the wrapper through both HTTPRequestSequence and the WebSocket upgrade path, so any bytes pulled past the upgrade request remain available to the framer. Measurements: release build of an MRP REST daemon under identical workload, 16 s perf captures: total cycles 45.8e9 -> 42.2e9 (-7.9%); average CPU rate 3056 Mc/s -> 2649 Mc/s (-13%). HTTPDecoder.decodeRequest self time drops from indistinguishable in the noise to 0.0-0.02%; the parser essentially disappears from the profile. All 426 existing tests pass.
Buffer HTTP request bytes once per connection
read()/recvfrom()/recvmsg() returning 0 is an orderly EOF, not an error, and does not set errno. The error classification checked errno == EWOULDBLOCK before count == 0, so an EOF read whose errno was left as EWOULDBLOCK by an earlier would-block read on the same thread was misclassified as .blocked instead of .disconnected. The reader then re-suspended waiting for more data instead of closing. Under concurrency (errno is per-thread) this both leaked the connection (CLOSE-WAIT, never closed) and, because epoll re-reports the readable EOF socket, spun the pool re-arming and re-reading it, pegging CPU. Check count == 0 first; errno is only meaningful after a -1 return.
epoll_wait returns -1/EINTR when a signal interrupts it, which is not a
fatal condition. getNotifications() treated any non-positive return as a
failure and threw SocketError.makeFailed("epoll wait"), tearing down the
server ("epoll wait(4): Interrupted system call"). Return no events on
EINTR so the caller polls again.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017Kkgk9vUQAZUTMUnbus1RR
…assification Socket: treat read()/recv()==0 as EOF before consulting errno
make HTTPClient public ~Copyable
UnsafeMutablePointer.allocate was never deallocated on either the success or throw path, leaking one heap block per socket-option read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ianegordon
force-pushed
the
ian/tvt-1062-socketgetvalue-leaks-a-heap-allocation-on-every-call
branch
from
July 21, 2026 01:14
0f0e8d8 to
48e68fa
Compare
Owner
Author
|
Superseded by upstream PR swhitty#226 — the fix now targets the parent repository directly. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Socket.getValueallocated anUnsafeMutablePointer<O.SocketValue>that was never deallocated on either the success or throw path, leaking one heap block on every socket-option read (called bysocketType, buffer-size accessors, etc.).defer { valuePtr.deallocate() }immediately after allocation. This satisfies the documenteddeallocate()precondition — the pointer is the start of the allocated block and the memory is never initialized through Swift's typed model.Resolves TVT-1062.
Testing
SocketTestsforsocketType,receiveBufferSize,sendBufferSize); the leak itself is not unit-assertable, so no new tests were added.🤖 Generated with Claude Code