From faafbe7ff7b0aa34201eecd2b51532e1b4f7c4ff Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 16 Dec 2014 17:17:28 -0500 Subject: [PATCH 1/7] net: throw on invalid socket timeouts This commit restricts socket timeouts non-negative, finite numbers. Any other value throws a TypeError or RangeError. This prevents subtle bugs that can happen due to type coercion. Fixes: https://github.com/joyent/node/issues/8618 PR-URL: https://github.com/joyent/node/pull/8884 Reviewed-By: Trevor Norris Reviewed-By: Timothy J Fontaine Conflicts: lib/timers.js test/simple/test-net-settimeout.js test/simple/test-net-socket-timeout.js --- lib/net.js | 12 ++++++------ lib/timers.js | 18 +++++++++++------ test/parallel/test-net-settimeout.js | 2 +- test/parallel/test-net-socket-timeout.js | 25 ++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 13 deletions(-) diff --git a/lib/net.js b/lib/net.js index 030083d3f40983..294c7f99ee6a7e 100644 --- a/lib/net.js +++ b/lib/net.js @@ -300,17 +300,17 @@ Socket.prototype.listen = function() { Socket.prototype.setTimeout = function(msecs, callback) { - if (msecs > 0 && isFinite(msecs)) { + if (msecs === 0) { + timers.unenroll(this); + if (callback) { + this.removeListener('timeout', callback); + } + } else { timers.enroll(this, msecs); timers._unrefActive(this); if (callback) { this.once('timeout', callback); } - } else if (msecs === 0) { - timers.unenroll(this); - if (callback) { - this.removeListener('timeout', callback); - } } }; diff --git a/lib/timers.js b/lib/timers.js index 4dc483ce8be1a8..e3e64287509a30 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -3,15 +3,13 @@ const Timer = process.binding('timer_wrap').Timer; const L = require('_linklist'); const assert = require('assert').ok; - +const util = require('util'); +const debug = util.debuglog('timer'); const kOnTimeout = Timer.kOnTimeout | 0; // Timeout values > TIMEOUT_MAX are set to 1. const TIMEOUT_MAX = 2147483647; // 2^31-1 -const debug = require('util').debuglog('timer'); - - // IDLE TIMEOUTS // // Because often many sockets will have the same idle timeout we will not @@ -132,13 +130,21 @@ const unenroll = exports.unenroll = function(item) { // Does not start the time, just sets up the members needed. exports.enroll = function(item, msecs) { + if (typeof msecs !== 'number') { + throw new TypeError('msecs must be a number'); + } + + if (msecs < 0 || !isFinite(msecs)) { + throw new RangeError('msecs must be a non-negative finite number'); + } + // if this item was already in a list somewhere // then we should unenroll it from that if (item._idleNext) unenroll(item); // Ensure that msecs fits into signed int32 - if (msecs > 0x7fffffff) { - msecs = 0x7fffffff; + if (msecs > TIMEOUT_MAX) { + msecs = TIMEOUT_MAX; } item._idleTimeout = msecs; diff --git a/test/parallel/test-net-settimeout.js b/test/parallel/test-net-settimeout.js index 3bf26974576f2b..28943a1ef0c96b 100644 --- a/test/parallel/test-net-settimeout.js +++ b/test/parallel/test-net-settimeout.js @@ -12,7 +12,7 @@ var server = net.createServer(function(c) { }); server.listen(common.PORT); -var killers = [0, Infinity, NaN]; +var killers = [0]; var left = killers.length; killers.forEach(function(killer) { diff --git a/test/parallel/test-net-socket-timeout.js b/test/parallel/test-net-socket-timeout.js index c4d84fa177f51b..f4038298763e8d 100644 --- a/test/parallel/test-net-socket-timeout.js +++ b/test/parallel/test-net-socket-timeout.js @@ -2,6 +2,31 @@ var common = require('../common'); var net = require('net'); var assert = require('assert'); +// Verify that invalid delays throw +var noop = function() {}; +var s = new net.Socket(); +var nonNumericDelays = ['100', true, false, undefined, null, '', {}, noop, []]; +var badRangeDelays = [-0.001, -1, -Infinity, Infinity, NaN]; +var validDelays = [0, 0.001, 1, 1e6]; + +for (var i = 0; i < nonNumericDelays.length; i++) { + assert.throws(function() { + s.setTimeout(nonNumericDelays[i], noop); + }, TypeError); +} + +for (var i = 0; i < badRangeDelays.length; i++) { + assert.throws(function() { + s.setTimeout(badRangeDelays[i], noop); + }, RangeError); +} + +for (var i = 0; i < validDelays.length; i++) { + assert.doesNotThrow(function() { + s.setTimeout(validDelays[i], noop); + }); +} + var timedout = false; var server = net.Server(); From 39be594c3d6b89493ff6b3e8b50956caff516f92 Mon Sep 17 00:00:00 2001 From: Andrei Sedoi Date: Fri, 9 Jan 2015 19:55:32 +0200 Subject: [PATCH 2/7] doc: use correct signature for assert() The message argument is optional for both assert() and assert.ok(). This commit makes message optional for assert(). PR-URL: https://github.com/joyent/node/pull/9003 Reviewed-By: Colin Ihrig Reviewed-By: Trevor Norris --- doc/api/assert.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/assert.markdown b/doc/api/assert.markdown index 91628506a000b7..0901a250bcf20a 100644 --- a/doc/api/assert.markdown +++ b/doc/api/assert.markdown @@ -9,7 +9,7 @@ access it with `require('assert')`. Throws an exception that displays the values for `actual` and `expected` separated by the provided operator. -## assert(value, message), assert.ok(value[, message]) +## assert(value[, message]), assert.ok(value[, message]) Tests if value is truthy, it is equivalent to `assert.equal(true, !!value, message);` From 970841d13d85b0fb20e1bd2de9c64da2ff5545c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Herbert=20Voj=C4=8D=C3=ADk?= Date: Thu, 12 Feb 2015 14:50:47 +0100 Subject: [PATCH 3/7] module: replace NativeModule.require The NativeModule system passes NativeModule.require transparently and so is unnecessary to call explicitly. The only one which should have the prefix is the in line 295, where actually implements a big fs-based module system and actually requires a native module. That is left unchanged. PR-URL: https://github.com/joyent/node/pull/9201 Ref: https://github.com/joyent/node/issues/2009 Reviewed-by: Trevor Norris Conflicts: lib/module.js --- lib/module.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/module.js b/lib/module.js index 3f42975a63c87a..0d9f0932e55b62 100644 --- a/lib/module.js +++ b/lib/module.js @@ -1,11 +1,12 @@ 'use strict'; const NativeModule = require('native_module'); -const util = NativeModule.require('util'); +const util = require('util'); const runInThisContext = require('vm').runInThisContext; const runInNewContext = require('vm').runInNewContext; const assert = require('assert').ok; -const fs = NativeModule.require('fs'); +const fs = require('fs'); +const path = require('path'); // If obj.hasOwnProperty has been overridden, then calling @@ -41,9 +42,6 @@ Module.globalPaths = []; Module.wrapper = NativeModule.wrapper; Module.wrap = NativeModule.wrap; - -const path = NativeModule.require('path'); - Module._debug = util.debuglog('module'); From 43907ca7e08cec306ed80ab3a26fdbb8dea2b20a Mon Sep 17 00:00:00 2001 From: Dan Dascalescu Date: Wed, 11 Feb 2015 23:25:01 -0800 Subject: [PATCH 4/7] doc: fix code syntax Add a ';' to the end of a function call in documentation. PR-URL: https://github.com/joyent/node/pull/9198 Reviewed-by: Trevor Norris --- doc/api/fs.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown index 71685f8ff3acb5..fe78082cfe57c3 100644 --- a/doc/api/fs.markdown +++ b/doc/api/fs.markdown @@ -29,7 +29,7 @@ Here is the synchronous version: var fs = require('fs'); - fs.unlinkSync('/tmp/hello') + fs.unlinkSync('/tmp/hello'); console.log('successfully deleted /tmp/hello'); With the asynchronous methods there is no guaranteed ordering. So the From ff66973c53be75d1f597bcfce19d827daab6db1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Sch=C3=A4r?= Date: Mon, 9 Feb 2015 16:51:12 +0100 Subject: [PATCH 5/7] test: Timeout#unref() does not return instance Timeout#unref() call returns undefined, not this. The test already worked before, because the interval was still unref'd, and the test also succeeds without clearing the interval. PR-URL: https://github.com/joyent/node/pull/9171 Reviewed-by: Colin Ihrig Reviewed-by: Timothy J Fontaine Conflicts: test/simple/test-timers-unref.js --- test/parallel/test-timers-unref.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-timers-unref.js b/test/parallel/test-timers-unref.js index c4f9d820e5601f..9434dbbd36ad78 100644 --- a/test/parallel/test-timers-unref.js +++ b/test/parallel/test-timers-unref.js @@ -21,7 +21,8 @@ setTimeout(function() { interval = setInterval(function() { unref_interval = true; clearInterval(interval); -}, SHORT_TIME).unref(); +}, SHORT_TIME); +interval.unref(); setTimeout(function() { unref_timer = true; From 22c034676e4fb6500386f862a6a5d3577722d960 Mon Sep 17 00:00:00 2001 From: Debjeet Biswas Date: Mon, 9 Feb 2015 04:15:03 +0530 Subject: [PATCH 6/7] doc: grammar fix in smalloc PR-URL: https://github.com/joyent/node/pull/9164 Reviewed-by: Colin Ihrig --- doc/api/smalloc.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/smalloc.markdown b/doc/api/smalloc.markdown index f72857ff870dc8..19c18dc89e25c5 100644 --- a/doc/api/smalloc.markdown +++ b/doc/api/smalloc.markdown @@ -38,7 +38,7 @@ this it is possible to allocate external array data to more than a plain Object. v8 does not support allocating external array data to an Array, and if passed will throw. -It's possible is to specify the type of external array data you would like. All +It's possible to specify the type of external array data you would like. All possible options are listed in `smalloc.Types`. Example usage: var doubleArr = smalloc.alloc(3, smalloc.Types.Double); From a699cee296be33f9b11071d67f7986f137f03918 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 23 Sep 2014 23:08:35 -0400 Subject: [PATCH 7/7] net: remove use of arguments in Server constructor The current implementation uses the arguments object in the Server() constructor. Since both arguments to Server() are optional, there was a high likelihood of accessing a non-existent element in arguments, which carries a performance overhead. This commit replaces the arguments object with named arguments. Reviewed-by: Trevor Norris Conflicts: lib/net.js --- lib/net.js | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/net.js b/lib/net.js index 294c7f99ee6a7e..9f566b684d46ce 100644 --- a/lib/net.js +++ b/lib/net.js @@ -48,8 +48,8 @@ function isPipeName(s) { return typeof s === 'string' && toNumber(s) === false; } -exports.createServer = function() { - return new Server(arguments[0], arguments[1]); +exports.createServer = function(options, connectionListener) { + return new Server(options, connectionListener); }; @@ -991,22 +991,24 @@ function afterConnect(status, handle, req, readable, writable) { } -function Server(/* [ options, ] listener */) { - if (!(this instanceof Server)) return new Server(arguments[0], arguments[1]); +function Server(options, connectionListener) { + if (!(this instanceof Server)) + return new Server(options, connectionListener); + events.EventEmitter.call(this); var self = this; - var options; - if (typeof arguments[0] === 'function') { + if (typeof options === 'function') { + connectionListener = options; options = {}; - self.on('connection', arguments[0]); + self.on('connection', connectionListener); } else { - options = arguments[0] || {}; + options = options || {}; - if (typeof arguments[1] === 'function') { - self.on('connection', arguments[1]); + if (typeof connectionListener === 'function') { + self.on('connection', connectionListener); } }