Skip to content

dns: handle array holes in setServers() - #8567

Merged
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:8538
Sep 21, 2016
Merged

dns: handle array holes in setServers()#8567
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:8538

Conversation

@cjihrig

Copy link
Copy Markdown
Contributor
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-botnodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Sep 16, 2016

@imyllerimyller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@imyller

Copy link
Copy Markdown
Member

@imyller

Copy link
Copy Markdown
Member

@lpincalpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@imyller

Copy link
Copy Markdown
Member

@cjihrig any objections for landing this?

@cjihrig

Copy link
Copy Markdown
ContributorAuthor

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

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

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

@imyllerimyller self-assigned this Sep 20, 2016
@ChALkeR

Copy link
Copy Markdown
Member

Is this semver-minor or semver-patch?

@imyller

imyller commented Sep 20, 2016

Copy link
Copy Markdown
Member

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

@addaleax

Copy link
Copy Markdown
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.

@imyllerimyller removed their assignment Sep 20, 2016

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

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 <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

Copy link
Copy Markdown
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

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
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: #8538
PR-URL: #8567
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dnsIssues and PRs related to the dns subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cjihrig@imyller@ChALkeR@addaleax@MylesBorins@jasnell@lpinca@nodejs-github-bot