dgram: socket.send() crash when input array is modified

[bnoordhuis]

#4374 introduces a bug: it makes socket.send() accept an array but it's not resilient against that array getting modified afterwards:

$ node -e '
  var a = ["boom!"], s = dgram.createSocket("udp4");
  s.send(a, 1234);
  a.splice(0);
'
node: ../deps/uv/src/unix/udp.c:390: uv__udp_send: Assertion `nbufs > 0' failed.
Aborted (core dumped)

A secondary issue with #4374 is that it penalizes the common case of passing in a buffer by always wrapping it in an array.

/cc @mcollina @jasnell

[mcollina]

I tried hard to benchmark the array allocation penalty, and I failed to produce a statistical difference, at least when I did the PR.

Regarding the array change you are right, we need to clone. I hope it will not cause a perfomance penalty.

BTW, did this come from a user issue? Is there some urgency in fixing it? I will not have bandwidth till mid-may to work on it.

[bnoordhuis]

BTW, did this come from a user issue? Is there some urgency in fixing it?

I don't think it's critical. I was checking something in lib/dgram.js and this stood out.

[mcollina]

@bnoordhuis let me know if you find any other area worth improving (there is actually http://docs.libuv.org/en/v1.x/udp.html#c.uv_udp_try_send to implement)

[ghost]

This doesn't look like an array modification problem because the following works:

$ node -e '
  var a = ["boom!", "bang"], s = dgram.createSocket("udp4");
  s.send(a, 1234);
  a.splice(1);
'

It just looks like send doesn't accept an empty set of buffers:

$ node -e '
  var a = [], s = dgram.createSocket("udp4");
  s.send(a, 1234);
'
node: ../deps/uv/src/unix/udp.c:390: uv__udp_send: Assertion `nbufs > 0' failed.
Abort trap: 6

[ghost]

I'm new to this codebase so let me know if I'm off base here but after looking at the uv code for unix and win, it looks like the unix version shouldn't have a problem because it copies the buffer pointers:

int uv__udp_send(uv_udp_send_t* req,
                 uv_udp_t* handle,
                 const uv_buf_t bufs[],
...
  memcpy(req->bufs, bufs, nbufs * sizeof(bufs[0]));
...

The windows version, however, passes them in directly to WSASendTo whose documentation says the buffers should not be modified for the duration of the call:

pBuffers [in]
A pointer to an array of WSABUF structures. Each WSABUF structure contains a pointer to a buffer and the length of the buffer, in bytes. For a Winsock application, once the WSASendTo function is called, the system owns these buffers and the application may not access them. This array must remain valid for the duration of the send operation.

Is it an option to only wrap the win call (or to fix it so that it also copies the buffer pointers in the C code?)

[mcollina]

@brown-dragon there is a slight chance for a GC run to deallocate those buffers in the meanwhile, as the buffers are not copied. A new array needs to be allocated and all values copied over.