tls: fix leak of WriteWrap+TLSWrap combination #9586

indutny · 2016-11-12T21:23:10Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls

Description of change

Writing data to TLSWrap instance during handshake will result in it
being queued in write_item_queue_. This queue won't get cleared up
until the end of the handshake.

Technically, it gets cleared on ~TLSWrap invocation, however this
won't ever happen because every WriteWrap holds a reference to the
TLSWrap through JS object, meaning that they are doomed to be alive
for eternity.

To breach this dreadful contract a knight shall embark from the
close function to kill the dragon of memory leak with his magic
spear of destroySSL.

destroySSL cleans up write_item_queue_ and frees SSL structure,
both are good for memory usage.

R= @bnoordhuis and @nodejs/crypto

Writing data to TLSWrap instance during handshake will result in it being queued in `write_item_queue_`. This queue won't get cleared up until the end of the handshake. Technically, it gets cleared on `~TLSWrap` invocation, however this won't ever happen because every `WriteWrap` holds a reference to the `TLSWrap` through JS object, meaning that they are doomed to be alive for eternity. To breach this dreadful contract a knight shall embark from the `close` function to kill the dragon of memory leak with his magic spear of `destroySSL`. `destroySSL` cleans up `write_item_queue_` and frees `SSL` structure, both are good for memory usage.

indutny · 2016-11-12T21:23:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4832/

indutny · 2016-11-12T21:49:10Z

Unrelated CI failures as far as I can tell.

indutny · 2016-11-13T02:17:11Z

Here is a video about identifying this leak for anyone interested: https://asciinema.org/a/be58dh43vbsy7o02wqewxvrrt

cjihrig · 2016-11-13T15:00:36Z

lib/_tls_wrap.js

+        ssl._secureContext.context = null;
+      }
+    }
+    if (cb)


Can you check typeof cb === 'function'

cjihrig · 2016-11-13T15:01:54Z

test/parallel/test-tls-writewrap-leak.js

