dgram: handle default address case when offset and length are specified #5407

mcollina · 2016-02-24T08:20:29Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

dgram

Description of change

Fixes a regression introduced by #4374 and released in 5.7.0.
Adds a new test to avoid similar issue in the future.

Fixes: #5398

Please review @jasnell @saghul @mafintosh @Fishrock123 @rvagg

rvagg · 2016-02-24T08:38:52Z

lib/dgram.js

@@ -294,8 +302,7 @@ Socket.prototype.send = function(buffer,
                                 callback) {
  var self = this;

-  // same as arguments.length === 5 || arguments.length === 6
-  if (address) {
+  if (address || port && !isFunction(port)) {


we've given up on util.is*(), just do a typeof instead

mcollina · 2016-02-24T08:57:30Z

@rvagg updated! thanks!

rvagg · 2016-02-24T09:15:46Z

lib/dgram.js

@@ -294,8 +301,7 @@ Socket.prototype.send = function(buffer,
                                 callback) {
  var self = this;

-  // same as arguments.length === 5 || arguments.length === 6
-  if (address) {
+  if (address || port && typeof port !== 'function') {


sorry, a very minor nit but I'd prefer brackets around the && section to be explicit although I don't know if we've set precedent elsewhere in the code

rvagg · 2016-02-24T09:16:10Z

lgtm

evanlucas · 2016-02-24T12:42:15Z

LGTM

mcollina · 2016-02-24T12:47:42Z

@rvagg nit fixed. I'll wait for CI to come back online before merging.

@mafintosh can you please check if this solves the problem for you?

Fishrock123 · 2016-02-24T15:58:51Z

linking to release proposal: #5400

mcollina · 2016-02-25T10:32:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1743/

mcollina · 2016-02-25T11:20:50Z

I have a failure on windows: https://ci.nodejs.org/job/node-test-binary-windows/1193/RUN_SUBSET=3,VS_VERSION=vs2013,label=win2008r2/tapTestReport/test.tap-49/

It's a timeout issue, maybe I should just increate the timeout?
I think it's a flaky one, as it happens only on some envs, I'll retry.

New run, without any changes: https://ci.nodejs.org/job/node-test-pull-request/1744/

mcollina · 2016-02-25T14:06:30Z

Ok, it is definitely an issue on windows 2008r2 and windows 2012r2. Any idea? BTW, this case was not covered by unit tests before, so it might well be the default address was not working on windows.

santigimeno · 2016-02-25T14:10:30Z

@mcollina what happens if you remove the timer from the test?

mcollina · 2016-02-25T15:36:03Z

I've increased the timeout by 10 times, let's see how it goes: https://ci.nodejs.org/job/node-test-pull-request/1750/.

mcollina · 2016-02-25T16:18:10Z

Ok, it seems this is problematic on windows. I'll check tomorrow on my windows box (or is there anybody that can help). Given there were no unit tests for this case before, it's possible that we have this bug on Windows even on previous releases - I'll check that as well.

mcollina · 2016-02-26T09:14:42Z

Confirmed, this does not work on windows at all (tested quickly in v4.2.4):

'use strict'

const dgram = require('dgram')
const socket = dgram.createSocket('udp4')
const port = 2222
const assert = require('assert')
const buf = new Buffer('hello')

socket.on('listening', () => {
  socket.send(buf, 0, buf.length, port)
})

const timer = setTimeout(() => {
  assert.fail('no message')
}, 1000)

socket.on('message', (rec) => {
  assert.ok(rec.equals(buf), 'buffer do not matches')
  clearTimeout(timer)
  socket.close()
})

socket.bind(port)

mcollina · 2016-02-26T09:18:06Z

I might need some help to get this fixed and released soonish. @nodejs/platform-windows can you help me out (also the windows fix might need backport to LTS)?

rvagg · 2016-02-26T09:19:37Z

@saghul @indutny perhaps you could weight in as well?

If this doesn't work on Windows, perhaps it shouldn't be an officially supported piece of functionality then? Sounds like it was supported unintentionally in the first place.

saghul · 2016-02-26T10:22:22Z

I can take a look, but before I do, can you try the following? Bind to 127.0.0.1 instead of letting the autobind magic work.

Also, the target address should be specified IMHO. It may work somewhere by accident, but what does it mean to send a datagram to port 2222? As in, port 2222 of what IP address?

saghul · 2016-02-26T10:24:35Z

I took a quick peek, and the internal lookup thing sets the address to '0.0.0.0' if undefined, that's not going to fly on all platforms, I'm afraid.

mcollina · 2016-02-26T10:33:25Z

@saghul binding to 127.0.0.1 does not work either.

Also, the target address should be specified IMHO. It may work somewhere by accident, but what does it mean to send a datagram to port 2222? As in, port 2222 of what IP address?

I completely agree with you, but in the docs we support address to be optional (IMHO it should not be):

If the address is not specified or is an empty string, '0.0.0.0' or '::0' will be used instead

@mafintosh this means you had a bug in your library on windows systems anyway.

We have an accidental behavior on which modules rely on, so I think we should keep supporting that, and maybe look for deprecating the default address (in dgram, net, tls, http, ...) in the next semver-major cycle for outgoing connections/messages.

How can I rewrite the tests so that they are run only on Linux/Mac? Should I just add an if at the beginning and early return if windows?

saghul · 2016-02-26T12:02:45Z

I completely agree with you, but in the docs we support address to be optional (IMHO it should not be):

The docs actually say this: "It is possible, depending on the network configuration, that these defaults may not work; accordingly, it is best to be explicit about the destination address. " so, I suggest you write the test by passing the address and no rely on it being undefined.

mcollina · 2016-02-26T12:22:17Z

@saghul the bug we are trying to address here is that I (unintentionally) broke the old "half-broken" behavior:

It is possible, depending on the network configuration, that these defaults may not work; accordingly, it is best to be explicit about the destination address.

To avoid any regressions, I am adding a unit test, because there was no one before.
I will update the unit test and returning early if windows.

mcollina · 2016-02-26T13:05:09Z

new CI: https://ci.nodejs.org/job/node-test-pull-request/1758/

mcollina · 2016-02-27T17:21:47Z

Tests passes, I've added a bit more of comments around it.

Can I get a bunch of LGTM so we can land this?

mcollina · 2016-02-27T17:21:55Z

cc @silverwind

silverwind · 2016-02-28T01:48:35Z

LGTM as a short-term fix but not too happy about the Windows skip, it should be researched further.

mcollina · 2016-02-28T10:28:47Z

Landed in 725ffdb

Fixes a regression introduced by: #4374. Adds a new test to avoid similar issue in the future. The test is disabled on windows, because this feature never worked there. Fixes: #5398 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Evan Lucas <evanlucas@me.com>

Notable changes: * governance: The Core Technical Committee (CTC) added four new members to help guide Node.js core development: Evan Lucas, Rich Trott, Ali Ijaz Sheikh and Сковорода Никита Андреевич (Nikita Skovoroda). * openssl: Upgrade from 1.0.2f to 1.0.2g (Ben Noordhuis) #5507 - Fix a double-free defect in parsing malformed DSA keys that may potentially be used for DoS or memory corruption attacks. It is likely to be very difficult to use this defect for a practical attack and is therefore considered low severity for Node.js users. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0705 - Fix a defect that can cause memory corruption in certain very rare cases relating to the internal `BN_hex2bn()` and `BN_dec2bn()` functions. It is believed that Node.js is not invoking the code paths that use these functions so practical attacks via Node.js using this defect are _unlikely_ to be possible. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0797 - Fix a defect that makes the CacheBleed Attack (https://ssrg.nicta.com.au/projects/TS/cachebleed/) possible. This defect enables attackers to execute side-channel attacks leading to the potential recovery of entire RSA private keys. It only affects the Intel Sandy Bridge (and possibly older) microarchitecture when using hyper-threading. Newer microarchitectures, including Haswell, are unaffected. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0702 * Fixed several regressions that appeared in v5.7.0: - path.relative(): - Output is no longer unnecessarily verbose (Brian White) #5389 - Resolving UNC paths on Windows now works correctly (Owen Smith) #5456 - Resolving paths with prefixes now works correctly from the root directory (Owen Smith) #5490 - url: Fixed an off-by-one error with `parse()` (Brian White) #5394 - dgram: Now correctly handles a default address case when offset and length are specified (Matteo Collina) #5407 PR-URL: #5464

Notable changes: * governance: The Core Technical Committee (CTC) added four new members to help guide Node.js core development: Evan Lucas, Rich Trott, Ali Ijaz Sheikh and Сковорода Никита Андреевич (Nikita Skovoroda). * openssl: Upgrade from 1.0.2f to 1.0.2g (Ben Noordhuis) nodejs#5507 - Fix a double-free defect in parsing malformed DSA keys that may potentially be used for DoS or memory corruption attacks. It is likely to be very difficult to use this defect for a practical attack and is therefore considered low severity for Node.js users. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0705 - Fix a defect that can cause memory corruption in certain very rare cases relating to the internal `BN_hex2bn()` and `BN_dec2bn()` functions. It is believed that Node.js is not invoking the code paths that use these functions so practical attacks via Node.js using this defect are _unlikely_ to be possible. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0797 - Fix a defect that makes the CacheBleed Attack (https://ssrg.nicta.com.au/projects/TS/cachebleed/) possible. This defect enables attackers to execute side-channel attacks leading to the potential recovery of entire RSA private keys. It only affects the Intel Sandy Bridge (and possibly older) microarchitecture when using hyper-threading. Newer microarchitectures, including Haswell, are unaffected. More info is available at https://www.openssl.org/news/vulnerabilities.html#2016-0702 * Fixed several regressions that appeared in v5.7.0: - path.relative(): - Output is no longer unnecessarily verbose (Brian White) nodejs#5389 - Resolving UNC paths on Windows now works correctly (Owen Smith) nodejs#5456 - Resolving paths with prefixes now works correctly from the root directory (Owen Smith) nodejs#5490 - url: Fixed an off-by-one error with `parse()` (Brian White) nodejs#5394 - dgram: Now correctly handles a default address case when offset and length are specified (Matteo Collina) nodejs#5407 PR-URL: nodejs#5464

In net we default to 'localhost' as the default address for connect. Not doing the same on dgram is confusing, because sending to 0.0.0.0 works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 / ::1 addresses that. Related: nodejs#5407 Related: nodejs#5398 Fixes: nodejs#5487

In net we default to 'localhost' as the default address for connect. Not doing the same on dgram is confusing, because sending to 0.0.0.0 works on Linux/OS X but not on Windows. Defaulting that to 127.0.0.1 / ::1 addresses that. Related: #5407 Related: #5398 Fixes: #5487 PR-URL: #5493 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Fixes a regression introduced by: #4374. Adds a new test to avoid similar issue in the future. The test is disabled on windows, because this feature never worked there. Fixes: #5398 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Evan Lucas <evanlucas@me.com>

mcollina added the dgram Issues and PRs related to the dgram subsystem / UDP. label Feb 24, 2016

mcollina added this to the 5.7.1 milestone Feb 24, 2016

mcollina mentioned this pull request Feb 24, 2016

propose v5.7.1 #5400

Closed

3 tasks

rvagg reviewed Feb 24, 2016
View reviewed changes

mcollina force-pushed the fix-dgram-regression branch from 1797304 to 056e607 Compare February 24, 2016 08:57

mcollina self-assigned this Feb 24, 2016

rvagg reviewed Feb 24, 2016
View reviewed changes

mcollina force-pushed the fix-dgram-regression branch from 056e607 to 7ae7c48 Compare February 24, 2016 12:46

mcollina force-pushed the fix-dgram-regression branch from 7ae7c48 to 379f99e Compare February 25, 2016 15:34

mcollina force-pushed the fix-dgram-regression branch from 379f99e to 07fec97 Compare February 26, 2016 13:04

mcollina closed this Feb 28, 2016

mcollina mentioned this pull request Feb 29, 2016

dgram: follow the same standard of net for the default address #5487

Closed

mcollina mentioned this pull request Feb 29, 2016

dgram: default send address to 127.0.0.1 or ::1 #5493

Closed

4 tasks

jasnell added the dont-land-on-v4.x label Mar 2, 2016

Clemens-git76 mentioned this pull request Apr 16, 2024

[Snyk] Upgrade rollup from 2.75.6 to 4.13.0 Clemens-git76/node#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dgram: handle default address case when offset and length are specified #5407

dgram: handle default address case when offset and length are specified #5407

mcollina commented Feb 24, 2016

rvagg Feb 24, 2016

mcollina commented Feb 24, 2016

rvagg Feb 24, 2016

rvagg commented Feb 24, 2016

evanlucas commented Feb 24, 2016

mcollina commented Feb 24, 2016

Fishrock123 commented Feb 24, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 25, 2016

santigimeno commented Feb 25, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 26, 2016

mcollina commented Feb 26, 2016

rvagg commented Feb 26, 2016

saghul commented Feb 26, 2016

saghul commented Feb 26, 2016

mcollina commented Feb 26, 2016

saghul commented Feb 26, 2016

mcollina commented Feb 26, 2016

mcollina commented Feb 26, 2016

mcollina commented Feb 27, 2016

mcollina commented Feb 27, 2016

silverwind commented Feb 28, 2016

mcollina commented Feb 28, 2016

dgram: handle default address case when offset and length are specified #5407

dgram: handle default address case when offset and length are specified #5407

Conversation

mcollina commented Feb 24, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

rvagg Feb 24, 2016

Choose a reason for hiding this comment

mcollina commented Feb 24, 2016

rvagg Feb 24, 2016

Choose a reason for hiding this comment

rvagg commented Feb 24, 2016

evanlucas commented Feb 24, 2016

mcollina commented Feb 24, 2016

Fishrock123 commented Feb 24, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 25, 2016

santigimeno commented Feb 25, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 25, 2016

mcollina commented Feb 26, 2016

mcollina commented Feb 26, 2016

rvagg commented Feb 26, 2016

saghul commented Feb 26, 2016

saghul commented Feb 26, 2016

mcollina commented Feb 26, 2016

saghul commented Feb 26, 2016

mcollina commented Feb 26, 2016

mcollina commented Feb 26, 2016

mcollina commented Feb 27, 2016

mcollina commented Feb 27, 2016

silverwind commented Feb 28, 2016

mcollina commented Feb 28, 2016