Skip to content

tls: defer re-entrant calls to SSL state machine from JS - #65105

Open
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-alpn-cb-write
Open

tls: defer re-entrant calls to SSL state machine from JS#65105
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-alpn-cb-write

Conversation

@pimterry

@pimterry pimterry commented Aug 7, 2026

Copy link
Copy Markdown
Member

This is a step towards #65035: making sure that we never mess with the OpenSSL/BoringSSL state machine from inside it.

Rather than hooking each of our callbacks/events inside the SSL library scope, I've wrapped each call that goes into OpenSSL at the top level, and then I'm checking against this in DoWrite, DoShutdown and Cycle, which can be triggered from JS. In each case, if we're inside the SSL library scope (which means we're being called from a JS callback that was triggered by the SSL library) then we defer.

This covers some cases that currently work by luck but will break soon in BoringSSL and some cases that already fail today, like the ALPNCallback write example (see #65035 for details).

This just covers SSL_read/SSL_write/SSL_shutdown which are impacted by the pending BoringSSL patch, we may need to review other SSL_set... calls as well later.

Signed-off-by: Tim Perry <pimterry@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 7, 2026
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.60870% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (27d6cfa) to head (30ff143).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_tls.cc 80.95% 3 Missing and 9 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65105      +/-   ##
==========================================
- Coverage   90.31%   90.30%   -0.02%     
==========================================
  Files         759      759              
  Lines      248258   248359     +101     
  Branches    46854    46867      +13     
==========================================
+ Hits       224225   224290      +65     
- Misses      15465    15478      +13     
- Partials     8568     8591      +23     
Files with missing lines Coverage Δ
src/crypto/crypto_tls.h 86.95% <100.00%> (+4.60%) ⬆️
src/crypto/crypto_tls.cc 78.53% <80.95%> (-0.20%) ⬇️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants