Skip to content

Fail the SSL call when the write callback's socket write fails - #86

Draft
krynju wants to merge 2 commits into
JuliaWeb:mainfrom
krynju:kr/write-callback-minus-one
Draft

krynju wants to merge 2 commits into
JuliaWeb:mainfrom
krynju:kr/write-callback-minus-one

Conversation

@krynju

@krynju krynju commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Split out of #73, as asked in the review there: the first of the independent fixes, against main.

Bug. on_bio_stream_write returns 0 when the socket write inside it throws. With no retry flag set on the BIO, OpenSSL (3.5 included) takes a 0 from a write callback as "nothing written yet": it keeps the record pending and SSL_write_ex reports success. A write whose socket had already closed is therefore returned to the caller as written.

Fix. Return -1 from the callback's catch, which fails the SSL call in every OpenSSL version; @geterror then closes the stream and throws IOError, as it does for any other SSL error.

Also: with -1, OpenSSL queues its own account of the failure (tls_retry_write_records failure, ERR_LIB_SYS) on the thread's error queue. @geterror's error branch now drains the queue into the IOError message instead of leaving it for an unrelated call to report (review finding).

Tests. WriteCallbackFailure closes the socket under a connected stream and checks that the next write throws IOError naming SSL_ERROR_SYSCALL, the stream is closed, and the error queue is empty. On main the write returns 3 and the stream stays open. RawWriteFailure does the same through a raw SSL_write_ex on ssl.ssl: the call fails with SSL_ERROR_SYSCALL, and on OpenSSL 3.2+ its entry stays on the queue for the raw caller to read, as for any raw call (1.1 and 3.0 record nothing for a SYSCALL error; the test checks the queue only on 3.2+, which the OpenSSL 1.1 CI run on LTS needs).

The test file gains three small helpers (self-signed server context, a handshake loop over the one-round accept(ssl), a connected client/server pair). #87 and #88 add the same block in the same place, so whichever lands later only has to re-append its testset.

Stacked on #89. Its commit is included here because the two interact on nightly: there the finalizer's close_notify write throws, the callback now returns -1 instead of 0, and OpenSSL records that failure on the thread's error queue, which ErrorTaskTLS later asserts is empty (first CI run of this PR). With #89 the finalizer writes nothing. Once #89 is merged this PR is the one callback change.

No version bump; the fixes can be released together.

🤖 Generated with Claude Code

The finalizer called `close(ssl)`, which sends the close_notify, a
socket write. A write that has to wait for the socket switches tasks,
which a finalizer may not do: on nightly, where the socket write path
waits more readily, `NoCloseStream` printed "task switch not allowed
from inside gc finalizer". Older versions only wait when the socket
buffer is full, so the hazard was latent. The finalizer now calls
`close(ssl, false)`: the SSL object is freed and the socket closed,
nothing is written.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.11%. Comparing base (10f7b7b) to head (d2f66bb).
⚠️ Report is 41 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   77.56%   79.11%   +1.54%     
==========================================
  Files           2        2              
  Lines        1083     1192     +109     
==========================================
+ Hits          840      943     +103     
- Misses        243      249       +6     

☔ 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
krynju force-pushed the kr/write-callback-minus-one branch from d1e156d to 31aac48 Compare October 16, 2026 10:00
@krynju
krynju marked this pull request as draft October 16, 2026 10:16
@krynju
krynju force-pushed the kr/write-callback-minus-one branch from 31aac48 to 5818bae Compare October 16, 2026 10:50
`on_bio_stream_write` returned 0 when `unsafe_write` on the socket threw.
With no retry flag set on the BIO, OpenSSL (3.5 included) takes a 0 from
a write callback as "nothing written yet": it keeps the record pending
and `SSL_write_ex` reports success. A write whose socket had already
closed was therefore returned to the caller as written. Return -1, which
fails the SSL call; `@geterror` then closes the stream and throws
`IOError`.

Adds `WriteCallbackFailure`: closes the socket under a connected stream
and checks that the next write throws and closes the stream.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@krynju
krynju force-pushed the kr/write-callback-minus-one branch from 5818bae to d2f66bb Compare October 16, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant