GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — dgram: handle default address case when offset and length are specified by mcollina · Pull Request #5407 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented Feb 24, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mcollina mcollina added the dgram Issues and PRs related to UDP and the dgram module. label Feb 24, 2016
@mcollina mcollina added this to the 5.7.1 milestone Feb 24, 2016
@mcollina mcollina mentioned this pull request Feb 24, 2016
propose v5.7.1 #5400
Closed
3 tasks
rvagg
rvagg reviewed Feb 24, 2016
View reviewed changes
Comment thread lib/dgram.js Outdated

@rvagg rvagg Feb 24, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mcollina commented Feb 24, 2016

Copy link
Copy Markdown
Member Author

@rvagg updated! thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina self-assigned this Feb 24, 2016
rvagg
rvagg reviewed Feb 24, 2016
View reviewed changes
Comment thread lib/dgram.js Outdated

@rvagg rvagg Feb 24, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rvagg

rvagg commented Feb 24, 2016

Copy link
Copy Markdown
Member

lgtm

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@evanlucas

evanlucas commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mcollina commented Feb 24, 2016

Copy link
Copy Markdown
Member Author

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fishrock123

Fishrock123 commented Feb 24, 2016

Copy link
Copy Markdown
Contributor

linking to release proposal: #5400

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 25, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 25, 2016

Copy link
Copy Markdown
Member Author

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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 25, 2016

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@santigimeno

santigimeno commented Feb 25, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mcollina commented Feb 25, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 25, 2016

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rvagg

rvagg commented Feb 26, 2016

Copy link
Copy Markdown
Member

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@saghul

saghul commented Feb 26, 2016

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@saghul

saghul commented Feb 26, 2016

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@saghul

saghul commented Feb 26, 2016

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mcollina commented Feb 26, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 27, 2016

Copy link
Copy Markdown
Member Author

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 27, 2016

Copy link
Copy Markdown
Member Author

cc @silverwind

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind

silverwind commented Feb 28, 2016

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Feb 28, 2016

Copy link
Copy Markdown
Member Author

Landed in 725ffdb

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina closed this Feb 28, 2016
@mcollina mcollina mentioned this pull request Feb 29, 2016
dgram: follow the same standard of net for the default address #5487
Closed
@mcollina mcollina mentioned this pull request Feb 29, 2016
dgram: default send address to 127.0.0.1 or ::1 #5493
Closed
4 tasks done
@jasnell jasnell added the dont-land-on-v4.x label Mar 2, 2016
Fishrock123 referenced this pull request Mar 2, 2016
@mcollina @Fishrock123
dgram: handle default address case when offset and length are specified
d3f9b84
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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Fishrock123 added a commit that referenced this pull request Mar 2, 2016
@Fishrock123
2016-03-02, Version 5.7.1 (Stable)
805f054
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
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 2, 2016
@Fishrock123
2016-03-02, Version 5.7.1 (Stable)
9091ccd
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
mcollina added a commit to mcollina/node that referenced this pull request Mar 4, 2016
@mcollina
dgram: default send address to 127.0.0.1 or ::1
cdb236d
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
mcollina added a commit that referenced this pull request Mar 5, 2016
@mcollina
dgram: default send address to 127.0.0.1 or ::1
8af4bb8
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é <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@mcollina @Fishrock123
dgram: default send address to 127.0.0.1 or ::1
ff42937
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é <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
@mcollina @Fishrock123
dgram: default send address to 127.0.0.1 or ::1
67e9f65
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é <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins referenced this pull request Mar 11, 2016
@mcollina
dgram: handle default address case when offset and length are specified
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 <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@Clemens-git76 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@mcollina mcollina

Labels

dgram Issues and PRs related to UDP and the dgram module.

Projects

None yet

Milestone

5.7.1

Development

Successfully merging this pull request may close these issues.

dgram: socket.send without address fails with weird error on 5.7

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@mcollina @rvagg @evanlucas @Fishrock123 @santigimeno @saghul @silverwind @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.