From 9bd34dccf73170bbc7c32eabb3a6aad0efe33542 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 17 Mar 2017 11:35:28 +0100 Subject: [PATCH] tls: fix SecurePair external memory reporting Ensure that AdjustAmountOfExternalAllocatedMemory() is called when the SecurePair is destroyed. Not doing so is not an actual memory leak but it makes `process.memoryUsage().external` wildly inaccurate and can cause performance problems due to excessive garbage collection. --- src/node_crypto.cc | 6 +---- test/parallel/test-tls-securepair-leak.js | 29 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-tls-securepair-leak.js diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 700508067790de..7dbc15e32f00a5 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3245,11 +3245,7 @@ void Connection::Start(const FunctionCallbackInfo& args) { void Connection::Close(const FunctionCallbackInfo& args) { Connection* conn; ASSIGN_OR_RETURN_UNWRAP(&conn, args.Holder()); - - if (conn->ssl_ != nullptr) { - SSL_free(conn->ssl_); - conn->ssl_ = nullptr; - } + conn->DestroySSL(); } diff --git a/test/parallel/test-tls-securepair-leak.js b/test/parallel/test-tls-securepair-leak.js new file mode 100644 index 00000000000000..b513bcd4c7c73a --- /dev/null +++ b/test/parallel/test-tls-securepair-leak.js @@ -0,0 +1,29 @@ +// Flags: --expose-gc --no-deprecation +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} + +const { createSecureContext } = require('tls'); +const { createSecurePair } = require('_tls_legacy'); + +const before = process.memoryUsage().external; +{ + const context = createSecureContext(); + const options = {}; + for (let i = 0; i < 1e4; i += 1) + createSecurePair(context, false, false, false, options).destroy(); +} +global.gc(); +const after = process.memoryUsage().external; + +// It's not an exact science but a SecurePair grows .external by about 45 kB. +// Unless AdjustAmountOfExternalAllocatedMemory() is called on destruction, +// 10,000 instances make it grow by well over 400 MB. Allow for some slop +// because objects like buffers also affect the external limit. +assert(after - before < 25 << 20);