From c14e98b476156487024254fe391512ae95e642e6 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 e6e88b1a9a..e6cc169dc6 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 be141a672c..bd7aa418bd 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 f9883527b2..b443938983 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 c2a1a53e00..8e4870fbcb 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 0000000000..efe61dad10 --- /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 f6845360d0..45edc135dc 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 0000000000..6ece56e02a --- /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);