Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: handle array holes in setServers() #8567

Merged
merged 1 commit into from
Sep 21, 2016
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Sep 16, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

dns

Description of change

This commit adds better handling of exceptional array formats passed to dns.setServers(). Prior to this commit, the input array was validated using map(), which preserves holes, allowing them to be passed to c-ares, crashing Node. This commit replaces map() with forEach(), which skips holes.

Tests taken from #8538. Fixes #8538

@nodejs-github-bot nodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Sep 16, 2016
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller
Copy link
Member

imyller commented Sep 16, 2016

@imyller
Copy link
Member

imyller commented Sep 17, 2016

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller
Copy link
Member

imyller commented Sep 20, 2016

@cjihrig any objections for landing this?

@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 20, 2016

I was waiting for the CI to clear up to get a better run. Feel free to land it. Otherwise, I'll try to get to it this evening.

@imyller
Copy link
Member

imyller commented Sep 20, 2016

Ok, I'll take this to a smooth landing. Let's resolve semver question first.

@imyller imyller self-assigned this Sep 20, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Sep 20, 2016

Is this semver-minor or semver-patch?

@imyller
Copy link
Member

imyller commented Sep 20, 2016

@ChALkeR should I stop landing? Was about to push when you posted your comment.

@addaleax
Copy link
Member

Since I think we usually equate failing CHECKs with bugs in core (if only for missing type checking in JS), I’d say semver-patch.

@imyller imyller removed their assignment Sep 20, 2016
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

This commit adds better handling of exceptional array formats
passed to dns.setServers(). Prior to this commit, the input
array was validated using map(), which preserves holes, allowing
them to be passed to c-ares, crashing Node. This commit replaces
map() with forEach(), which skips holes.

Fixes: nodejs#8538
PR-URL: nodejs#8567
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

the original test does not dump core on v4.x

@cjihrig let me know if this should be backported, labeling as don't land for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants