Access violation in nodejs when restarting a listening server

[Arno-v]

Nodejs version 0.12.3
Windows 7 and Windows 2012 x64

For our application we need a https or spdy server which can be restarted using a path on the filesystem. On the first call to open() this functions OK, however on the second call it often crashes nodejs. The crash occurs in

https://github.com/joyent/node/blob/master/src/stream_wrap.cc

on line 492. The variable wrap = NULL.

We have tried this with https as well as with spdy.
Code:

function RestartableHttps() {
    var server = null;
    var sockets = [];
    var closing = false;
    var setServer=function(options,hostName) {
        server = spdy.createServer(options, app);
        server.on('connection', function (socket) {
            if (closing) {
                socket.destroy();
                return;
            }
            sockets.push(socket);
            socket.once('close', function () {
                console.log('socket closed');
                sockets.splice(sockets.indexOf(socket), 1);
            });
        });
        closing = false;
        server.listen(443,hostName,function() {
            console.log("Listening on 443");
        });
    }

    var createCertOptions = function (path, type) {
        var keyPath = path + '/Settings/' + type + '.key';
        var certPath = path + '/Settings/' + type + '.crt';
        var caCert = 'Webserver/ca.crt';
        if (!fs.existsSync(keyPath))
            return null;
        if (!fs.existsSync(certPath))
            return null;
        if (!fs.existsSync(caCert))
            return null;
        var options = {
            key: fs.readFileSync(keyPath),
            cert: fs.readFileSync(certPath),
            ca: [fs.readFileSync(caCert)]
        };
        return options;
    }

    var forcedClose = function (cb) {
        if (server === null) {
            cb();
            return;
        }
        closing = true;
        for (var i = 0; i < sockets.length; i++) {
            sockets[i].destroy();
        }
        sockets = [];
        server.close(cb);
    }


    this.open = function (path, certificateType,hostName) {
        var open = function () {
            var options = createCertOptions(path, certificateType);
            if (options === null)
                server = null;
            else
                setServer(options,hostName);
        }
        forcedClose(open);
    }    
}

[jasnell]

@Arno-v ... thanks for the code but it's not clear from the code exactly how to recreate the behavior you're seeing. Can you provide a more complete snippet?

[19h]

Here's an actually readable and usable version of the code:

var fs = require("fs");
var spdy = require("spdy");

var restartableHttps = function () {
    var server = null;
    var sockets = [];
    var closing = false;

    var setServer = function(options,hostName) {
        server = spdy.createServer(options, function (a, b) {
            b.end("Hello world");
        });

        server.on('connection', function (socket) {
            if (closing) {
                socket.destroy();
                return;
            }
            sockets.push(socket);
            socket.once('close', function () {
                console.log('socket closed');
                sockets.splice(sockets.indexOf(socket), 1);
            });
        });
        closing = false;
        server.listen(443,hostName,function() {
            console.log("Listening on 443");
        });
    }

    var createCertOptions = function (path) {
        var keyPath = path + 'key';
        var certPath = path + 'cert';
        var caCert = 'ca';

        if (!fs.existsSync(keyPath))
            return -1;

        if (!fs.existsSync(certPath))
            return -2;

        if (!fs.existsSync(caCert))
            return -3;

        var options = {
            key: fs.readFileSync(keyPath),
            cert: fs.readFileSync(certPath),
            ca: [fs.readFileSync(caCert)]
        };

        return options;
    }

    var forcedClose = function (cb) {
        if (server === null) {
            cb();
            return;
        }
        closing = true;
        for (var i = 0; i < sockets.length; i++) {
            sockets[i].destroy();
        }
        sockets = [];
        server.close(cb);
    }

    this.open = function (path, hostName) {
        var open = function () {
            var options = createCertOptions(path);
            if (options === null || options < 0) {
                server = null;

                console.warn(options);
            } else {
                setServer(options,hostName);
            }
        }

        forcedClose(open);
    }    
};

var x = new restartableHttps()

x.open("./", "127.0.0.1");
x.open("./", "0.0.0.0");

@Arno-v could you please clarify if you're talking about an error in the means of: "Error: listen EADDRINUSE 0.0.0.0:443", or "Segmentation Fault"?

Thanks

[Arno-v]

@KenanSulayman
I am talking about an access violation in Windows which happens if you try to dereference a NULL pointer in C++. It is probably the same as a segmentation fault in UNIX/Linux.

it happens in:

https://github.com/joyent/node/blob/master/src/stream_wrap.cc

on line 492. The variable wrap = NULL.

I modified the code in stream_wrap.cc to so it would test for wrap==NULL, and this would prevent node from crashing, however, the server would not start listening again.

The restartableHttps object is used in a much larger program. It is used because we want to be able to switch to a different configuration in runtime. We want to close the listening server, and restart it with different certificates. The command to switch comes from another connection on localhost.

We use a pattern in which we keep track of the connections, and destroy the connections via socket.destroy if we want to switch the configuration.

I have created a sample program and directories which exhibit the behaviour. where can I upload this zip-file?

[EricSmekens]

https://github.com/EricSmekens/testNodeJS

