Skip to content

quic: fix per-session SSL leak - #66730

Open
pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-quic-tls-leak
Open

pimterry wants to merge 1 commit into
nodejs:mainfrom
pimterry:fix-quic-tls-leak

Conversation

@pimterry

Copy link
Copy Markdown
Member

Every single QUIC session (clients and servers) currently leaks the SSL instance, leaking 10KB (server) or 30KB (client) for every connection that is never recovered.

This happens because:

  • TLSSession::Initialize creates a SSL from the context.
  • We pass this to ngtcp2 with ngtcp2_crypto_ossl_ctx_set_ssl.
  • Ngtcp2 borrows it, but doesn't take ownership.
  • We assume it does (calling .release()) so we never clean it up, and ngtcp2 never does either.

For the server every connection leaks the SSL, for the client it leaks SSL + SSL_CTX (since we create a new ctx for every connection).

This PR fixes that:

  • OSSLContext now owns the SSL pointer
  • When we clean up OSSLContext in OSSLContext::reset() we explicitly clean up references to the SSL, and free the SSL itself.

Found and tested via ASAN - there's other leaks in QUIC in some cases, but this is easily the biggest one, and fixing this does reliably resolve it.

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation. labels Oct 14, 2026
@codecov

codecov Bot commented Oct 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.47%. Comparing base (ea6ec93) to head (93ad3d6).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66730      +/-   ##
==========================================
- Coverage   90.48%   90.47%   -0.01%     
==========================================
  Files         791      791              
  Lines      277204   277204              
  Branches    53301    53318      +17     
==========================================
- Hits       250825   250814      -11     
- Misses      16803    16806       +3     
- Partials     9576     9584       +8     

see 30 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++. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants