Repository: nodejs/node
Pull request: fix: avoid external memory leak on invalid TLS protocol versions

Refs https://github.com/nodejs/node/pull/58070

Fixes a crash caused by unbalanced external memory accounting when
`tls.createSecureContext()` is called with invalid minVersion/maxVersion values:


<details><summary>Stacktrace</summary>
<p>

```
#
# 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) ---
```

</p>
</details> 

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.

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/tls/common.js
patch: @@ -82,10 +82,11 @@ function SecureContext(secureProtocol, secureOptions, minVersion, maxVersion) {
       throw new ERR_TLS_PROTOCOL_VERSION_CONFLICT(maxVersion, secureProtocol);
   }
 
+  const minV = toV('minimum', minVersion, tls.DEFAULT_MIN_VERSION);
+  const maxV = toV('maximum', maxVersion, tls.DEFAULT_MAX_VERSION);
+
   this.context = new NativeSecureContext();
-  this.context.init(secureProtocol,
-                    toV('minimum', minVersion, tls.DEFAULT_MIN_VERSION),
-                    toV('maximum', maxVersion, tls.DEFAULT_MAX_VERSION));
+  this.context.init(secureProtocol, minV, maxV);
 
   if (secureOptions) {
     validateInteger(secureOptions, 'secureOptions');