Fix read/write deadlock (#72): write ciphertext outside ssl.lock; defined close/cancel/timeout semantics

[krynju]

Fixes #72.

The bug

SSLStream guards its SSL object with one lock, as OpenSSL requires. But the write BIO callback wrote to the socket from inside that lock:

function on_bio_stream_write(bio::BIO, in::Ptr{Cchar}, inlen::Cint)::Cint
    io = bio_get_data(bio)::TCPSocket
    written = unsafe_write(io, in, inlen)   # blocks until the peer makes room

SSL_write_ex, SSL_connect, SSL_accept and SSL_shutdown hold ssl.lock while OpenSSL runs that callback, and SSL_read_ex needs the same lock. A write waiting for the peer's receive window therefore blocked every read on the connection. Any traffic that fills both directions at once deadlocks.

The fix

before                                        after

SSL_write_ex ── ssl.lock held ──┐             SSL_write_ex ── ssl.lock held ──┐
   └─ BIO write                 │                └─ BIO write: append to buf  │  short
        └─ socket write ────────┘  blocks      take!: swap buf out, get ticket ┘
             (waits for peer)      reads      drain!  ── no ssl.lock ──────────
                                                 └─ wait for ticket's turn
                                                 └─ socket write (waits for peer)

• The write BIO now only appends to a buffer. The task that made the SSL call takes its output with take! while it still holds ssl.lock. Then it releases the lock and writes the chunk to the socket with drain!.
• A ticket, handed out in take!, keeps chunks in the order OpenSSL produced them, which the record MAC requires.
• Each writer waits for its own bytes. write returns once its ciphertext has reached the socket, and gets the IOError itself if it did not.
• Output produced during a read, such as a KeyUpdate response, is sent from a task of its own. A reader never waits behind a writer.

Most of the diff is about keeping this correct when tasks are cancelled or the stream is closed. The resulting behaviour:

Closing

• close(ssl) is graceful.
  • Writes already issued on other tasks finish first. Later writes are refused, and iswritable returns false from the start of the close.
  • The close_notify goes out behind those writes, then the socket is closed after its queue is flushed.
• close(ssl, false) aborts. The stream is marked closed at once. Output produced before the abort still goes out in order, then the socket is closed.
• Both closes are bounded by progress, not by a fixed time. A writer parked on a peer that has stopped reading would otherwise hold up either close forever.
  • A watch closes the socket outright once nothing has moved for CLOSE_GRACE (10 s, graceful close) or ABORT_GRACE (1 s, abort). That fails the parked write.
  • A peer that keeps reading, however slowly, is not cut off.
• A failed SSL call aborts the stream in one step, under ssl.lock.
  • The alert OpenSSL produced still reaches the peer.
  • The IOError now carries OpenSSL's reason, for example tlsv1 alert unknown ca or certificate verify failed.
  • The thread's error queue is left empty afterwards.

Cancellation

• A writer cancelled while it waits for its turn still consumes its ticket, so the writers behind it are not parked forever. This covers schedule(task, ex; error=true), an interrupt and a timeout wrapper. The cancelled writer's record is lost, which makes the stream unusable, so the stream is aborted.
• A writer cancelled during the socket write has its socket closed outright, because a graceful close would wait behind that same write. libuv still holds a pointer into the chunk, so the chunk is kept alive until the close completes. This prevents a use-after-free.
• Cleanup paths hold interrupts while they take their locks. An exception thrown into a task waiting on one of those locks cannot skip the cleanup. The remaining cleanup is handed to a library task.

Handshake

• Sockets.connect(ssl; timeout=…) and Sockets.accept(ssl; timeout=…) take one deadline for the whole handshake, including the certificate check. On any failure, including the timeout, the stream is closed. timeout=Inf runs the whole handshake with no deadline.
• Sockets.accept(ssl) without timeout keeps its old contract: one round, OpenSSLError when it needs more bytes from the peer, the caller's loop waiting and checking its own deadline between calls. A round that fails outright now throws IOError and closes the stream, as every SSL call does.

Compatibility

The 1.x contracts are kept, so this can go out as 1.7.0: a minor bump for the new timeout keyword, nothing a "1.6" compat entry picks up changes behaviour.

• Sockets.accept(ssl) without timeout is still one non-blocking round that throws OpenSSLError when it needs more bytes. Retry loops with their own deadline work as before (tested: LegacyAccept). The whole handshake is accept(ssl; timeout=Inf) or a deadline. One deliberate difference: a round that fails outright (a rejected certificate, say) throws IOError and closes the stream, as every SSL call on this branch does; main threw OpenSSLError there too and left the broken stream open.
• Calls made on ssl.ssl from outside the package keep working. The write BIO callback knows whether one of the package's own calls is in progress (incall); outside one, it writes to the socket itself, as every write did before, after waiting for any records the package's calls have ticketed, so order is kept. Such callers hold ssl.lock, as they always had to. The wait for the ticketed records is bounded: if nothing moves for CLOSE_GRACE, the raw call fails rather than holding ssl.lock, where no close or watch could reach it, for good. The socket write itself, once started, blocks on a peer that does not read as every write did before 1.6.2, until the caller closes the socket. A raw write that fails marks the stream unsendable; one that libuv may still hold also cuts the socket, so the request is cancelled before SSL_free frees its buffer.
• A failing write callback now returns -1, not 0. OpenSSL up to 3.5.6 takes a zero from a legacy write callback with no retry flag as "nothing written yet", reports the call a success and keeps the record pending for the next call; -1 fails the call in every version. The callback's catch-all did return 0 on main as well, so on 3.x a socket error inside it was reported to the caller as a successful write. TLSStreams 0.2.0's own SSL_accept loop completes the handshake on this branch unchanged (tested: RawSSLCalls, and the TLSStreams suite).
• ssl_accept(::SSL) is back, unchanged from main.
• Two undocumented things did change: close(ssl) returns nothing rather than the @async task that closed the socket, and the readbytes/writebytes fields of SSLStream are gone (the counts are per call now). Say if either should be kept too.
• get_error gives the same result as before. It now reads the error queue through a helper, errorqueue, which is also what IOError messages use.

Verification

• CI covers Julia LTS, 1 and nightly on Linux, macOS and Windows. The LTS job's "Test with OpenSSL v1.1" step runs the suite against OpenSSL_jll 1.1 as well (it caught a test race on this branch).

  • The two "HTTP.jl" integration jobs exercise nothing here: HTTP.jl master (2.x) no longer depends on OpenSSL.jl, which is also why the OpenSSL 1.1 integration job skips. HTTP.jl 1.x over this branch was checked locally instead: 480 keep-alive GET/POST rounds on 16 threads against a local server, then Connection: close requests.
  • Locally, the suite also passes on 1.6, 1.7, 1.8, 1.10 and 1.12, with 1 and with 4 threads, using OpenSSL_jll 3.5.6.
  • codecov/patch is red for a measurement reason: GitHub's API omits the diff of src/ssl.jl as too large, so codecov has no diff lines for it and scores the patch over src/OpenSSL.jl alone (35 lines, 23 hit). The misses are errorqueue's allocation-failure fallbacks. Project coverage is up from 77.6% to 80.7%.

• 33 new testsets cover:

  • concurrent readers and writers
  • cancelled and parked writers
  • graceful and aborting closes against a stalled peer
  • handshake deadlines and alert delivery
  • interrupted cleanup, a logger that throws, and lost records
  • the old accept contract, raw SSL_* calls (including one that gives up behind a ticket nobody drains), and a reply task that cannot be scheduled

• Reads that could stall run under deadlines, so a regression fails its testset instead of hanging the suite.

• ConcurrentReadWrite is the SSL_write holds ssl.lock across the socket write, so a blocked write deadlocks reads on the same SSLStream #72 scenario: one write parked against a peer that stopped reading, and one read of bytes the peer already sent. On main the read times out; on this branch it completes.

• Performance against main, over loopback on one thread:

  	main	this PR
  1 MiB writes	800–850 MB/s	735–758 MB/s
  16 KiB writes	811–819 MB/s	750–765 MB/s
  64 B writes	217–220 k/s	193–206 k/s
  allocation per 64 B write	27 B	75 B
  100 B request/response round trip	31–32 µs	35–36 µs

  Each SSL_write_ex call covers one record (16 KiB), and a chunk that reached the socket becomes the next buffer. Before that change, 1 MiB writes ran at under half of main's speed.

• A self-signed server rejected by a verifying client now fails with tlsv1 alert unknown ca, not a bare EOF.

• Two platform differences show up in the tests. The tests handle both, so neither is a library bug:

  • On Windows, closing a socket while the server's session tickets are still unread sends the peer a reset.
  • On macOS, a raw peer reader sees neither FIN nor RST after the client's fallback close until the server side closes too.
  • On Windows the kernel keeps growing the socket buffers for a peer that does not read, so a "parked" write completes on its own. That exposes a race in Base, not in this package: uv_writecb_task schedules the task waiting in uv_write unconditionally while the request still names it, so a task cancelled (schedule(task, ex; error=true)) just as its write completes gets "schedule: Task not runnable" thrown out of the libuv callback into whatever task runs the event loop, and the loop can wedge. The two testsets that cancel a writer inside the socket write, CancelledInFlightWriter and ThrowingLogger, are skipped on Windows for that reason. The package's own handling of such a cancellation (the kept chunk, the cut) is unaffected; it is only the moment of cancellation Base cannot make safe.

Also in here: an unrelated CI failure

ReadPEMCert took the second entry of MozillaCACerts_jll.cacert and asserted that it has an OU. In the bundle shipped with Julia 1.13 and nightly, that entry has none. The testset failed, and because it sits at top level it stopped the rest of the file. The test now picks a certificate that has the fields it checks. Happy to split this out.

Notes for review

• The work since the first review is one commit, "Address review: …". After it come the version bump, the write-path performance fix, test-only commits that fix CI on Windows, macOS and Linux, the commit for the second review round (the 1.x contracts and the unscheduled reply task), and one that bounds the raw path and fixes the write callback's failure return.
• A few failure paths have no test, because triggering them needs fault injection: an interrupt or an allocation failure at a chosen point. The code comments say what each one covers.
• A possible follow-up, not in this PR: simplify the allocation-failure handling if it seems heavier than it is worth. Most of it guards against out-of-memory events.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (10f7b7b) to head (abe9d7a).
⚠️ Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
src/OpenSSL.jl	50.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   77.56%   80.71%   +3.14%     
==========================================
  Files           2        2              
  Lines        1083     1742     +659     
==========================================
+ Hits          840     1406     +566     
- Misses        243      336      +93     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[krynju]

@quinnj Can you please take a look at this? This was supposed to only fix a deadlock scenario, but has a few more fixes as well. Every CI run and review I ran uncovered more random issues, but it should be good now

[krynju]

@quinnj I addressed these review comments you left and ran a couple of reviews overnight. It was bringing up correctness issues until review number ~60
The state now is that I ran one last correctness review, performance benchmark and fixed one performance regression. Should be good at this point I hope

[quinnj]

[reviewed by AI] Thanks for working through the earlier comments. I re-reviewed 04bffd45cb365276576e09e29e41ec2446ed9146. The five earlier findings are addressed: the EOF scope fix, cancellation during lock acquisition, aborting an in-progress graceful close, queued-writer ordering, and the accept timeout/documentation changes.

I still see two issues to address before merging: the minor version bump doesn't isolate the breaking API changes, and failure to submit a detached drain task can leave later writers stuck. Details inline.

Pkg.test() passes locally on macOS with Julia 1.12.7 and OpenSSL_jll 3.5.6, with both 1 and 4 threads. The platform-specific FailedWriteKeepsNothing test wasn't exercised locally. All 14 CI checks are green, and the normal HTTP.jl integration job ran its tests. The OpenSSL 1.1 integration job skipped its tests after a dependency resolution failure (OpenSSL_jll@1.1 conflicts with the project's 3.5.6 - 3 compatibility); its green check doesn't establish 1.1 compatibility.

The record-chunk reuse checks passed byte-for-byte, and I didn't reproduce a bulk throughput regression in a loopback benchmark with matched read-ahead and TCP_NODELAY. Small-write timings varied too much to confirm the reported 8–12% cost.

[quinnj]

[reviewed by AI] [P1] A minor bump won't protect existing callers from these API changes

Julia's default caret compatibility treats OpenSSL = "1.6" as >=1.6.0, <2.0.0; 1.7.0 is accepted. I verified this with Pkg.Types.semver_spec("1.6"). The PR's claim that the minor bump keeps those environments on 1.6 is incorrect.

The timeout keyword and doc changes address my earlier comment, but existing callers still need code changes. With a silent TCP client, an unchanged retry loop with a 0.5 s deadline returns :deadline_expired on the base; here it is still blocked in its first accept(ssl) call after 1.5 s. A raw SSL_accept loop also completes on the base but stalls here with 2,316 bytes of unsent handshake ciphertext.

Can we preserve the existing contracts in 1.x, or make this a 2.0 change with migration guidance? Documentation and the new keyword won't stop a compatible dependency update from hanging existing callers.

[krynju]

[addressed by Claude] Fixed in fb331be. You're right that "1.6" admits 1.7.0; that claim was wrong. I went with keeping the 1.x contracts rather than a 2.0:

