From 4f14221f03516b30ea4fba3117f7f9e169ac82da Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Mon, 27 May 2013 14:44:33 +0400 Subject: [PATCH] tls: invoke write cb only after opposite read end Stream's `._write()` callback should be invoked only after it's opposite stream has finished processing incoming data, otherwise `finish` event fires too early and connection might be closed while there's some data to send to the client. see #5544 --- lib/tls.js | 96 +++++++++++++++++++++-------- test/simple/test-tls-client-destroy-soon.js | 75 ++++++++++++++++++++++ 2 files changed, 144 insertions(+), 27 deletions(-) create mode 100644 test/simple/test-tls-client-destroy-soon.js diff --git a/lib/tls.js b/lib/tls.js index 4441bd1..5335a3a 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -254,6 +254,8 @@ function CryptoStream(pair, options) { this._pendingCallback = null; this._doneFlag = false; this._retryAfterPartial = false; + this._halfRead = false; + this._sslOutCb = null; this._resumingSession = false; this._reading = true; this._destroyed = false; @@ -319,6 +321,19 @@ function onCryptoStreamEnd() { } +// NOTE: Called once `this._opposite` is set. +CryptoStream.prototype.init = function init() { + var self = this; + this._opposite.on('sslOutEnd', function() { + if (self._sslOutCb) { + var cb = self._sslOutCb; + self._sslOutCb = null; + cb(null); + } + }); +}; + + CryptoStream.prototype._write = function write(data, encoding, cb) { assert(this._pending === null); @@ -347,28 +362,36 @@ CryptoStream.prototype._write = function write(data, encoding, cb) { return cb(this.pair.error(true)); } + // Whole buffer was written + if (written === data.length) { + if (this === this.pair.cleartext) { + debug('cleartext.write succeed with ' + written + ' bytes'); + } else { + debug('encrypted.write succeed with ' + written + ' bytes'); + } + + // Invoke callback only when all data read from opposite stream + if (this._opposite._halfRead) { + assert(this._sslOutCb === null); + this._sslOutCb = cb; + } else { + cb(null); + } + } + // Force SSL_read call to cycle some states/data inside OpenSSL this.pair.cleartext.read(0); // Cycle encrypted data - if (this.pair.encrypted._internallyPendingBytes()) { + if (this.pair.encrypted._internallyPendingBytes()) this.pair.encrypted.read(0); - } // Get NPN and Server name when ready this.pair.maybeInitFinished(); - // Whole buffer was written if (written === data.length) { - if (this === this.pair.cleartext) { - debug('cleartext.write succeed with ' + data.length + ' bytes'); - } else { - debug('encrypted.write succeed with ' + data.length + ' bytes'); - } - - return cb(null); - } - if (written !== 0 && written !== -1) { + return; + } else if (written !== 0 && written !== -1) { assert(!this._retryAfterPartial); this._retryAfterPartial = true; this._write(data.slice(written), encoding, cb); @@ -470,25 +493,42 @@ CryptoStream.prototype._read = function read(size) { this._opposite._done(); // EOF - return this.push(null); + this.push(null); + } else { + // Bail out + this.push(''); } - - // Bail out - return this.push(''); + } else { + // Give them requested data + if (this.ondata) { + var self = this; + this.ondata(pool, start, start + bytesRead); + + // Consume data automatically + // simple/test-https-drain fails without it + process.nextTick(function() { + self.read(bytesRead); + }); + } + this.push(pool.slice(start, start + bytesRead)); } - // Give them requested data - if (this.ondata) { - var self = this; - this.ondata(pool, start, start + bytesRead); + // Let users know that we've some internal data to read + var halfRead = this._internallyPendingBytes() !== 0; - // Consume data automatically - // simple/test-https-drain fails without it - process.nextTick(function() { - self.read(bytesRead); - }); + // Smart check to avoid invoking 'sslOutEnd' in the most of the cases + if (this._halfRead !== halfRead) { + this._halfRead = halfRead; + + // Notify listeners about internal data end + if (this === this.pair.cleartext) { + debug('cleartext.sslOutEnd'); + } else { + debug('encrypted.sslOutEnd'); + } + + this.emit('sslOutEnd'); } - return this.push(pool.slice(start, start + bytesRead)); }; @@ -600,7 +640,7 @@ CryptoStream.prototype.destroySoon = function(err) { if (this.writable) this.end(); - if (this._writableState.finished) + if (this._writableState.finished && this._opposite._ended) this.destroy(); else this.once('finish', this.destroy); @@ -870,6 +910,8 @@ function SecurePair(credentials, isServer, requestCert, rejectUnauthorized, /* Let streams know about each other */ this.cleartext._opposite = this.encrypted; this.encrypted._opposite = this.cleartext; + this.cleartext.init(); + this.encrypted.init(); process.nextTick(function() { /* The Connection may be destroyed by an abort call */ diff --git a/test/simple/test-tls-client-destroy-soon.js b/test/simple/test-tls-client-destroy-soon.js new file mode 100644 index 0000000..529b84a --- /dev/null +++ b/test/simple/test-tls-client-destroy-soon.js @@ -0,0 +1,75 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +// Create an ssl server. First connection, validate that not resume. +// Cache session and close connection. Use session on second connection. +// ASSERT resumption. + +if (!process.versions.openssl) { + console.error('Skipping because node compiled without OpenSSL.'); + process.exit(0); +} + +var common = require('../common'); +var assert = require('assert'); +var tls = require('tls'); +var fs = require('fs'); + +var options = { + key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'), + cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem') +}; + +var big = new Buffer(2 * 1024 * 1024); +var connections = 0; +var bytesRead = 0; + +big.fill('Y'); + +// create server +var server = tls.createServer(options, function(socket) { + socket.end(big); + socket.destroySoon(); + connections++; +}); + +// start listening +server.listen(common.PORT, function() { + var client = tls.connect({ + port: common.PORT, + rejectUnauthorized: false + }, function() { + client.on('readable', function() { + var d = client.read(); + if (d) + bytesRead += d.length; + }); + + client.on('end', function() { + server.close(); + }); + }); +}); + +process.on('exit', function() { + assert.equal(1, connections); + assert.equal(big.length, bytesRead); +}); -- 2.7.4