From 2cad7a69ce3228b1e40f3bf8117ca739a5d6929d Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 20 May 2013 14:37:55 -0700 Subject: buffer: throw when writing beyond buffer Previously one could write anywhere in a buffer pool if they accidently got their offset wrong. Mainly because the cc level checks only test against the parent slow buffer and not against the js object properties. So now we check to make sure values won't go beyond bounds without letting the dev know. --- lib/buffer.js | 3 +++ 1 file changed, 3 insertions(+) (limited to 'lib') diff --git a/lib/buffer.js b/lib/buffer.js index c75dbc93a8..000c54b3a8 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -339,6 +339,9 @@ Buffer.prototype.write = function(string, offset, length, encoding) { } encoding = String(encoding || 'utf8').toLowerCase(); + if (string.length > 0 && (length < 0 || offset < 0)) + throw new RangeError('attempt to write beyond buffer bounds'); + var ret; switch (encoding) { case 'hex': -- cgit v1.2.3 From f46ad012bc5a40194242ea1e9669c9cc25bd7047 Mon Sep 17 00:00:00 2001 From: Timothy J Fontaine Date: Fri, 17 May 2013 15:04:24 -0700 Subject: timers: internal unref'd timer for api timeouts When an internal api needs a timeout, they should use timers._unrefActive since that won't hold the loop open. This solves the problem where you might have unref'd the socket handle but the timeout for the socket was still active. --- lib/timers.js | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) (limited to 'lib') diff --git a/lib/timers.js b/lib/timers.js index 708f0af16e..076503e601 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -373,3 +373,111 @@ exports.clearImmediate = function(immediate) { process._needImmediateCallback = false; } }; + + +// Internal APIs that need timeouts should use timers._unrefActive isntead of +// timers.active as internal timeouts shouldn't hold the loop open + +var unrefList, unrefTimer; + + +function unrefTimeout() { + var now = Date.now(); + + debug('unrefTimer fired'); + + var first; + while (first = L.peek(unrefList)) { + var diff = now - first._idleStart; + + if (diff < first._idleTimeout) { + diff = first._idleTimeout - diff; + unrefTimer.start(diff, 0); + unrefTimer.when = now + diff; + debug('unrefTimer rescheudling for later'); + return; + } + + L.remove(first); + + var domain = first.domain; + + if (!first._onTimeout) continue; + if (domain && domain._disposed) continue; + + try { + if (domain) domain.enter(); + var threw = true; + debug('unreftimer firing timeout'); + first._onTimeout(); + threw = false; + if (domain) domain.exit(); + } finally { + if (threw) process.nextTick(unrefTimeout); + } + } + + debug('unrefList is empty'); + unrefTimer.when = -1; +} + + +exports._unrefActive = function(item) { + var msecs = item._idleTimeout; + if (!msecs || msecs < 0) return; + assert(msecs >= 0); + + L.remove(item); + + if (!unrefList) { + debug('unrefList initialized'); + unrefList = {}; + L.init(unrefList); + + debug('unrefTimer initialized'); + unrefTimer = new Timer(); + unrefTimer.unref(); + unrefTimer.when = -1; + unrefTimer.ontimeout = unrefTimeout; + } + + var now = Date.now(); + item._idleStart = now; + + if (L.isEmpty(unrefList)) { + debug('unrefList empty'); + L.append(unrefList, item); + + unrefTimer.start(msecs, 0); + unrefTimer.when = now + msecs; + debug('unrefTimer scheduled'); + return; + } + + var when = now + msecs; + + debug('unrefList find where we can insert'); + + var cur, them; + + for (cur = unrefList._idlePrev; cur != unrefList; cur = cur._idlePrev) { + them = cur._idleStart + cur._idleTimeout; + + if (when < them) { + debug('unrefList inserting into middle of list'); + + L.append(cur, item); + + if (unrefTimer.when > when) { + debug('unrefTimer is scheduled to fire too late, reschedule'); + unrefTimer.start(msecs, 0); + unrefTimer.when = when; + } + + return; + } + } + + debug('unrefList append to end'); + L.append(unrefList, item); +}; -- cgit v1.2.3 From a846d9388cbdf298f17b341d0ed669c1852185a8 Mon Sep 17 00:00:00 2001 From: Timothy J Fontaine Date: Fri, 17 May 2013 15:07:28 -0700 Subject: net: use timers._unrefActive for internal timeouts --- lib/net.js | 14 ++++----- test/simple/test-net-socket-timeout-unref.js | 47 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 test/simple/test-net-socket-timeout-unref.js (limited to 'lib') diff --git a/lib/net.js b/lib/net.js index 646983f6ca..7f6f306b84 100644 --- a/lib/net.js +++ b/lib/net.js @@ -307,7 +307,7 @@ Socket.prototype.listen = function() { Socket.prototype.setTimeout = function(msecs, callback) { if (msecs > 0 && !isNaN(msecs) && isFinite(msecs)) { timers.enroll(this, msecs); - timers.active(this); + timers._unrefActive(this); if (callback) { this.once('timeout', callback); } @@ -481,7 +481,7 @@ function onread(buffer, offset, length) { var self = handle.owner; assert(handle === self._handle, 'handle != self._handle'); - timers.active(self); + timers._unrefActive(self); var end = offset + length; debug('onread', process._errno, offset, length, end); @@ -612,7 +612,7 @@ Socket.prototype._write = function(data, encoding, cb) { this._pendingData = null; this._pendingEncoding = ''; - timers.active(this); + timers._unrefActive(this); if (!this._handle) { this._destroy(new Error('This socket is closed.'), cb); @@ -702,7 +702,7 @@ function afterWrite(status, handle, req) { return; } - timers.active(self); + timers._unrefActive(self); if (self !== process.stderr && self !== process.stdout) debug('afterWrite call cb'); @@ -783,7 +783,7 @@ Socket.prototype.connect = function(options, cb) { self.once('connect', cb); } - timers.active(this); + timers._unrefActive(this); self._connecting = true; self.writable = true; @@ -814,7 +814,7 @@ Socket.prototype.connect = function(options, cb) { self._destroy(); }); } else { - timers.active(self); + timers._unrefActive(self); addressType = addressType || 4; @@ -861,7 +861,7 @@ function afterConnect(status, handle, req, readable, writable) { if (status == 0) { self.readable = readable; self.writable = writable; - timers.active(self); + timers._unrefActive(self); self.emit('connect'); diff --git a/test/simple/test-net-socket-timeout-unref.js b/test/simple/test-net-socket-timeout-unref.js new file mode 100644 index 0000000000..32aef444a9 --- /dev/null +++ b/test/simple/test-net-socket-timeout-unref.js @@ -0,0 +1,47 @@ +// 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. + +var common = require('../common'); +var assert = require('assert'); +var net = require('net'); + +var server = net.createServer(function (c) { + c.write('hello'); + c.unref(); +}); +server.listen(common.PORT); +server.unref(); + +var timedout = false; + +[8, 5, 3, 6, 2, 4].forEach(function (T) { + var socket = net.createConnection(common.PORT, 'localhost'); + socket.setTimeout(T * 1000, function () { + console.log(process._getActiveHandles()); + timedout = true; + socket.destroy(); + }); + socket.unref(); +}); + +process.on('exit', function () { + assert.strictEqual(timedout, false, 'Socket timeout should not hold loop open'); +}); -- cgit v1.2.3 From fda2b319dc8a5dae528f8b12096911b71a0282db Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 May 2013 21:43:35 +0200 Subject: http: save roundtrips, convert buffers to strings This commit adds an optimization to the HTTP client that makes it possible to: * Pack the headers and the first chunk of the request body into a single write(). * Pack the chunk header and the chunk itself into a single write(). Because only one write() system call is issued instead of several, the chances of data ending up in a single TCP packet are phenomenally higher: the benchmark with `type=buf size=32` jumps from 50 req/s to 7,500 req/s, a 150-fold increase. This commit removes the check from e4b716ef that pushes binary encoded strings into the slow path. The commit log mentions that: We were assuming that any string can be concatenated safely to CRLF. However, for hex, base64, or binary encoded writes, this is not the case, and results in sending the incorrect response. For hex and base64 strings that's certainly true but binary strings are 'das Ding an sich': string.length is the same before and after decoding. Fixes #5528. --- benchmark/http/client-request-body.js | 69 +++++++++++++++++++++++++++++++++++ lib/http.js | 20 +++++++++- 2 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 benchmark/http/client-request-body.js (limited to 'lib') diff --git a/benchmark/http/client-request-body.js b/benchmark/http/client-request-body.js new file mode 100644 index 0000000000..7a3468a670 --- /dev/null +++ b/benchmark/http/client-request-body.js @@ -0,0 +1,69 @@ +// Measure the time it takes for the HTTP client to send a request body. + +var common = require('../common.js'); +var http = require('http'); + +var bench = common.createBenchmark(main, { + dur: [5], + type: ['asc', 'utf', 'buf'], + bytes: [32, 256, 1024], + method: ['write', 'end '] // two spaces added to line up each row +}); + +function main(conf) { + var dur = +conf.dur; + var len = +conf.bytes; + + var encoding; + var chunk; + switch (conf.type) { + case 'buf': + chunk = new Buffer(len); + chunk.fill('x'); + break; + case 'utf': + encoding = 'utf8'; + chunk = new Array(len / 2 + 1).join('ΓΌ'); + break; + case 'asc': + chunk = new Array(len + 1).join('a'); + break; + } + + var nreqs = 0; + var options = { + headers: { 'Connection': 'keep-alive', 'Transfer-Encoding': 'chunked' }, + agent: new http.Agent({ maxSockets: 1 }), + host: '127.0.0.1', + port: common.PORT, + path: '/', + method: 'POST' + }; + + var server = http.createServer(function(req, res) { + res.end(); + }); + server.listen(options.port, options.host, function() { + setTimeout(done, dur * 1000); + bench.start(); + pummel(); + }); + + function pummel() { + var req = http.request(options, function(res) { + nreqs++; + pummel(); // Line up next request. + res.resume(); + }); + if (conf.method === 'write') { + req.write(chunk, encoding); + req.end(); + } else { + req.end(chunk, encoding); + } + } + + function done() { + bench.end(nreqs); + } +} diff --git a/lib/http.js b/lib/http.js index 4a4fe94f3e..bcde516dae 100644 --- a/lib/http.js +++ b/lib/http.js @@ -782,12 +782,28 @@ OutgoingMessage.prototype.write = function(chunk, encoding) { if (chunk.length === 0) return false; + // TODO(bnoordhuis) Temporary optimization hack, remove in v0.11. We only + // want to convert the buffer when we're sending: + // + // a) Transfer-Encoding chunks, because it lets us pack the chunk header + // and the chunk into a single write(), or + // + // b) the first chunk of a fixed-length request, because it lets us pack + // the request headers and the chunk into a single write(). + // + // Converting to strings is expensive, CPU-wise, but reducing the number + // of write() calls more than makes up for that because we're dramatically + // reducing the number of TCP roundtrips. + if (chunk instanceof Buffer && (this.chunkedEncoding || !this._headerSent)) { + chunk = chunk.toString('binary'); + encoding = 'binary'; + } + var len, ret; if (this.chunkedEncoding) { if (typeof(chunk) === 'string' && encoding !== 'hex' && - encoding !== 'base64' && - encoding !== 'binary') { + encoding !== 'base64') { len = Buffer.byteLength(chunk, encoding); chunk = len.toString(16) + CRLF + chunk + CRLF; ret = this._send(chunk, encoding); -- cgit v1.2.3 From a2f93cf77a8121db713bb003e26031bcca446f1f Mon Sep 17 00:00:00 2001 From: isaacs Date: Thu, 23 May 2013 15:12:13 -0700 Subject: http: Return true on empty writes, not false Otherwise, writing an empty string causes the whole program to grind to a halt when piping data into http messages. This wasn't as much of a problem (though it WAS a bug) in 0.8 and before, because our hyperactive 'drain' behavior would mean that some *previous* write() would probably have a pending drain event, and cause things to start moving again. --- lib/http.js | 4 +- test/simple/test-http-zero-length-write.js | 93 ++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 test/simple/test-http-zero-length-write.js (limited to 'lib') diff --git a/lib/http.js b/lib/http.js index bcde516dae..ab36cd7491 100644 --- a/lib/http.js +++ b/lib/http.js @@ -780,7 +780,9 @@ OutgoingMessage.prototype.write = function(chunk, encoding) { throw new TypeError('first argument must be a string or Buffer'); } - if (chunk.length === 0) return false; + // If we get an empty string or buffer, then just do nothing, and + // signal the user to keep writing. + if (chunk.length === 0) return true; // TODO(bnoordhuis) Temporary optimization hack, remove in v0.11. We only // want to convert the buffer when we're sending: diff --git a/test/simple/test-http-zero-length-write.js b/test/simple/test-http-zero-length-write.js new file mode 100644 index 0000000000..e68f947c32 --- /dev/null +++ b/test/simple/test-http-zero-length-write.js @@ -0,0 +1,93 @@ +// 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. + +var common = require('../common'); +var assert = require('assert'); + +var http = require('http'); + +var Stream = require('stream'); + +function getSrc() { + // An old-style readable stream. + // The Readable class prevents this behavior. + var src = new Stream(); + + // start out paused, just so we don't miss anything yet. + var paused = false; + src.pause = function() { + paused = true; + }; + src.resume = function() { + paused = false; + }; + + var chunks = [ '', 'asdf', '', 'foo', '', 'bar', '' ]; + var interval = setInterval(function() { + if (paused) + return + + var chunk = chunks.shift(); + if (chunk !== undefined) { + src.emit('data', chunk); + } else { + src.emit('end'); + clearInterval(interval); + } + }, 1); + + return src; +} + + +var expect = 'asdffoobar'; + +var server = http.createServer(function(req, res) { + var actual = ''; + req.setEncoding('utf8'); + req.on('data', function(c) { + actual += c; + }); + req.on('end', function() { + assert.equal(actual, expect); + getSrc().pipe(res); + }); + server.close(); +}); + +server.listen(common.PORT, function() { + var req = http.request({ port: common.PORT, method: 'POST' }); + var actual = ''; + req.on('response', function(res) { + res.setEncoding('utf8'); + res.on('data', function(c) { + actual += c; + }); + res.on('end', function() { + assert.equal(actual, expect); + }); + }); + getSrc().pipe(req); +}); + +process.on('exit', function(c) { + if (!c) console.log('ok'); +}); -- cgit v1.2.3 From a40133d10cdb911b27fe8d46d67a835b0103bbf1 Mon Sep 17 00:00:00 2001 From: Nathan Zadoks Date: Fri, 24 May 2013 19:34:38 +0200 Subject: http: remove bodyHead from 'upgrade' events Streams2 makes this unnecessary. An empty buffer is provided for compatibility. --- doc/api/http.markdown | 20 ++++++++------------ lib/http.js | 10 ++++++++-- test/simple/test-http-upgrade-agent.js | 4 +++- test/simple/test-http-upgrade-client.js | 4 +++- test/simple/test-http-upgrade-client2.js | 5 +++++ test/simple/test-http-upgrade-server.js | 2 +- 6 files changed, 28 insertions(+), 17 deletions(-) (limited to 'lib') diff --git a/doc/api/http.markdown b/doc/api/http.markdown index e93aa2d2dd..7204690e34 100644 --- a/doc/api/http.markdown +++ b/doc/api/http.markdown @@ -93,7 +93,7 @@ not be emitted. ### Event: 'connect' -`function (request, socket, head) { }` +`function (request, socket) { }` Emitted each time a client requests a http CONNECT method. If this event isn't listened for, then clients requesting a CONNECT method will have their @@ -102,8 +102,6 @@ connections closed. * `request` is the arguments for the http request, as it is in the request event. * `socket` is the network socket between the server and client. -* `head` is an instance of Buffer, the first packet of the tunneling stream, - this may be empty. After this event is emitted, the request's socket will not have a `data` event listener, meaning you will need to bind to it in order to handle data @@ -111,7 +109,7 @@ sent to the server on that socket. ### Event: 'upgrade' -`function (request, socket, head) { }` +`function (request, socket) { }` Emitted each time a client requests a http upgrade. If this event isn't listened for, then clients requesting an upgrade will have their connections @@ -120,7 +118,6 @@ closed. * `request` is the arguments for the http request, as it is in the request event. * `socket` is the network socket between the server and client. -* `head` is an instance of Buffer, the first packet of the upgraded stream, this may be empty. After this event is emitted, the request's socket will not have a `data` @@ -595,7 +592,7 @@ Emitted after a socket is assigned to this request. ### Event: 'connect' -`function (response, socket, head) { }` +`function (response, socket) { }` Emitted each time a server responds to a request with a CONNECT method. If this event isn't being listened for, clients receiving a CONNECT method will have @@ -612,14 +609,13 @@ A client server pair that show you how to listen for the `connect` event. res.writeHead(200, {'Content-Type': 'text/plain'}); res.end('okay'); }); - proxy.on('connect', function(req, cltSocket, head) { + proxy.on('connect', function(req, cltSocket) { // connect to an origin server var srvUrl = url.parse('http://' + req.url); var srvSocket = net.connect(srvUrl.port, srvUrl.hostname, function() { cltSocket.write('HTTP/1.1 200 Connection Established\r\n' + 'Proxy-agent: Node-Proxy\r\n' + '\r\n'); - srvSocket.write(head); srvSocket.pipe(cltSocket); cltSocket.pipe(srvSocket); }); @@ -639,7 +635,7 @@ A client server pair that show you how to listen for the `connect` event. var req = http.request(options); req.end(); - req.on('connect', function(res, socket, head) { + req.on('connect', function(res, socket) { console.log('got connected!'); // make a request over an HTTP tunnel @@ -658,7 +654,7 @@ A client server pair that show you how to listen for the `connect` event. ### Event: 'upgrade' -`function (response, socket, head) { }` +`function (response, socket) { }` Emitted each time a server responds to a request with an upgrade. If this event isn't being listened for, clients receiving an upgrade header will have @@ -673,7 +669,7 @@ A client server pair that show you how to listen for the `upgrade` event. res.writeHead(200, {'Content-Type': 'text/plain'}); res.end('okay'); }); - srv.on('upgrade', function(req, socket, head) { + srv.on('upgrade', function(req, socket) { socket.write('HTTP/1.1 101 Web Socket Protocol Handshake\r\n' + 'Upgrade: WebSocket\r\n' + 'Connection: Upgrade\r\n' + @@ -698,7 +694,7 @@ A client server pair that show you how to listen for the `upgrade` event. var req = http.request(options); req.end(); - req.on('upgrade', function(res, socket, upgradeHead) { + req.on('upgrade', function(res, socket) { console.log('got upgraded!'); socket.end(); process.exit(0); diff --git a/lib/http.js b/lib/http.js index ab36cd7491..3c3a56c50b 100644 --- a/lib/http.js +++ b/lib/http.js @@ -28,6 +28,9 @@ var FreeList = require('freelist').FreeList; var HTTPParser = process.binding('http_parser').HTTPParser; var assert = require('assert').ok; +// an empty buffer for UPGRADE/CONNECT bodyHead compatibility +var emptyBuffer = new Buffer(0); + var debug; if (process.env.NODE_DEBUG && /http/.test(process.env.NODE_DEBUG)) { debug = function(x) { console.error('HTTP: %s', x); }; @@ -1579,7 +1582,9 @@ function socketOnData(d, start, end) { socket.removeListener('close', socketCloseListener); socket.removeListener('error', socketErrorListener); - req.emit(eventName, res, socket, bodyHead); + socket.unshift(bodyHead); + + req.emit(eventName, res, socket, emptyBuffer); req.emit('close'); } else { // Got Upgrade header or CONNECT method, but have no handler. @@ -1952,7 +1957,8 @@ function connectionListener(socket) { var eventName = req.method === 'CONNECT' ? 'connect' : 'upgrade'; if (EventEmitter.listenerCount(self, eventName) > 0) { - self.emit(eventName, req, req.socket, bodyHead); + socket.unshift(bodyHead); + self.emit(eventName, req, req.socket, emptyBuffer); } else { // Got upgrade header or CONNECT method, but have no handler. socket.destroy(); diff --git a/test/simple/test-http-upgrade-agent.js b/test/simple/test-http-upgrade-agent.js index 1077a983dc..ff8e540556 100644 --- a/test/simple/test-http-upgrade-agent.js +++ b/test/simple/test-http-upgrade-agent.js @@ -67,7 +67,7 @@ srv.listen(common.PORT, '127.0.0.1', function() { req.on('upgrade', function(res, socket, upgradeHead) { // XXX: This test isn't fantastic, as it assumes that the entire response // from the server will arrive in a single data callback - assert.equal(upgradeHead, 'nurtzo'); + assert.equal(upgradeHead, ''); console.log(res.headers); var expectedHeaders = { 'hello': 'world', @@ -78,6 +78,8 @@ srv.listen(common.PORT, '127.0.0.1', function() { // Make sure this request got removed from the pool. assert(!http.globalAgent.sockets.hasOwnProperty(name)); + assert.equal(socket.read(), 'nurtzo'); + req.on('close', function() { socket.end(); srv.close(); diff --git a/test/simple/test-http-upgrade-client.js b/test/simple/test-http-upgrade-client.js index 3bf5beccf5..9361e0b503 100644 --- a/test/simple/test-http-upgrade-client.js +++ b/test/simple/test-http-upgrade-client.js @@ -56,7 +56,7 @@ srv.listen(common.PORT, '127.0.0.1', function() { req.on('upgrade', function(res, socket, upgradeHead) { // XXX: This test isn't fantastic, as it assumes that the entire response // from the server will arrive in a single data callback - assert.equal(upgradeHead, 'nurtzo'); + assert.equal(upgradeHead, ''); console.log(res.headers); var expectedHeaders = {'hello': 'world', @@ -64,6 +64,8 @@ srv.listen(common.PORT, '127.0.0.1', function() { 'upgrade': 'websocket' }; assert.deepEqual(expectedHeaders, res.headers); + assert.equal(socket.read(), 'nurtzo'); + socket.end(); srv.close(); diff --git a/test/simple/test-http-upgrade-client2.js b/test/simple/test-http-upgrade-client2.js index fa39f2a572..53f15d2939 100644 --- a/test/simple/test-http-upgrade-client2.js +++ b/test/simple/test-http-upgrade-client2.js @@ -30,6 +30,9 @@ server.on('upgrade', function(req, socket, head) { socket.write('HTTP/1.1 101 Ok' + CRLF + 'Connection: Upgrade' + CRLF + 'Upgrade: Test' + CRLF + CRLF + 'head'); + socket.on('readable', function() { + socket.read(); + }); socket.on('end', function() { socket.end(); }); @@ -50,6 +53,7 @@ server.listen(common.PORT, function() { wasUpgrade = true; request.removeListener('upgrade', onUpgrade); + socket.unref(); socket.end(); } request.on('upgrade', onUpgrade); @@ -75,6 +79,7 @@ server.listen(common.PORT, function() { successCount++; // Test pass console.log('Pass!'); + server.unref(); server.close(); }); }); diff --git a/test/simple/test-http-upgrade-server.js b/test/simple/test-http-upgrade-server.js index 6a14d39aa5..88b83ca85e 100644 --- a/test/simple/test-http-upgrade-server.js +++ b/test/simple/test-http-upgrade-server.js @@ -55,7 +55,7 @@ function testServer() { 'Connection: Upgrade\r\n' + '\r\n\r\n'); - request_upgradeHead = upgradeHead; + request_upgradeHead = socket.read(); socket.ondata = function(d, start, end) { var data = d.toString('utf8', start, end); -- cgit v1.2.3 From f7ff8b4454513557ca8854cb1bf8a3539946fd11 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 25 May 2013 01:02:34 +0400 Subject: tls: retry writing after hello parse error When writing bad data to EncryptedStream it'll first get to the ClientHello parser, and, only after it will refuse it, to the OpenSSL. But ClientHello parser has limited buffer and therefore write could return `bytes_written` < `incoming_bytes`, which is not the case when working with OpenSSL. After such errors ClientHello parser disables itself and will pass-through all data to the OpenSSL. So just trying to write data one more time will throw the rest into OpenSSL and let it handle it. --- lib/tls.js | 9 ++++- test/simple/test-tls-hello-parser-failure.js | 50 ++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 test/simple/test-tls-hello-parser-failure.js (limited to 'lib') diff --git a/lib/tls.js b/lib/tls.js index 1ff0d5d679..7bf0ca1886 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -253,6 +253,7 @@ function CryptoStream(pair, options) { this._pendingEncoding = ''; this._pendingCallback = null; this._doneFlag = false; + this._retryAfterPartial = false; this._resumingSession = false; this._reading = true; this._destroyed = false; @@ -361,7 +362,13 @@ CryptoStream.prototype._write = function write(data, encoding, cb) { return cb(null); } - assert(written === 0 || written === -1); + if (written !== 0 && written !== -1) { + assert(!this._retryAfterPartial); + this._retryAfterPartial = true; + this._write(data.slice(written), encoding, cb); + this._retryAfterPartial = false; + return; + } } else { debug('cleartext.write queue is full'); diff --git a/test/simple/test-tls-hello-parser-failure.js b/test/simple/test-tls-hello-parser-failure.js new file mode 100644 index 0000000000..f7e1f741fa --- /dev/null +++ b/test/simple/test-tls-hello-parser-failure.js @@ -0,0 +1,50 @@ +// 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. + +var common = require('../common'); +var tls = require('tls'); +var net = require('net'); +var fs = require('fs'); +var assert = require('assert'); + +var options = { + key: fs.readFileSync(common.fixturesDir + '/test_key.pem'), + cert: fs.readFileSync(common.fixturesDir + '/test_cert.pem') +}; + +var server = tls.createServer(options, function(c) { + +}).listen(common.PORT, function() { + var client = net.connect(common.PORT, function() { + var bonkers = new Buffer(1024 * 1024); + bonkers.fill(42); + client.end(bonkers); + }); + + var once = false; + client.on('error', function() { + if (!once) { + once = true; + client.destroy(); + server.close(); + } + }); +}); -- cgit v1.2.3