• Sockets.accept(ssl) without timeout is one non-blocking round again and throws OpenSSLError when it needs more bytes, so a retry loop with its own deadline behaves as on main (LegacyAccept tests a 0.5 s deadline against a silent client). The whole handshake is accept(ssl; timeout=Inf) or with a deadline.
• Calls made on ssl.ssl from outside the package work again. The write BIO callback checks whether one of the package's own calls is in progress (incall); outside one it writes to the socket itself, as before, after waiting for any ticketed records so order is kept. TLSStreams 0.2.0's raw SSL_accept loop completes the handshake on this branch unchanged (RawSSLCalls, plus the TLSStreams suite).
• ssl_accept(::SSL) is restored as it was.
• Two deliberate differences remain in those paths: a failed round throws IOError and closes the stream rather than throwing OpenSSLError and leaving it open, and a raw write that fails cuts the socket (libuv may still hold a request pointing into OpenSSL's buffer, which SSL_free would otherwise free under it). A raw write parked on a peer that does not read still blocks under ssl.lock, as every write did before 1.6.2; only the caller can bound that, by closing the socket.

Two undocumented things still differ: close(ssl) returns nothing instead of the @async task, and the readbytes/writebytes fields are gone. Happy to restore either if you think they matter. The version stays 1.7.0 for the new timeout keyword.

[krynju]

[addressed by Claude] Follow-up in 3bb045b, after another review pass over the raw-call path:

• The wait a raw write does for the ticketed records before it is now bounded: it holds ssl.lock, which every close, abort and watch needs first, so a ticket no task would ever drain could have held the stream for good. It gives up once nothing has moved for CLOSE_GRACE, and the call fails (RawWriteGivesUp). The socket write itself still blocks on a peer that does not read, as before 1.6.2.
• A failed raw write cuts the socket only when libuv may still hold the request; otherwise it just marks the stream unsendable, so records before it still go out with the close.
• The write callback returns -1 on failure, not 0. OpenSSL up to 3.5.6 takes a zero with no retry flag as "nothing written yet", reports the call a success and keeps the record pending (verified against the 3.5.6 source; the 3.5 branch has since made it fatal). The callback's catch-all returned 0 on main too, so on 3.x a socket error inside it was reported as a successful write.
• finish_sslcall! gives the reply's ticket up only when the task was not scheduled, so a drain and the catch cannot both pass one turn.
• CancelledInFlightWriter and ThrowingLogger run on Linux only: they cancel a writer inside the socket write, and Base's uv_writecb_task schedules the waiting task unconditionally while the request names it, so a write completing at that moment throws "schedule: Task not runnable" out of the libuv callback. Windows and macOS grow the socket buffers for a non-reading peer, which completes the parked record and hits that window (it hung two Windows jobs). Not something this package can fix.

[quinnj]

[reviewed by AI] [P2] Handle failure to create or schedule the detached drain task

pending already owns a ciphertext ticket here. The try/catch inside the task can't run if background(...) itself throws, so the record is neither drained nor abandoned and the stream stays open.

I checked this with a valid 27-byte KeyUpdate record and targeted fault injection: make background throw OutOfMemoryError before it submits the task, then restore it. That leaves turn == 2, nextticket == 3, and an empty abandoned set. A later write stayed parked on ticket 3 after 0.5 s. Injecting InterruptException at the same boundary gave the same result.

Please cover task creation/submission with cleanup that accounts for the ticket and aborts the stream before rethrowing. Simply advancing the turn would send later ciphertext across a missing TLS record. A regression test should verify that subsequent writers terminate with an error rather than waiting forever.

[krynju]

[addressed by Claude] Fixed in fb331be. finish_sslcall! now wraps the task's creation and scheduling; if that throws, the ticket is given up with abandon!, which refuses every later ticket instead of parking them, and the stream is aborted, both with interrupts held back, then the exception is rethrown. If the task was scheduled after all (an interrupt landing after the fact), its drain finds the ticket given up and does nothing harmful.

DetachedDrainNotScheduled covers it. It drives finish_sslcall! directly with a taken ticket, through an ONBACKGROUND test hook that makes background throw OutOfMemoryError for that task only: on OpenSSL 3.5.6 the client's reply to a KeyUpdate goes out with its next write rather than during the read, so real traffic does not reach this path here. The test checks the ticket is consumed, lostfrom is set, and a later write fails with IOError at once rather than waiting.