Here is the sample project. Certificates probably need to updated for your machine name.

[dnakamura]

To be clear, to reproduce, run program and visit http://localhost/SwitchServer
If so, its working fine for me

[EricSmekens]

Addition: In test.js, you want to change 'nlltvriesa01' to your machinename. To reproduce it, open a link to https://$MACHINENAME_HERE$/test.html and then on a second tab/browser.

You will have to bypass the cerfificate security, or make your own.

Navigate to http://localhost/SwitchServer on the other tab. (Switching server while there is a connection seems to be the problem.)
Try again a few times, it does seem to happen randomly.

[EricSmekens]

Issue also reproduceable on Windows 8.1 - Node v0.12.4. Node silently terminates.

[Arno-v]

I finally found the problems. It is part the application, and part node.js not handling a strange situation properly.
By entering these lines in line 413 stream_wrap.cc, node no longer crashes, but generates a javascript exception:

  StreamWrap* wrap = Unwrap<StreamWrap>(args.Holder());
  if (wrap == NULL)
  {
      args.GetReturnValue().Set(EIO);
      return;
  }

This way I found out that a the js application tried to send a response over a destroyed socket, res is being send AFTER the server has been restarted:

var counter=0;
app.get('/TestPoll',function(req,res){
  counter++;
  setTimeout(function(){
      res.send({counter:counter})
      },500);
});

When I look at the rest of the stream_wrap.cc source, it seems to me that the line

  StreamWrap* wrap = Unwrap<StreamWrap>(args.Holder());

Could initialize wrap to NULL on more places.

[dnakamura]

I have also been looking at the problem, and I am able to reproduce on Windows and Linux. It does look like the issue is caused by writing to a socket that has been destroyed. Ideally, I/O operations on sockets should be checking to see if they have been destroyed. At the moment I'm working on a simplified test case to reproduce the issue

[dnakamura]

I think I have this tracked down and it has to do with the way file io is processed by the thread pool. Basically, when the write to the socket occurs, it is still open (and thus the checks all pass). A request is then put in the queue to be processed by a worker in the thread pool. Before the request gets picked up, the socket gets closed, thus when the threadpool worker starts the request it segfaults. Ideally, destroy() should set the state to closed (to prevent further read/writes), and wait for outstanding requests to finish before actually closing the handle and settig it to null. Now its just a matter of figuring out how to make that happen

[dnakamura]

This aught to fix the issue eed9392

[dnakamura]

I was looking at this some more and my previous conclusion was wrong. In the end the problem is that you are closing the underlying socket and the server tries to write to the TLS socket (which shares a handle, but is not notified when the regular socket is destroyed).
The solution is to listen for the secureConnection event rather than the connection event

[jasnell]

@dnakamura ... given your most recent comment, is the fix in eed9392 still necessary?

[Arno-v]

The problem remains that nodejs crashes and does not throw an exception in
the original code. I think the fix is still necessary.

On Thu, Jun 25, 2015 at 10:47 PM, James M Snell [email protected]
wrote:

@dnakamura https://github.com/dnakamura ... given your most recent
comment, is the fix in eed9392
eed9392
still necessary?

—
Reply to this email directly or view it on GitHub
#25365 (comment).

[EricSmekens]

I think so as well, at this moment nodejs silently terminates.

[dnakamura]

@jasnell Strinctly speaking no, eed9392 is not required. However, it is still probably useful to keep so that any future bugs will fail gracefully rather than segfault.

@EricSmekens The ideal solution would be to modify the tls socket code to detect when you have closed the underlying handle. However you do bring up another interesting point, why does node silently terminate rather than printing out the standard segfault info?

[EricSmekens]

@dnakamura For me, (Node 0.12.5), it is not printing any error/info when this occurs.

If I am using 'secureConnection' instead of 'connection', then I will receive an error and it seems that it occurs fewer times. (EADDRINUSE).

[jasnell]

@EricSmekens ... just to cover all the bases, can you check to see if the same issue occurs in the latest io.js?

[dnakamura]

@EricSmekens sounds like you are getting that error because you are trying to bind to the server socket before it has been released. Looking at my code I added a delay, so maybe I was hitting the same problem. Relevant code:

 var forcedClose = function (cb) {
        if (server === null) {
            cb();
            return;
        }
        var delayedCb = function(){setTimeout(cb, 1000));
        closing = true;
        server.close(delayedCb);
        for (var i = 0; i < sockets.length; i++) {
            sockets[i].destroy();
        }
        sockets = [];
    }

Also note, I moved server.close() before the for loop. This stops the server from accepting new connections while you are closing the old ones.

[dnakamura]

Also, for anybody thats interested, the problem arises because while the TLS socket, and the normal socket share the file handle, they maintain little no level of coordination. My initial solution was to add a line to the TLSSocket constructor socket.once('close', function(){self.detroy();});, however this does not work due to some of the oddities of the handle, namely the close() method of the underlying handle which has code equivalent to the following:

function close(cb){
  //do stuff
  this.close = cb;
  cb();
}

Which means that a second call to close() on the same file handle does not behave as expected