Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

stream_wrap: Fix issue with null file handles #25535

Closed
wants to merge 2 commits into from

Conversation

dnakamura
Copy link

Fixes #25365. Essentially, a file I/O request is generated and queued, in a different thread the stream is closed and handle set to null. When a worker thread then tries to execute the request it gets a null handle

@jasnell
Copy link
Member

jasnell commented Jun 26, 2015

@trevnorris @cjihrig ... can either of you take a quick look at this as well?

@jasnell
Copy link
Member

jasnell commented Jun 26, 2015

@mhdawson @dnakamura ... we'd likely want to target this PR against v0.12 rather than master.
@dnakamura ... when you get a chance, would you be able to check v0.10 and the latest io.js as well? We may want to backport this fix to v0.10 and get it landed in the converged repo as well.

@@ -224,6 +224,10 @@ void StreamWrap::WriteBuffer(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args.GetIsolate());

StreamWrap* wrap = Unwrap<StreamWrap>(args.Holder());
if ( NULL == wrap) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the space between ( and NULL in all of your if statements. I think core normally writes as wrap == NULL as well.

@mhdawson
Copy link
Member

mhdawson commented Jul 2, 2015

In putting together a test case we discovered a few other problems as well. Devin is working on capturing the test cases and looking at alternate changes to see if what was in the pull request still makes sense or something higher up would make more sense

@dnakamura
Copy link
Author

I gave some explanation to the underlying problem in #25365. Code for reproducing, which is run in the root directory of https://github.com/EricSmekens/testNodeJS (which is just used for server certs)

client:

var net = require('net');
var tls = require('tls');
var sock = tls.connect(1234, {rejectUnauthorized: false});
sock.pipe(process.stdout);

server:

var tls = require('tls');
var path = require('path');
var fs = require('fs');

var createCertOptions = function (pth, type) {
        var keyPath = path.join(pth, 'Settings', type + '.key');
        var certPath = path.join(pth , 'Settings' , type + '.crt');
        var caCert = path.join('Webserver', 'ca.crt');
        if (!fs.existsSync(keyPath)){
            console.warn("no keypath ", keyPath);
            return null;
        }
        if (!fs.existsSync(certPath)){
            console.warn("no cert path");
            return null;
        }
        if (!fs.existsSync(caCert)){
            console.warn("no cacert");
            return null;
        }
        var options = {
            key: fs.readFileSync(keyPath),
            cert: fs.readFileSync(certPath),
            ca: [fs.readFileSync(caCert)]
        };
        return options;
}


var regSocket = null;
var dummyFunction = function(){};

function secureConnectionListener(secureSock){
    secureSock.on('close', function(){console.log("Tlssocket close event")});
    setTimeout(function(){
        secureSock.write("HELLO\n");
        regSocket.destroy();
        setTimeout(function(){
            secureSock.write("Never received");
        }, 100);
    }, 100);
}

function connectionListener(sock){
    regSocket = sock;
    sock.on('close', function(){console.log("Regular socket close event")});
}

var server = tls.createServer(createCertOptions('./certs', 'primary'));
server.on('connection', connectionListener);
server.on('secureConnection', secureConnectionListener);
server.listen(1234);

Also note that closing the secure socket after closing the normal socket results in unexpected behaviour.

function secureConnectionListener(secureSock){
    secureSock.on('close', function(){console.log("Tlssocket close event")});
    setTimeout(function(){
        secureSock.write("HELLO\n");
        regSocket.destroy();
        secureSock.destroy();
    }, 100);
}

gives the output:

node server.js
Regular socket close event
Regular socket close event

Changing the order (closing the secure socket before the regular socket) results in the close event on the secure socket being emitted twice

@jasnell jasnell added the fs label Aug 27, 2015
@dnakamura dnakamura closed this Aug 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants