Skip to content

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Oct 24, 2025

Refs #58070

Fixes a crash caused by unbalanced external memory accounting when
tls.createSecureContext() is called with invalid minVersion/maxVersion values:

Stacktrace

#
# Fatal error in ../../v8/src/api/api.cc, line 12326
# Debug check failed: amount_of_external_memory_ == 0U (1024 vs. 0).
#
#
#
#FailureMessage Object: 0x16ef3daa8
----- Native stack trace -----

 1: 0x11b8a2898 node::NodePlatform::GetStackTracePrinter()::$_0::__invoke() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 2: 0x122f0eba4 V8_Fatal(char const*, int, char const*, ...) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 3: 0x122f0e734 v8::base::SetFatalFunction(void (*)(char const*, int, char const*)) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 4: 0x11ce64fdc v8::ExternalMemoryAccounter::~ExternalMemoryAccounter() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 5: 0x11b7ba948 node::Environment::~Environment() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 6: 0x11b754434 node::FreeEnvironment(node::Environment*) [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 7: 0x11b480614 electron::NodeMain() [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 8: 0x11b478fe0 ElectronInitializeICUandStartNode [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework]
 9: 0x100ec04d0 main [/Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/MacOS/Electron]
10: 0x19a11dd54 start [/usr/lib/dyld]
Command: /Users/codebytere/Developer/electron-gn/src/out/Testing/Electron.app/Contents/MacOS/Electron --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/codebytere/Developer/electron-gn/src/third_party/electron_node/test/parallel/test-tls-basic-validations.js
--- CRASHED (Signal: 5) ---

Prior to this change, lib/internal/tls/common.js instantiated a native SecureContext
which incremented V8 external memory via
env->external_memory_accounter()->Increase(kExternalSize) in crypto_context.cc
before protocol version validation ran in toV(), so an early
ERR_TLS_INVALID_PROTOCOL_VERSION throw left the +1024 bytes un-decremented
and V8 asserted in ExternalMemoryAccounter::~ExternalMemoryAccounter
during Environment teardown.

Fix this by reordering the constructor to validate minVersion/maxVersion first and
only allocate the native SecureContext on success.

@codebytere codebytere requested a review from targos October 24, 2025 13:41
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Oct 24, 2025
@codecov
Copy link

codecov bot commented Oct 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.58%. Comparing base (ddbe136) to head (6bd1cf6).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60390      +/-   ##
==========================================
- Coverage   88.59%   88.58%   -0.01%     
==========================================
  Files         704      704              
  Lines      207774   207775       +1     
  Branches    40035    40028       -7     
==========================================
- Hits       184068   184065       -3     
- Misses      15748    15757       +9     
+ Partials     7958     7953       -5     
Files with missing lines Coverage Δ
lib/internal/tls/common.js 97.45% <100.00%> (+0.01%) ⬆️

... and 23 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.

@lpinca
Copy link
Member

lpinca commented Oct 24, 2025

Is it possible to add a test?

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Oct 26, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2025
@nodejs-github-bot nodejs-github-bot merged commit fb84f35 into nodejs:main Oct 26, 2025
69 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in fb84f35

aduh95 pushed a commit that referenced this pull request Oct 26, 2025
PR-URL: #60390
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants