From 353c6e2194be46d7ae0555ac0f4ba7e2987abeca Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 17 May 2016 12:36:54 +0200 Subject: [PATCH 1/4] dgram: copy the list in send This commit fix a possible crash situation in dgram send(). A crash is possible if an array is passed, and then altered after the send call, as the call to libuv is wrapped in process.nextTick(). Fixes: https://github.com/nodejs/node/issues/6616 --- lib/dgram.js | 12 ++++--- .../test-dgram-send-multi-buffer-copy.js | 34 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-dgram-send-multi-buffer-copy.js diff --git a/lib/dgram.js b/lib/dgram.js index e6e88b1a9a5ed6..a107ed6391a938 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -264,15 +264,19 @@ function sliceBuffer(buffer, offset, length) { function fixBuffer(buffer) { + const newlist = new Array(buffer.length); + for (var i = 0, l = buffer.length; i < l; i++) { var buf = buffer[i]; if (typeof buf === 'string') - buffer[i] = Buffer.from(buf); + newlist[i] = Buffer.from(buf); else if (!(buf instanceof Buffer)) - return false; + return null; + else + newlist[i] = buf; } - return true; + return newlist; } @@ -324,7 +328,7 @@ Socket.prototype.send = function(buffer, } else { buffer = [ buffer ]; } - } else if (!fixBuffer(buffer)) { + } else if (!(buffer = fixBuffer(buffer))) { throw new TypeError('Buffer list arguments must be buffers or strings'); } diff --git a/test/parallel/test-dgram-send-multi-buffer-copy.js b/test/parallel/test-dgram-send-multi-buffer-copy.js new file mode 100644 index 00000000000000..c85a5fd082dfc4 --- /dev/null +++ b/test/parallel/test-dgram-send-multi-buffer-copy.js @@ -0,0 +1,34 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const dgram = require('dgram'); + +const client = dgram.createSocket('udp4'); + +const timer = setTimeout(function() { + throw new Error('Timeout'); +}, common.platformTimeout(200)); + +const onMessage = common.mustCall(function(err, bytes) { + assert.equal(bytes, buf1.length + buf2.length); + clearTimeout(timer); + client.close(); +}); + +const buf1 = Buffer.alloc(256, 'x'); +const buf2 = Buffer.alloc(256, 'y'); + +client.on('listening', function() { + const toSend = [buf1, buf2]; + client.send(toSend, common.PORT, common.localhostIPv4, onMessage); + toSend.splice(0, 2); +}); + +client.on('message', function(buf, info) { + const expected = Buffer.concat([buf1, buf2]); + assert.ok(buf.equals(expected), 'message was received correctly'); + client.close(); +}); + +client.bind(common.PORT); From 1059d07873d19cf739813bb67ade692990fa9b3a Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 19 May 2016 11:30:42 +0200 Subject: [PATCH 2/4] dgram: support sending an empty array Sending an empty array was crashing in libuv, this commit allocates an empty buffer to allow sending an empty array. --- lib/dgram.js | 3 ++ test/parallel/test-dgram-send-empty-array.js | 35 ++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 test/parallel/test-dgram-send-empty-array.js diff --git a/lib/dgram.js b/lib/dgram.js index a107ed6391a938..73214efdc6bda4 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -346,6 +346,9 @@ Socket.prototype.send = function(buffer, if (self._bindState == BIND_STATE_UNBOUND) self.bind({port: 0, exclusive: true}, null); + if (buffer.length === 0) + buffer[0] = Buffer.allocUnsafe(0); + // If the socket hasn't been bound yet, push the outbound packet onto the // send queue and send after binding is complete. if (self._bindState != BIND_STATE_BOUND) { diff --git a/test/parallel/test-dgram-send-empty-array.js b/test/parallel/test-dgram-send-empty-array.js new file mode 100644 index 00000000000000..bc74d498869abc --- /dev/null +++ b/test/parallel/test-dgram-send-empty-array.js @@ -0,0 +1,35 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const dgram = require('dgram'); + +if (process.platform === 'darwin') { + common.skip('because of 17894467 Apple bug'); + return; +} + +const client = dgram.createSocket('udp4'); + +const timer = setTimeout(function() { + throw new Error('Timeout'); +}, common.platformTimeout(200)); + +const onMessage = common.mustCall(function(err, bytes) { + assert.equal(bytes, 0); + clearTimeout(timer); + client.close(); +}); + +client.on('listening', function() { + const toSend = []; + client.send(toSend, common.PORT, common.localhostIPv4, onMessage); +}); + +client.on('message', function(buf, info) { + const expected = Buffer.alloc(0); + assert.ok(buf.equals(expected), 'message was received correctly'); + client.close(); +}); + +client.bind(common.PORT); From 0f5f91751b8ec85becae7572f5415d9f733cad1d Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 19 May 2016 15:08:03 +0200 Subject: [PATCH 3/4] dgram: renamed buffer -> list in send nits about variable names and push() --- lib/dgram.js | 33 +++++++++++++++++---------------- src/udp_wrap.cc | 2 +- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index 73214efdc6bda4..e6cc169dc644fe 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -263,11 +263,11 @@ function sliceBuffer(buffer, offset, length) { } -function fixBuffer(buffer) { - const newlist = new Array(buffer.length); +function fixBufferList(list) { + const newlist = new Array(list.length); - for (var i = 0, l = buffer.length; i < l; i++) { - var buf = buffer[i]; + for (var i = 0, l = list.length; i < l; i++) { + var buf = list[i]; if (typeof buf === 'string') newlist[i] = Buffer.from(buf); else if (!(buf instanceof Buffer)) @@ -310,7 +310,8 @@ Socket.prototype.send = function(buffer, port, address, callback) { - var self = this; + const self = this; + let list; if (address || (port && typeof port !== 'function')) { buffer = sliceBuffer(buffer, offset, length); @@ -322,13 +323,13 @@ Socket.prototype.send = function(buffer, if (!Array.isArray(buffer)) { if (typeof buffer === 'string') { - buffer = [ Buffer.from(buffer) ]; + list = [ Buffer.from(buffer) ]; } else if (!(buffer instanceof Buffer)) { throw new TypeError('First argument must be a buffer or a string'); } else { - buffer = [ buffer ]; + list = [ buffer ]; } - } else if (!(buffer = fixBuffer(buffer))) { + } else if (!(list = fixBufferList(buffer))) { throw new TypeError('Buffer list arguments must be buffers or strings'); } @@ -346,23 +347,23 @@ Socket.prototype.send = function(buffer, if (self._bindState == BIND_STATE_UNBOUND) self.bind({port: 0, exclusive: true}, null); - if (buffer.length === 0) - buffer[0] = Buffer.allocUnsafe(0); + if (list.length === 0) + list.push(Buffer.allocUnsafe(0)); // If the socket hasn't been bound yet, push the outbound packet onto the // send queue and send after binding is complete. if (self._bindState != BIND_STATE_BOUND) { - enqueue(self, [buffer, port, address, callback]); + enqueue(self, [list, port, address, callback]); return; } self._handle.lookup(address, function afterDns(ex, ip) { - doSend(ex, self, ip, buffer, address, port, callback); + doSend(ex, self, ip, list, address, port, callback); }); }; -function doSend(ex, self, ip, buffer, address, port, callback) { +function doSend(ex, self, ip, list, address, port, callback) { if (ex) { if (typeof callback === 'function') { callback(ex); @@ -376,7 +377,7 @@ function doSend(ex, self, ip, buffer, address, port, callback) { } var req = new SendWrap(); - req.buffer = buffer; // Keep reference alive. + req.list = list; // Keep reference alive. req.address = address; req.port = port; if (callback) { @@ -384,8 +385,8 @@ function doSend(ex, self, ip, buffer, address, port, callback) { req.oncomplete = afterSend; } var err = self._handle.send(req, - buffer, - buffer.length, + list, + list.length, port, ip, !!callback); diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index be141a672cd3ab..bd7aa418bd89cf 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -257,7 +257,7 @@ void UDPWrap::DoSend(const FunctionCallbackInfo& args, int family) { args.Holder(), args.GetReturnValue().Set(UV_EBADF)); - // send(req, buffer, port, address, hasCallback) + // send(req, list, port, address, hasCallback) CHECK(args[0]->IsObject()); CHECK(args[1]->IsArray()); CHECK(args[2]->IsUint32()); From e1a4eeeddfd390380fa95dc451544b3cba894d14 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 19 May 2016 16:12:16 +0200 Subject: [PATCH 4/4] dgram: use common.mustCall and common.platformTimeout in send tests Fixes situations were some events were not asserted to be emitted. Removed some flakyness from tests. --- .../test-dgram-send-callback-buffer-length.js | 26 +++++++++---------- .../test-dgram-send-callback-multi-buffer.js | 9 +++---- test/parallel/test-dgram-send-empty-array.js | 16 ++++-------- test/parallel/test-dgram-send-empty-buffer.js | 4 +-- .../test-dgram-send-multi-buffer-copy.js | 5 ++-- 5 files changed, 25 insertions(+), 35 deletions(-) diff --git a/test/parallel/test-dgram-send-callback-buffer-length.js b/test/parallel/test-dgram-send-callback-buffer-length.js index f9883527b244c1..b44393898331ed 100644 --- a/test/parallel/test-dgram-send-callback-buffer-length.js +++ b/test/parallel/test-dgram-send-callback-buffer-length.js @@ -1,26 +1,24 @@ 'use strict'; -var common = require('../common'); -var assert = require('assert'); -var dgram = require('dgram'); -var client, timer, buf, len, offset; +const common = require('../common'); +const assert = require('assert'); +const dgram = require('dgram'); +const client = dgram.createSocket('udp4'); -client = dgram.createSocket('udp4'); - -buf = Buffer.allocUnsafe(256); -offset = 20; - -len = buf.length - offset; +const timer = setTimeout(function() { + throw new Error('Timeout'); +}, common.platformTimeout(200)); +const buf = Buffer.allocUnsafe(256); +const offset = 20; +const len = buf.length - offset; -client.send(buf, offset, len, common.PORT, '127.0.0.1', function(err, bytes) { +const messageSent = common.mustCall(function messageSent(err, bytes) { assert.notEqual(bytes, buf.length); assert.equal(bytes, buf.length - offset); clearTimeout(timer); client.close(); }); -timer = setTimeout(function() { - throw new Error('Timeout'); -}, 200); +client.send(buf, offset, len, common.PORT, '127.0.0.1', messageSent); diff --git a/test/parallel/test-dgram-send-callback-multi-buffer.js b/test/parallel/test-dgram-send-callback-multi-buffer.js index c2a1a53e007ff5..8e4870fbcbcbde 100644 --- a/test/parallel/test-dgram-send-callback-multi-buffer.js +++ b/test/parallel/test-dgram-send-callback-multi-buffer.js @@ -10,23 +10,22 @@ const timer = setTimeout(function() { throw new Error('Timeout'); }, common.platformTimeout(200)); -const onMessage = common.mustCall(function(err, bytes) { +const messageSent = common.mustCall(function messageSent(err, bytes) { assert.equal(bytes, buf1.length + buf2.length); clearTimeout(timer); - client.close(); }); const buf1 = Buffer.alloc(256, 'x'); const buf2 = Buffer.alloc(256, 'y'); client.on('listening', function() { - client.send([buf1, buf2], common.PORT, common.localhostIPv4, onMessage); + client.send([buf1, buf2], common.PORT, common.localhostIPv4, messageSent); }); -client.on('message', function(buf, info) { +client.on('message', common.mustCall(function onMessage(buf, info) { const expected = Buffer.concat([buf1, buf2]); assert.ok(buf.equals(expected), 'message was received correctly'); client.close(); -}); +})); client.bind(common.PORT); diff --git a/test/parallel/test-dgram-send-empty-array.js b/test/parallel/test-dgram-send-empty-array.js index bc74d498869abc..efe61dad10b978 100644 --- a/test/parallel/test-dgram-send-empty-array.js +++ b/test/parallel/test-dgram-send-empty-array.js @@ -15,21 +15,15 @@ const timer = setTimeout(function() { throw new Error('Timeout'); }, common.platformTimeout(200)); -const onMessage = common.mustCall(function(err, bytes) { - assert.equal(bytes, 0); +client.on('message', common.mustCall(function onMessage(buf, info) { + const expected = Buffer.alloc(0); + assert.ok(buf.equals(expected), 'message was received correctly'); clearTimeout(timer); client.close(); -}); +})); client.on('listening', function() { - const toSend = []; - client.send(toSend, common.PORT, common.localhostIPv4, onMessage); -}); - -client.on('message', function(buf, info) { - const expected = Buffer.alloc(0); - assert.ok(buf.equals(expected), 'message was received correctly'); - client.close(); + client.send([], common.PORT, common.localhostIPv4); }); client.bind(common.PORT); diff --git a/test/parallel/test-dgram-send-empty-buffer.js b/test/parallel/test-dgram-send-empty-buffer.js index f6845360d006e3..45edc135dc445a 100644 --- a/test/parallel/test-dgram-send-empty-buffer.js +++ b/test/parallel/test-dgram-send-empty-buffer.js @@ -11,10 +11,10 @@ const client = dgram.createSocket('udp4'); client.bind(common.PORT); -client.on('message', function(buffer, bytes) { +client.on('message', common.mustCall(function onMessage(buffer, bytes) { clearTimeout(timer); client.close(); -}); +})); const buf = Buffer.alloc(0); client.send(buf, 0, 0, common.PORT, '127.0.0.1', function(err, len) { }); diff --git a/test/parallel/test-dgram-send-multi-buffer-copy.js b/test/parallel/test-dgram-send-multi-buffer-copy.js index c85a5fd082dfc4..6ece56e02a9925 100644 --- a/test/parallel/test-dgram-send-multi-buffer-copy.js +++ b/test/parallel/test-dgram-send-multi-buffer-copy.js @@ -13,7 +13,6 @@ const timer = setTimeout(function() { const onMessage = common.mustCall(function(err, bytes) { assert.equal(bytes, buf1.length + buf2.length); clearTimeout(timer); - client.close(); }); const buf1 = Buffer.alloc(256, 'x'); @@ -25,10 +24,10 @@ client.on('listening', function() { toSend.splice(0, 2); }); -client.on('message', function(buf, info) { +client.on('message', common.mustCall(function onMessage(buf, info) { const expected = Buffer.concat([buf1, buf2]); assert.ok(buf.equals(expected), 'message was received correctly'); client.close(); -}); +})); client.bind(common.PORT);