+  });
+
+  c.write('hello', common.mustCall((err) => {
+    assert.equal(err.code, 'ECANCELED');


Can you use assert.strictEqual() please.

subeeshcbabu-zz · 2016-11-13T20:54:15Z

Thanks @indutny for this

subeeshcbabu-zz · 2016-11-15T02:27:45Z

👍 Tested this patch on Mac OS x and I can see that, the patch fixes the leak, even after creating thousands of timeouts on connect. Awesome @indutny

I will share more details and numbers after testing this on our production environment.

indutny · 2016-11-15T02:29:22Z

@bnoordhuis or @shigeki do you have some time to take a look at this? This is a pretty severe issue and I hope that we could release a fix for it ASAP.

cc @nodejs/lts (since it needs to be backported to v4)

bnoordhuis · 2016-11-15T08:28:01Z

lib/_tls_wrap.js

+  }
+
+  // Invoke `destroySSL` on close to clean up possibly pending write requests
+  // that may self-reference TLSWrap, leading to leak


"leading to a leak" dot.

bnoordhuis · 2016-11-15T08:39:17Z

lib/_tls_wrap.js

+  const done = () => {
+    if (ssl) {
+      ssl.destroySSL();
+      if (ssl._secureContext.singleUse) {


What does .singleUse signify? It's always true as far as I can tell because it's the inversion of options.keepAlive, which is not a documented option and seems to have no test coverage or uses in core.

keepAlive is an option passed by https.Agent, I suspect that it should be tested in our test suite through https module.

I don't quite understand. keepAlive is a layer 7 thing, why would that influence the TLS layer?

bnoordhuis · 2016-11-15T08:41:15Z

lib/_tls_wrap.js

+    }
+    if (typeof cb === 'function')
+      cb();
+  };


I'm curious, what would happen when ssl.destroySSL() gets called synchronously, i.e., outside the callback?

Crash, because of StreamBase callbacks.

The reason is actually way simpler than this, I'll simplify the code.

Thank you for bringing my attention to this.

It crashes if done in the same tick because it may be invoked within the OpenSSL's call stack. If invoked with setImmediate - it won't crash.

Oh... just run the test suite after using setImmediate() and it confirms my initial assumption. Sorry, not going to fix it.

The write callbacks are invoked with UV_ECANCELED and thus things are getting back to tls_wrap.cc at places where it doesn't expect ssl_ to be nullptr. Therefore it is better to free it up after closing the underlying libuv handle.

To be honest, I'm so unhappy with what we have right now (I know that I wrote most of it). I wish we would move to uv_ssl_t.

bnoordhuis

LGTM with style nit.

bnoordhuis · 2016-11-15T13:09:14Z

test/parallel/test-tls-writewrap-leak.js

+})).listen(0, common.mustCall(() => {
+  const c = tls.connect({ port: server.address().port });
+  c.on('error', () => {
+    // Otherwise `.write()` callback won't be invoked


indutny · 2016-11-15T20:15:16Z

Landed in 478fabf, thank you!

Writing data to TLSWrap instance during handshake will result in it being queued in `write_item_queue_`. This queue won't get cleared up until the end of the handshake. Technically, it gets cleared on `~TLSWrap` invocation, however this won't ever happen because every `WriteWrap` holds a reference to the `TLSWrap` through JS object, meaning that they are doomed to be alive for eternity. To breach this dreadful contract a knight shall embark from the `close` function to kill the dragon of memory leak with his magic spear of `destroySSL`. `destroySSL` cleans up `write_item_queue_` and frees `SSL` structure, both are good for memory usage. PR-URL: #9586 Reviewed-By: Ben Noordhuis <[email protected]>

Writing data to TLSWrap instance during handshake will result in it being queued in `write_item_queue_`. This queue won't get cleared up until the end of the handshake. Technically, it gets cleared on `~TLSWrap` invocation, however this won't ever happen because every `WriteWrap` holds a reference to the `TLSWrap` through JS object, meaning that they are doomed to be alive for eternity. To breach this dreadful contract a knight shall embark from the `close` function to kill the dragon of memory leak with his magic spear of `destroySSL`. `destroySSL` cleans up `write_item_queue_` and frees `SSL` structure, both are good for memory usage. PR-URL: nodejs#9586 Reviewed-By: Ben Noordhuis <[email protected]>

indutny · 2016-11-15T20:20:23Z

Backport to v4.x: #9626

Writing data to TLSWrap instance during handshake will result in it being queued in `write_item_queue_`. This queue won't get cleared up until the end of the handshake. Technically, it gets cleared on `~TLSWrap` invocation, however this won't ever happen because every `WriteWrap` holds a reference to the `TLSWrap` through JS object, meaning that they are doomed to be alive for eternity. To breach this dreadful contract a knight shall embark from the `close` function to kill the dragon of memory leak with his magic spear of `destroySSL`. `destroySSL` cleans up `write_item_queue_` and frees `SSL` structure, both are good for memory usage. PR-URL: #9586 Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2016-11-17T20:35:23Z

This landed cleanly on v6.x fwiw

Writing data to TLSWrap instance during handshake will result in it being queued in `write_item_queue_`. This queue won't get cleared up until the end of the handshake. Technically, it gets cleared on `~TLSWrap` invocation, however this won't ever happen because every `WriteWrap` holds a reference to the `TLSWrap` through JS object, meaning that they are doomed to be alive for eternity. To breach this dreadful contract a knight shall embark from the `close` function to kill the dragon of memory leak with his magic spear of `destroySSL`. `destroySSL` cleans up `write_item_queue_` and frees `SSL` structure, both are good for memory usage. PR-URL: #9586 Reviewed-By: Ben Noordhuis <[email protected]>

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) #9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchán) #9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) #9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) #9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) #9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9735

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) #7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) #3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) #7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) #4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) #6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergström) #9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) #8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) #9586 PR-URL: #9736

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) nodejs/node#7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) nodejs/node#3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) nodejs/node#7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) nodejs/node#4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) nodejs/node#4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) nodejs/node#6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) nodejs/node#8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9736 Signed-off-by: Ilkka Myller <[email protected]>

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) nodejs/node#9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchan) nodejs/node#9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) nodejs/node#9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) nodejs/node#9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) nodejs/node#9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9735 Signed-off-by: Ilkka Myller <[email protected]>

This LTS release comes with 108 commits. This includes 30 which are doc related, 28 which are test related, 16 which are build / tool related, and 4 commits which are updates to dependencies. Notable Changes: The SEMVER-MINOR changes include: * build: - export openssl symbols on Windows making it possible to build addons linked against the bundled version of openssl (Alex Hultman) nodejs/node#7576 * debugger: - make listen address configurable in the debugger server (Ben Noordhuis) nodejs/node#3316 * dgram: - generalized send queue to handle close fixing a potential throw when dgram socket is closed in the listening event handler. (Matteo Collina) nodejs/node#7066 * http: - Introduce the 451 status code "Unavailable For Legal Reasons" (Max Barinov) nodejs/node#4377 * tls: - introduce `secureContext` for `tls.connect` which is useful for caching client certificates, key, and CA certificates. (Fedor Indutny) nodejs/node#4246 Notable SEMVER-PATCH changes include: * build: - introduce the configure --shared option for embedders (sxa555) nodejs/node#6994 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * src: - node no longer aborts when c-ares initialization fails (Ben Noordhuis) nodejs/node#8710 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9736 Signed-off-by: Ilkka Myller <[email protected]>

This LTS release comes with 144 commits. This includes 47 that are docs related, 46 that are test related, 15 which are build / tools related, and 9 commits which are updates to dependencies Notable Changes: * buffer: - coerce slice parameters consistently (Sakthipriyan Vairamani (thefourtheye)) nodejs/node#9101 * deps: - *npm*: - upgrade npm to 3.10.9 (Kat Marchan) nodejs/node#9286 - *V8*: - Various fixes to destructuring edge cases - cherry-pick 3c39bac from V8 upstream (Cristian Cavalli) nodejs/node#9138 - cherry pick 7166503 from upstream v8 (Cristian Cavalli) nodejs/node#9173 * gtest: - the test reporter now outputs tap comments as yamlish (Johan Bergstrom) nodejs/node#9262 * inspector: - inspector now prompts user to use 127.0.0.1 rather than localhost (Eugene Ostroukhov) nodejs/node#9451 * tls: - fix memory leak when writing data to TLSWrap instance during handshake (Fedor Indutny) nodejs/node#9586 PR-URL: nodejs/node#9735 Signed-off-by: Ilkka Myller <[email protected]>

indutny added the tls Issues and PRs related to the tls subsystem. label Nov 12, 2016

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Nov 12, 2016

indutny added lts-watch-v4.x labels Nov 12, 2016

cjihrig reviewed Nov 13, 2016

View reviewed changes

fixes

e6869b8

bnoordhuis requested changes Nov 15, 2016

View reviewed changes

fix

717f9bf

bnoordhuis approved these changes Nov 15, 2016

View reviewed changes

indutny closed this Nov 15, 2016

indutny deleted the fix/tls-leak branch November 15, 2016 20:15

indutny mentioned this pull request Nov 15, 2016

tls: fix leak of WriteWrap+TLSWrap combination #9626

Closed

4 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 17, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: fix leak of WriteWrap+TLSWrap combination #9586

tls: fix leak of WriteWrap+TLSWrap combination #9586

indutny commented Nov 12, 2016 •

edited

Loading

indutny commented Nov 12, 2016

indutny commented Nov 12, 2016

indutny commented Nov 13, 2016

cjihrig Nov 13, 2016

indutny Nov 14, 2016

cjihrig Nov 13, 2016

indutny Nov 14, 2016

subeeshcbabu-zz commented Nov 13, 2016

subeeshcbabu-zz commented Nov 15, 2016

indutny commented Nov 15, 2016

bnoordhuis Nov 15, 2016

indutny Nov 15, 2016

bnoordhuis Nov 15, 2016

indutny Nov 15, 2016

bnoordhuis Nov 15, 2016

bnoordhuis Nov 15, 2016

indutny Nov 15, 2016

indutny Nov 15, 2016

indutny Nov 15, 2016

indutny Nov 15, 2016

indutny Nov 15, 2016

bnoordhuis left a comment

bnoordhuis Nov 15, 2016

indutny commented Nov 15, 2016

indutny commented Nov 15, 2016

MylesBorins commented Nov 17, 2016

tls: fix leak of WriteWrap+TLSWrap combination #9586

tls: fix leak of WriteWrap+TLSWrap combination #9586

Conversation

indutny commented Nov 12, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

indutny commented Nov 12, 2016

indutny commented Nov 12, 2016

indutny commented Nov 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subeeshcbabu-zz commented Nov 13, 2016

subeeshcbabu-zz commented Nov 15, 2016

indutny commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Nov 15, 2016

indutny commented Nov 15, 2016

MylesBorins commented Nov 17, 2016

indutny commented Nov 12, 2016 •

edited

Loading