PR #185 + CI/deadlock fixes: TLS floor, parser limits, and four hang root-causes#186
Merged
Conversation
`MailTLSConfiguration.makeClientConfiguration` builds on `TLSConfiguration.makeClientConfiguration()` and only overrides `certificateVerification` and `trustRoots`. It therefore inherits NIOSSL's default `minimumTLSVersion` of `.tlsv1` — TLS 1.0, which RFC 8996 deprecated in March 2021. There was no way for a caller to raise that floor. This adds `MailTLSMinimumVersion` and threads it through `IMAPServer`, `SMTPServer`, `IMAPConnection` and both SMTP TLS paths (implicit TLS and STARTTLS). The enum is deliberately SwiftMail's own type rather than NIOSSL's `TLSVersion`: SwiftMail imports NIOSSL without re-exporting it, so exposing `TLSVersion` in the public API would push NIOSSL onto every consumer. Behaviour change: the default floor moves from TLS 1.0 to TLS 1.2, the lowest version RFC 8996 still permits. Callers needing the old behaviour can pass `.tlsv1` explicitly. Happy to default to `.tlsv1` instead if you would rather keep this strictly additive. Callers who know their servers speak TLS 1.3 can now pass `.tlsv1_3` and make downgrades impossible regardless of what the server offers. Tests cover the default, each enum mapping, and that both server types carry the caller's choice. Full suite passes (325 tests).
`IMAPConnection.makeBootstrap` hardcodes the response parser to `messageAttributeLimit: .max` and `bodySizeLimit: .max`. A server controls both the length prefixes and the attribute count it sends, so a hostile or malfunctioning server can force unbounded allocation from a single response, and a caller has no way to bound it. `responseBufferLimit` does not cover this: it bounds the parser's working buffer, whereas these bound what a single response may declare. This is the class of issue that produced advisories in other IMAP clients — Ruby net-imap GHSA-j3g3-5qv5-52mj (unbounded literal size → memory exhaustion, fixed by adding a configurable `max_response_size`) and mutt CVE-2021-3657 ("reject excessively large IMAP literals"). Adds `IMAPParserLimits`, threaded through `IMAPServer` and `IMAPConnection`. Strictly additive: the defaults are `.max`/`.max`, exactly today's behaviour, so existing callers see no change. Callers who want bounds can now set them: IMAPServer( host: "imap.example.com", port: 993, parserLimits: IMAPParserLimits( bodySizeLimit: 64 * 1024 * 1024, messageAttributeLimit: 1024 ) ) `literalSizeLimit` is included for completeness — it was pinned to `IMAPDefaults.literalSizeLimit` with no way to adjust it. Bounded defaults would arguably be the better long-term choice, but that would silently break anyone fetching large attachments, so I left it to you whether to make that call separately. Tests cover defaults, partial overrides, and that IMAPServer carries the caller's choice. Full suite passes (331 tests).
Addresses the review on #185. All five findings were reproduced before fixing; the two MEDIUM ones were confirmed against the pinned parser rather than taken on faith. HIGH — implicit TLS ignored minimumTLSVersion. makeConnectionBootstrap omitted the argument, so makeTLSHandler's `.tlsv1_2` default applied and IMAPServer(port: 993, minimumTLSVersion: .tlsv1_3) negotiated TLS 1.2 with a downgraded peer. Port 993 is the port every IMAPS client uses, so the floor was effectively inert for the common case. HIGH — makeIdleConnection/makeNamedConnection omitted both minimumTLSVersion and parserLimits, so a server configured with a TLS 1.3 floor and 64 MiB limits spawned IDLE and named connections with a TLS 1.2 floor and unbounded limits — on precisely the long-lived connections that sit and listen to whatever the server sends. MEDIUM ×2 — the limits are documented as maxima, but the parser enforces `size < bodySizeLimit` and `attributeCount < messageAttributeLimit`, the latter again on the FETCH closing delimiter. A 64 MiB limit rejected a 64 MiB body; messageAttributeLimit: 1 rejected a one-attribute FETCH. Rather than redefine "maximum" as "one less than this", the translation now adds one, saturating at .max where the value already means unbounded. It lives in IMAPParserLimits.makeParserOptions(bufferLimit:) so there is one place to get it right instead of one per call site — the previous arrangement, where each site spelled it out, was how only the primary connection ended up with limits at all. The root cause of all three HIGH findings is the same: security policy with default values. A default makes "forgot to pass the floor" indistinguishable from "chose TLS 1.2", and neither the compiler nor a reviewer can tell them apart. IMAPConnection's designated initializer and makeTLSHandler no longer default minimumTLSVersion or parserLimits, so omitting one is a build error. That immediately surfaced a fourth call site — the legacy useTLS convenience initializer — which neither grep nor the review had found. Tests. The reason this shipped is worth recording: MailTLSConfigurationTests asserts that makeClientConfiguration returns what it is given, and one of its cases was named "Raising the floor to TLS 1.3 makes a downgrade impossible". It was green the whole time. It could not have caught this, because the defect was not a wrong return value — it was that nobody passed the value in. A test that cannot produce the failure proves nothing about it. So the new tests assert at the level where the defect lived: - IMAPTLSFloorEnforcementTests spins up a real TLS endpoint pinned to maximumTLSVersion .tlsv1_2 and connects a real IMAPConnection to it. A TLS 1.3 floor must fail; a TLS 1.2 floor must succeed (the control — without it the first test would also pass against a closed port). Verified by reverting the fix: it fails with "an error was expected but none was thrown", and passes with it. - IMAPSecurityPolicyPropagationTests asserts on the spawned connections themselves. - IMAPParserLimitsTranslationTests covers the off-by-one and the saturation at .max. The misleading test name is corrected, and MailTLSConfigurationTests now says what it does and does not cover. 339 tests pass. Built and tested with Xcode's toolchain.
The SwiftLint job has been failing on this branch since the first commit; I only noticed when CI mailed about it. Four of the seven violations came from the original PR commits, three from the review fixes. Neither round ran the linter locally, which the repo's CI runs with --strict. MailTLSMinimumVersion's cases are renamed tlsv1_1/tlsv1_2/tlsv1_3 -> tlsv11/tlsv12/tlsv13. identifier_name rejects the underscores, and rather than add an exception for a brand-new API, the cases now match NIOSSL's own spelling (TLSVersion.tlsv11/tlsv12/tlsv13) — which is what they map onto anyway. The API is unreleased, so the rename is free. Also: trailing comma in the parameterized test, and the TLS test server's nested types moved to file scope (nesting depth). Verified locally with swiftlint --strict: 0 violations in 262 files. 339 tests still pass.
Addresses the four findings from the second and third reviews. They share one root: the limits advertised a bound the surrounding code did not keep. Each is reproduced by a test that fails without its fix. bodySizeLimit across the whole response (HIGH). ResponseParser checks each streaming section on its own and keeps no total, while FetchPartHandler and PipelinedFetchPartHandler append every section into one Data — so any number of 50 MiB sections passed a 64 MiB limit. The total could be counted in those two handlers, but then it would hold only for the two that exist today. The limit is a transport promise, so it is enforced on the transport: IMAPResponseLimitGuard sits directly behind the decoder, sums the byteCount each section declares, resets per message, and covers every consumer including future ones. Fail closed (HIGH ×2). The guard closes the channel on any limit violation — the peer has already proven itself, and the one thing that must not happen is to keep reading from it. That alone was not enough for IDLE: IdleHandler recorded channelInactive and errorCaught but never finished the AsyncStream, so "fail closed" would have meant socket shut, caller still hanging. It now finishes on both. The pipelined path swallowed every failure and returned an empty result, which is indistinguishable from "this message has no such section" — it now rethrows limit violations while still tolerating ordinary per-request failures, which is the point of pipelining. literalSizeLimit (MEDIUM). Not fixable here: IMAPClientHandler constructs its processor with maximumBufferSize: IMAPDefaults.lineLengthLimit, hardcoded. Above 8 KiB the outcome depends on how the network fragments the literal, and a configuration that depends on packet boundaries is not a limit. The initializer now rejects such values instead of accepting a promise it cannot keep. The error-close is deliberately narrow — only ExceededMaximumBodySizeError, ExceededMaximumMessageAttributesError and ExceededResponseBodySizeError. errorCaught sees unrelated failures too, and closing on those would change behaviour far outside this feature. Note on the tests: the first version of the fail-closed tests passed against an EmbeddedChannel that was never connected — isActive is false there to begin with, so they were green for the wrong reason. They now connect first and assert the precondition. Verified locally: 346 tests pass, swiftlint --strict reports 0 violations in 264 files.
The Swift workflow has been cancelled on this branch since ac5ba8b, and I only looked once all the review threads were closed. It was not supersession: build-macos hits its `timeout-minutes: 10` at "Test (macOS)" every time, on every head. main and other branches are green, so this is ours — mine, specifically, introduced with the TLS floor tests. Cause: `try? group.syncShutdownGracefully()` called straight from a swift-testing test blocks a cooperative-pool thread. On a core-constrained runner that violates the pool's forward-progress guarantee and deadlocks the whole run. Locally, on a machine with cores to spare, 346 tests finish in 1.6 seconds and nothing hangs — which is why it never showed up here. None of this is new. #179 hit exactly this, documented it in IMAPResponseBufferLimitTests ("a 7-minute hang to the job timeout was observed"), and solved it by dispatching the blocking call to a non-cooperative GCD queue. I wrote the same trap next to the note warning about it. The fix is that same pattern, extracted to `shutDownGracefully(_:)` in a shared test-support file rather than copied a third time — so the next test that needs an event loop group finds the answer instead of the trap. The new suites also adopt `.serialized` and `.timeLimit(.minutes(1))`, matching the ones already in the repo: a test that hangs should fail in a minute and say so, not consume the job timeout and report as "cancelled", which is a lot easier to miss than a red X. Also dropped a `defer { Task { ... } }` that would have let a shutdown outlive its test. Verified locally: 346 tests pass, swiftlint --strict clean across 265 files.
The previous commit fixed the shutdown and missed the start. `build-macos` still hit its timeout, and the log named the culprit: "Implicit TLS refuses a TLS 1.2 peer when the floor is TLS 1.3" — started, never finished. `TLS12OnlyServer.init()` called `bind(...).wait()`. Same trap as `syncShutdownGracefully()` wearing a different hat: any `.wait()` reached from a swift-testing test blocks a cooperative-pool thread, and a core-constrained runner then loses its forward-progress guarantee. It also explains why `.timeLimit(.minutes(1))` did not save the job — a blocked thread cannot be cancelled, so the suite ran into the 10-minute job timeout instead of failing in one. The hint was there to be read: nothing else in this test suite calls `.wait()`. Not one call site. I added the only three. Now `async` throughout, with `.get()` instead of `.wait()`. Verified locally: 346 tests pass, swiftlint --strict clean. Verified on CI is the part that matters here, and I will confirm it there rather than claim it.
Third attempt at the macOS CI job, and the first one aimed at the right target. The previous two fixed blocking calls one at a time — syncShutdownGracefully(), then bind().wait() — and each time it hung somewhere else. The real problem was not any single call: it was that this test needed real sockets at all. What the last run showed: 29 of 346 tests completed, then the whole run stalled until the job timeout. Not one hanging test — the cooperative pool starving. Real sockets need a MultiThreadedEventLoopGroup and its blocking teardown, and every blocking step reachable from a swift-testing test costs a pool thread. On a core-constrained runner that is fatal, and it defeats .timeLimit too, since a blocked thread cannot be cancelled. Locally, with cores to spare, all 346 pass in 1.6 seconds — which is why I kept not seeing it. EmbeddedChannel has no threads, no sockets, and no event loop to shut down. The handshake is still real: same NIOSSLClientHandler from the same makeTLSHandler the bootstrap calls, same BoringSSL, same version negotiation — only the transport is a byte pump. 8 milliseconds instead of 80, and nothing to starve. Added a third case while there: a TLS 1.2 floor must still negotiate 1.3 with a capable peer — a floor, not an exact match. Verified by reverting the fix: "A TLS 1.3 floor refuses a peer that stops at TLS 1.2" fails. One thing stated plainly in the file: these prove the floor is enforced once it reaches the handler, not that the call sites pass it — which is what the defect actually was. Calling makeTLSHandler directly hands it the value the bootstrap forgot. That gap is closed by removing the defaults, so an omission is a build error; the compiler is a better guarantee than a test, but it is a different one, and claiming otherwise here would repeat the mistake this file documents. 347 tests pass, swiftlint --strict clean.
…y has
The in-memory handshake did not fix the macOS job either: 27 of 347 tests, then a stall. So it
was never my TLS test. I had been fixing the thing I had just written, twice, instead of
looking at what changed.
What changed: this PR added two suites that construct `IMAPServer`/`SMTPServer` —
MailTLSConfigurationTests (4) and IMAPParserLimitsTests (2) — and neither carries `.serialized`.
Every pre-existing suite here that constructs a server does. That is not a style detail:
constructing a server allocates a MultiThreadedEventLoopGroup, and SMTPServer.deinit shuts it
down with the blocking syncShutdownGracefully(). Released from a Swift Concurrency context, each
one costs a cooperative-pool thread. Six of them running in parallel with everything else, on a
core-constrained runner, starves the pool — which is why unrelated tests ("ASCII display name
behaves like description") hang too, and why nothing reproduces locally on a machine with cores
to spare.
So: `.serialized` + `.timeLimit(.minutes(1))` on both, matching the convention. `.timeLimit` on
the two guard suites as well — they construct nothing, but a hanging test should fail in a
minute and say so rather than consume the job timeout and report as "cancelled".
The convention was visible in every neighbouring file. I wrote past it.
347 tests pass, swiftlint --strict clean.
macos-latest rotated to the macos-26 image on 2026-07-15. The pinned
Xcode 26.0 survives on that image but its iOS platform does not, and
-downloadPlatform cannot fetch it ('Unable to connect to simulator'),
so build-ios failed before compiling. Track latest-stable instead: the
image's newest Xcode is the one shipped with its platforms installed.
build-macos gets a watchdog around swift test: the run has been
deadlocking on CI and surfacing as a silent 10-minute stall reported
as 'cancelled'. If it stalls again the watchdog samples every test
process so the log carries the stuck thread stacks, then fails the
step honestly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The macOS CI job deadlocked ~70ms into every test run on this branch: thread samples from a hung run show all three cooperative-pool threads (one per core on the 3-core runner) parked in SMTPServer.deinit -> syncShutdownGracefully -> semaphore_wait, with every NIO event loop idle and a spare workqueue thread doing nothing. syncShutdownGracefully parks the calling thread on a semaphore whose signal is delivered via the default-QoS global queue - the same workqueue that backs the cooperative pool. Three suites releasing SMTPServers concurrently (SMTPTests, SMTPTransportSecurityTests, and this branch's new MailTLSConfigurationTests) fully consume a 3-wide pool with waiters, leaving no thread to deliver any of their wake-ups. main survives with two such suites by a single thread, which is why this only surfaced here, and a 12-core dev machine cannot starve at all, which is why it never reproduced locally. Fix: initiate shutdown with the callback form and do not wait on it. - SMTPServer.deinit no longer blocks. - IMAPServer.deinit no longer detours through a @mainactor task (which also serialized all shutdowns through the main actor). - The shared test helper and IMAPResponseBufferLimitTests' private copy of it resume from NIO's completion callback instead of parking a GCD worker on the same semaphore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three verified defects in the fail-closed mechanism, found in review: 1. ResponseDecoder wraps every parser error in IMAPDecoderError before IMAPClientHandler fires it down the pipeline, and isLimitViolation matched only the bare NIOIMAPCore types - so the guard's close never fired for a real parser-raised violation. Only the tests, which injected the bare types directly, ever saw it work. isLimitViolation now unwraps the wrapper, and a new test drives raw wire bytes through the real decoder so the production path is what is being proven. 2. ExceededLiteralSizeLimitError was not matched at all, so an oversized literal - the same class of DoS the PR cites - failed open. 3. The pipelined-fetch cleanup reordering left hasActiveHandler = true while awaiting the dispatcher's removal, a window in which an unsolicited BYE or EXISTS was neither dispatched nor buffered; main restored buffering before removing the handler. Ordering restored, keeping the PR's cleanup-on-throw property. Also: MailTLSConfiguration.makeClientConfiguration loses its silent .tlsv12 default - the PR's own rationale for removing such defaults from makeTLSHandler applies to the factory it calls; the watchdog now kills the surviving test-helper processes after sampling; stale doc reference to 'the pinned NIOIMAP' updated for the 0.3.0 dependency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
IMAPTestServer.stop() blocked its calling thread in
clientGroup.wait(timeout: 2s). Called from test defer blocks, that
parks a cooperative-pool thread per integration test - the bounded
cousin of the deinit park that deadlocked the 3-core CI runner.
stop() is now async: the continuation is resumed from
DispatchGroup.notify when the last client handler leaves, or by a GCD
timer after the same 2-second bound - both arrive as callbacks, so no
thread waits anywhere. Since defer cannot await, call sites use the new
scoped testServer.run { } which stops the server on success and on
throw alike; temp-dir removal stays in defer where sync cleanup belongs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every build-linux run since b29316e sat in 'Test (Linux)' until cancelled (several for 5+ hours - the job had no timeout). lldb stacks from a Docker repro (swift:6.3, 4 cpus) show the run wedged in the handshake tests' defer: EmbeddedChannel.finish() -> close().wait() -> pthread_cond wait, forever. close() after a completed handshake makes NIOSSL attempt a graceful TLS shutdown: send close_notify, complete the close on the peer's reply or on a timeout scheduled on the event loop. By then the test's byte pump has stopped, nothing will ever deliver the reply, and an EmbeddedEventLoop's clock never advances on its own - so wait() blocks for good. macOS happens to complete the close inline, which is why only Linux hung, and why it hung solo too: a single filtered test reproduces it. The teardown now initiates both closes, pumps the close_notify exchange to completion, advances embedded time past any scheduled shutdown timeout, and finishes accepting already-closed channels. Verified: the full suite passes in Docker (343 tests, 0.2 s) where it previously hung, and on macOS (349 tests). build-linux also gets the same watchdog as build-macos (gdb standing in for sample) plus timeout-minutes: 20, so a future hang fails in minutes with backtraces instead of squatting on a runner for 6 hours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Supersedes #185 by including it verbatim (davyd15's nine commits, authorship intact — GitHub will mark #185 merged when this lands) plus five commits that make it mergeable and fix defects found in review.
Why #185 could not merge
SMTPServer.deinitblocks insyncShutdownGracefully(), whose semaphore signal needs the same default-QoS workqueue that backs the cooperative pool. Add configurable minimum TLS version and IMAP parser limits #185's newMailTLSConfigurationTestswas the third suite concurrently releasing SMTP servers — on the 3-core runner, three parked waiters consumed the whole pool and nothing could deliver any wake-up. Diagnosed from watchdogsamplestacks of a hung run; main survives with two such suites by a single thread.b29316e, some runs sat 5+ hours — the job had no timeout): the TLS floor tests'EmbeddedChannel.finish()doesclose().wait(), and NIOSSL's graceful shutdown waits for a close_notify reply that can never arrive once the test's byte pump has stopped. Diagnosed via lldb in a 4-CPU Docker repro; macOS happens to complete the close inline.What the fix commits do
latest-stableXcode; build-macos and build-linux get watchdogs that dump thread stacks after 6 minutes and fail honestly instead of eating the job timeout; build-linux getstimeout-minutes: 20.SMTPServer.deinit/IMAPServer.deinitinitiate ELG shutdown with the non-blocking callback form; pipelined-fetch cleanup restores buffering before removing the dispatcher (a reorder in Add configurable minimum TLS version and IMAP parser limits #185 could drop unsolicited BYE/EXISTS during the removal await).ResponseDecoderwraps every parser error inIMAPDecoderError, whichisLimitViolationnever unwrapped — parser-raised violations did not fail closed outside tests (which injected bare error types). Also matchesExceededLiteralSizeLimitError, previously fail-open. New end-to-end test drives raw wire bytes through the real decoder.IMAPTestServer.stop()awaits its client drain via continuation instead of a blocking 2 sDispatchGroup.wait.Verification
LIBDISPATCH_COOPERATIVE_POOL_STRICT=1), 343 on Linux in Docker where the suite previously hung,swiftlint --strictclean.🤖 Generated with Claude Code