joyent/node — pull request #5465 by lsegal (2013-05-13 21:46:13+00:00)
Title: crypto: Fix "leak" in SecureContext::SetCert()

Node.js borrows a function `SSL_CTX_use_certificate_chain` from
OpenSSL which handles loading of the certificate bundle provided from
a buffer object. However, it looks like the code taken from OpenSSL
was originally hanging onto the certificate resource created by
SSL_CTX_use_certificate, but it does not seem to be used by Node.js,
and not freeing it causes a large amount of memory to be generated
and lost (though eventually partially returned in a full GC run). I'm
not sure if this is necessarily a "memory leak", but at the very least
it is causing less-than-efficient memory usage. The patch frees the
resource creating a much more stable memory usage footprint.

In order to illustrate the "leak" I wrote a minimal reproduction case
listed below which causes system memory usage to skyrocket when creating
crypto credentials. Note that SetCert() is exposed through
`crypto.createCredentials()`, but for clarity, this minimal test case
shows the actual code path within createCredentials() that causes the
"leak":

```js
var crypto = require('crypto');
var fs = require('fs');
var binding = process.binding('crypto');
var SecureContext = binding.SecureContext;

// load any cert bundle (we use one similar to Firefox)
var cert = fs.readFileSync(__dirname + '/ca-bundle.crt');

var numTimes = 1;
function run() {
var mem = process.memoryUsage();
console.log(numTimes + ' ' + mem.rss + ' ' + mem.heapUsed +
' ' + mem.heapTotal);

// the following 3 lines is equivalent to calling:
// crypto.createCredentials({cert: cert});
var context = new SecureContext();
context.init();
context.setCert(cert);

if (numTimes++ < 5000) setTimeout(run, 0);
}

console.log("Attempt RSS \"Heap Used\" \"Heap Total\"");
run();
```

To clarify, the leak is only caused when `crypto.createCredentials()`
is called with the `cert` option filled, which causes it to call
`SecureContext.setCert()`. Without this, the memory usage looks normal.

The following graphs show memory usage when running the above script before and after applying the patch in this pull request:

(**Note**: these graphs are on a *logarithmic scale*)

Before patch:

![certchain-leak-log](https://f.cloud.github.com/assets/686/498476/2f0a711c-bc16-11e2-95c2-402d46248b01.png)

After patch:

![certchain-fix-leak-log](https://f.cloud.github.com/assets/686/498480/3562dce8-bc16-11e2-859e-8c1f59be7dc7.png)

I also stress tested this implementation to ensure that freeing memory in the implementation does not cause any segfault/side effects-- **I AM NOT SURE THAT THIS IS WILL NOT CAUSE A SEGFAULT**, but in my experimentation, it did not cause any. Please try this out on your machines and verify my patch! :)

Finally, my pull request applies this patch on master, but given that it is a backwards compatible change, I believe that it can (and probably should) be backported, since the current behaviour can cause critically poor memory performance.