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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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);

src/udp_wrap.cc

@@ -257,7 +257,7 @@ void UDPWrap::DoSend(const FunctionCallbackInfo<Value>& 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());

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);

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);

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);

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) { });

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);