_stream_wrap: prevent use after free in TLS

Queued write requests should be invoked on handle close, otherwise the
"consumer" might be already destroyed when the write callbacks of the
"consumed" handle will be invoked.

Fix: nodejs#1696

Author: indutny

lib/_stream_wrap.js

@@ -10,9 +10,11 @@ function StreamWrap(stream) {
 
   this.stream = stream;
 
+  this.queue = null;
+
   var self = this;
   handle.close = function(cb) {
-    cb();
+    self.close(cb);
   };
   handle.isAlive = function() {
     return self.isAlive();
@@ -35,10 +37,12 @@ function StreamWrap(stream) {
 
   this.stream.pause();
   this.stream.on('data', function(chunk) {
-    self._handle.readBuffer(chunk);
+    if (self._handle)
+      self._handle.readBuffer(chunk);
   });
   this.stream.once('end', function() {
-    self._handle.emitEOF();
+    if (self._handle)
+      self._handle.emitEOF();
   });
   this.stream.on('error', function(err) {
     self.emit('error', err);
@@ -88,6 +92,9 @@ StreamWrap.prototype.write = function write(req, bufs) {
   var pending = bufs.length;
   var self = this;
 
+  // Queue the request to be able to cancel it
+  self.enqueue(req);
+
   self.stream.cork();
   bufs.forEach(function(buf) {
     self.stream.write(buf, done);
@@ -103,6 +110,10 @@ StreamWrap.prototype.write = function write(req, bufs) {
 
     // Ensure that write was dispatched
     setImmediate(function() {
+      // Do not invoke callback twice
+      if (!self.dequeue(req))
+        return;
+
       var errCode = 0;
       if (err) {
         if (err.code && uv['UV_' + err.code])
@@ -118,3 +129,58 @@ StreamWrap.prototype.write = function write(req, bufs) {
 
   return 0;
 };
+
+StreamWrap.prototype.enqueue = function enqueue(req) {
+  if (this.queue === null) {
+    this.queue = req;
+    req._prev = req;
+    req._next = req;
+    return;
+  }
+
+  req._next = this.queue._next;
+  this.queue._next._prev = req;
+  req._prev = this.queue;
+  this.queue._next = req;
+};
+
+StreamWrap.prototype.dequeue = function dequeue(req) {
+  var next = req._next;
+  var prev = req._prev;
+
+  if (next === null && prev === null)
+    return false;
+
+  req._next = null;
+  req._prev = null;
+
+  if (next === prev) {
+    this.queue = null;
+  } else {
+    prev._next = next;
+    next._prev = prev;
+
+    if (this.queue === req)
+      this.queue = next;
+  }
+
+  return true;
+};
+
+StreamWrap.prototype.close = function close(cb) {
+  var self = this;
+
+  setImmediate(function() {
+    while (self.queue !== null) {
+      var req = self.queue;
+      self.dequeue(req);
+
+      var errCode = uv.UV_ECANCELED;
+      self._handle.doAfterWrite(req);
+      self._handle.finishWrite(req, errCode);
+    }
+
+    self._handle = null;
+    cb();
+  });
+};

src/js_stream.cc

@@ -163,7 +163,7 @@ template <class Wrap>
 void JSStream::Finish(const FunctionCallbackInfo<Value>& args) {
   Wrap* w = Unwrap<Wrap>(args[0].As<Object>());
 
-  w->Done(args[0]->Int32Value());
+  w->Done(args[1]->Int32Value());
 }
 
 

src/tls_wrap.cc

@@ -320,6 +320,10 @@ void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) {
   TLSWrap* wrap = req_wrap->wrap()->Cast<TLSWrap>();
   req_wrap->Dispose();
 
+  // We should not be getting here after `DestroySSL`, because all queued writes
+  // must be invoked with UV_ECANCELED
+  CHECK_NE(wrap->ssl_, nullptr);
+
   // Handle error
   if (status) {
     // Ignore errors after shutdown
@@ -331,9 +335,6 @@ void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) {
     return;
   }
 
-  if (wrap->ssl_ == nullptr)
-    return;
-
   // Commit
   NodeBIO::FromBIO(wrap->enc_out_)->Read(nullptr, wrap->write_size_);
 

test/parallel/test-tls-destroy-whilst-write.js

@@ -0,0 +1,29 @@
+'use strict';
+var assert = require('assert');
+var common = require('../common');
+
+if (!common.hasCrypto) {
+  console.log('1..0 # Skipped: missing crypto');
+  process.exit();
+}
+var tls = require('tls');
+var stream = require('stream');
+
+var delay = new stream.Duplex({
+  read: function read() {
+  },
+  write: function write(data, enc, cb) {
+    console.log('pending');
+    setTimeout(function() {
+      console.log('done');
+      cb();
+    }, 200);
+  }
+});
+
+var secure = tls.connect({
+  socket: delay
+});
+setImmediate(function() {
+  secure.destroy();
+});