Strange Segfault with SSL/TLS

[wiggzz]

We are experiencing a segfault with Node 4.3.2 under certain conditions.

The following code replicates it:

'use strict';

const Promise = require('bluebird');
const request = require('request-promise');

function thenHandler(options) {
  return request(options)
}

let options = {
  url: '/sdgasdnasodgnkn',
  method: 'GET',
  baseUrl: 'https://google.com'
};

Promise.resolve(options)
.then(thenHandler)
.catch(error => {
  console.log(error.response.socket);
  throw error;
});

Interestingly, removing thenHandler or using HTTP, not HTTPS, does not cause the error.

[analog-nico]

Ok, let's see...

• What does the Segfault say?
• What do you mean by "removing the thenHandler"? Does this mean the ONLY change you make for that is not using a separate function but .then(options => { return request(options) }) directly?
• Please try the following code with HTTPS to allow us to identify if the issue is caused by request-promise or request:

  'use strict';

  const Promise = require('bluebird');
- const request = require('request-promise');
+ const request = require('request');

  function thenHandler(options) {
-   return request(options)
+   return new Promise((resolve, reject) => {
+     request(options, (error, response, body) => {
+       if (error) {
+         reject(error);
+       } else {
+         resolve(body);
+       }
+     });
+   });
  }

  let options = {
    url: '/sdgasdnasodgnkn',
    method: 'GET',
    baseUrl: 'https://google.com'
  };

  Promise.resolve(options)
  .then(thenHandler)
  .catch(error => {
-   console.log(error.response.socket);
    throw error;
  });

[pameck]

@analog-nico
It is actually in console.log(error.response.socket) where the error happens.

The error randomly varies between:
Segmentation fault: 11 and Bus error: 10.

We tried replacing request-promise with only request and couldn't reproduce it.

[analog-nico]

@pameck Ok, using the right code you would be able to reproduce it with request but it is not even caused by request. We are talking here about very low level node.js functionality to send a request which returns the response that points to the socket.

Since your console.log line is the culprit it is not even the request but the way you access the socket by logging it. Why do you log the socket in the first place?

[wiggzz]

We're seeing this manifest because we were logging the error using a logging library. The library we are using happens to do a deep copy of the error object in certain situations, which causes it to crash. Just the simplest way to reproduce it was this console.log.

[analog-nico]

Alright, makes sense @wiggzz .

Then do either:

• Open an issue for your logging library since it obviously would benefit from a more robust deep copy
• Or check out the original Error types that are used by request-promise and manipulate the error objects with e.g. delete error.response before logging them.

Anything else I can help you with?

[wiggzz]

No, thanks! I guess the next step if we want to figure out where the bug is is to dig deeper into the Node request library. But your idea of removing the buggy error property is probably the workaround.

[analog-nico]

Alright.

To verify your assumption that the deep clone of your logging library is the culprit you may use error.response = require('lodash').cloneDeep(error.response) before passing the error to your logging library. This is probably a more robust deep clone.

[wiggzz]

Just out of curiosity, I dug a bit more. It's definitely a Node 4.3 bug. The following will cause a segfault. Don't ask me why there have to be three setImmediates.

'use strict';

const https = require('https');

let options = {
  path: '/',
  method: 'GET',
  hostname: 'google.com',
  port: 443
};

const req = https.request(options, (res) => {
  setImmediate((res) => {
    setImmediate((res) => {
      setImmediate((res) => {
        console.log(res.socket);
      }, res);
    }, res);
  }, res);
});
req.end();

[analog-nico]

Very interesting. Just a wild guess, but maybe because the socket gets closed after a few event loop cycles. Anyways, that's a perfect test case to report to the node.js project since you stripped it down to core functionality only. However, if it does not crash in v4.6.2 they will probably just ask you update.

[wiggzz]

Yea. Would love to update but AWS Lambda is only supporting v4.3.2 and v0.10, so we're kinda stuck on 4.3.2 until AWS decide to upgrade.

[wiggzz]

For future reference, it looks like this is the same issue which is fixed by this commit. Also, in order to patch in that change to 4.3, it seems possible to do something like:

const tls_wrap = process.binding('tls_wrap');
const old_close = tls_wrap.TLSWrap.prototype.close;
tls_wrap.TLSWrap.prototype.close = function(cb) {
  if (this.owner)
    this.owner.ssl = null;

  return old_close.apply(this, cb);
}