From 8e6f8b2fb541c5a7b71d34e2f4e5c22b1dc416e5 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 17 May 2016 12:36:54 +0200 Subject: [PATCH] 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(). It also avoid sending an empty array to libuv by allocating an empty buffer. It also does some cleanup inside send() to increase readability. It removes test flakyness by use common.mustCall and common.platformTimeout. Fixes situations were some events were not asserted to be emitted. Fixes: https://github.com/nodejs/node/issues/6616 PR-URL: https://github.com/nodejs/node/pull/6804 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- lib/dgram.js | 40 +++++++++++-------- src/udp_wrap.cc | 2 +- .../test-dgram-send-callback-buffer-length.js | 26 ++++++------ .../test-dgram-send-callback-multi-buffer.js | 9 ++--- test/parallel/test-dgram-send-empty-array.js | 29 ++++++++++++++ test/parallel/test-dgram-send-empty-buffer.js | 4 +- .../test-dgram-send-multi-buffer-copy.js | 33 +++++++++++++++ 7 files changed, 105 insertions(+), 38 deletions(-) create mode 100644 test/parallel/test-dgram-send-empty-array.js create mode 100644 test/parallel/test-dgram-send-multi-buffer-copy.js diff --git a/lib/dgram.js b/lib/dgram.js index 4473282148fdff..19ba38ce648529 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -263,16 +263,20 @@ function sliceBuffer(buffer, offset, length) { } -function fixBuffer(buffer) { - for (var i = 0, l = buffer.length; i < l; i++) { - var buf = buffer[i]; +function fixBufferList(list) { + const newlist = new Array(list.length); + + for (var i = 0, l = list.length; i < l; i++) { + var buf = list[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; } @@ -306,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); @@ -318,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 (!fixBuffer(buffer)) { + } else if (!(list = fixBufferList(buffer))) { throw new TypeError('Buffer list arguments must be buffers or strings'); } @@ -342,20 +347,23 @@ Socket.prototype.send = function(buffer, if (self._bindState == BIND_STATE_UNBOUND) self.bind({port: 0, exclusive: true}, null); + 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); @@ -369,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) { @@ -377,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()); 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 new file mode 100644 index 00000000000000..efe61dad10b978 --- /dev/null +++ b/test/parallel/test-dgram-send-empty-array.js @@ -0,0 +1,29 @@ +'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)); + +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() { + 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 new file mode 100644 index 00000000000000..6ece56e02a9925 --- /dev/null +++ b/test/parallel/test-dgram-send-multi-buffer-copy.js @@ -0,0 +1,33 @@ +'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); +}); + +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', 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);