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 — test: add non-internet resolveAny tests by addaleax · Pull Request #13883 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Jun 22, 2017

Copy link
Copy Markdown
Member

This is a bit of a check to see how people feel about having this kind of test.

Ref: #13137

/cc @nodejs/testing @XadillaX

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

test/dns

Sorry, something went wrong.

Uh oh!

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

All reactions
@addaleax addaleax added dns Issues and PRs related to the dns subsystem. dont-land-on-v4.x test Issues and PRs related to Node.js core tests and test infrastructure. labels Jun 22, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Jun 22, 2017
@refack

refack commented Jun 22, 2017

Copy link
Copy Markdown
Contributor

I'm not against the concept. (personally would prefer a tested implementation but the only one I found is https://www.npmjs.com/package/dnsd 😕 and it's kind of dormant).

All reactions

Sorry, something went wrong.

Uh oh!

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

@Trott

Trott commented Jun 23, 2017

Copy link
Copy Markdown
Member

So this allows a test double for the DNS server? Seems like a 👍 idea to me. Certainly better than not running the tests in CI ever!

All reactions

Sorry, something went wrong.

Uh oh!

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

@addaleax

addaleax commented Jun 23, 2017

Copy link
Copy Markdown
Member Author

@Trott Yes, it’s essentially an implementation of a small fake DNS server. One of the nice things about it is that we can use it to send invalid responses as well, or make testing less dependent on external factors like what kind of records exist for a specific hard-coded domain.

@refack If you want to put the work in it, sure, try to build something with dnsd and open a PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gibfahn

gibfahn commented Jun 23, 2017

Copy link
Copy Markdown
Member

+1 on this (needs something in test/common/README.md, but I'm pretty sure @addaleax is making sure everyone's okay with this first).

personally would prefer a tested implementation

Maybe, but I assume a tailored one can be much more concise (I assume we don't need a full DNS server).

All reactions

Sorry, something went wrong.

Uh oh!

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

cjihrig
cjihrig approved these changes Jun 23, 2017
View reviewed changes
Comment thread test/common/dns.js Outdated

@cjihrig cjihrig Jun 23, 2017

Copy link
Copy Markdown
Contributor

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.

Is there a specific rule that can be turned off here?

Sorry, something went wrong.

Uh oh!

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

All reactions

@Trott Trott Jun 23, 2017 •
edited
Loading

Uh oh!

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

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.

If &~ 0xC000 on the next line is changed to & ~0xC000, then we don't need this ESLint disable comment.

Sorry, something went wrong.

Uh oh!

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

All reactions

@addaleax addaleax Jun 26, 2017

Copy link
Copy Markdown
Member Author

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.

Eslint is unhappy about both the missing space before ~ and after &, so it’s two separate rules.

If &~ 0xC000 on the next line is changed to & ~0xC000, then we don't need this ESLint disable comment.

Right, but I’d consider &~ more idiomatic for bit-twiddling code (because it removes an extra thought iteration, from “bitwise and with some oddly generated mask” to “it’s just removing those bits”).

Sorry, something went wrong.

Uh oh!

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

All reactions

@refack refack Jun 26, 2017

Copy link
Copy Markdown
Contributor

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.

How about:

const kLengthMask = 0xC0;
const kOffsetMask = ~0xC000;

I get the idiom, but I have to use my fingers to figure out 0xC000 === 0o‭140000‬ == 0b1100000000000000 === 0x3 << 14 and my mind just has to do that. Using a const will allow me to abstract it away 😅

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread test/common/dns.js Outdated

@cjihrig cjihrig Jun 23, 2017

Copy link
Copy Markdown
Contributor

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.

Not that it really matters, but why not just use standard quote characters here?

Sorry, something went wrong.

Uh oh!

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

All reactions

@addaleax addaleax Jun 26, 2017

Copy link
Copy Markdown
Member Author

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.

But these are the proper quotation marks? ;) (All trolling aside, I’m just used to typing these if it’s not for code, that’s all.)

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread test/common/dns.js Outdated

@cjihrig cjihrig Jun 23, 2017

Copy link
Copy Markdown
Contributor

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.

There are a couple uses of split() on single characters in this PR. Would the string version work (split('.'))? It seems more readable.

Sorry, something went wrong.

Uh oh!

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

All reactions

@addaleax addaleax Jun 26, 2017

Copy link
Copy Markdown
Member Author

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.

@cjihrig I just literally forgot that split can work with plain strings. Updated :)

Sorry, something went wrong.

Uh oh!

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

All reactions
@refack

refack commented Jun 26, 2017

Copy link
Copy Markdown
Contributor

@refack If you want to put the work in it, sure, try to build something with dnsd and open a PR.

Let's do the opposite, publish this as dns-mock (add me and I'll write some tests).

All reactions

Sorry, something went wrong.

Uh oh!

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

refack
refack reviewed Jun 26, 2017
View reviewed changes
Comment thread test/common/dns.js Outdated

@refack refack Jun 26, 2017

Copy link
Copy Markdown
Contributor

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.

How about:

const kLengthMask = 0xC0;
const kOffsetMask = ~0xC000;

I get the idiom, but I have to use my fingers to figure out 0xC000 === 0o‭140000‬ == 0b1100000000000000 === 0x3 << 14 and my mind just has to do that. Using a const will allow me to abstract it away 😅

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread test/common/dns.js Outdated

@refack refack Jun 26, 2017

Copy link
Copy Markdown
Contributor

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, something went wrong.

Uh oh!

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

All reactions

@addaleax addaleax Jun 26, 2017

Copy link
Copy Markdown
Member Author

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.

·@refack This is not a very helpful comment, I have no idea what you are trying to say.

Sorry, something went wrong.

Uh oh!

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

refack reacted with laugh emoji
All reactions
  • 😄 1 reaction

@refack refack Jun 26, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

Please define this as a const so it's more obvious what are flags you want.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread test/common/dns.js Outdated

@refack refack Jun 26, 2017

Copy link
Copy Markdown
Contributor

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.

Do you need this?

Sorry, something went wrong.

Uh oh!

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

All reactions

@addaleax addaleax Jun 26, 2017

Copy link
Copy Markdown
Member Author

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.

Yes, the linter complains otherwise.

Sorry, something went wrong.

Uh oh!

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

refack reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Trott Trott Jun 27, 2017 •
edited
Loading

Uh oh!

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

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.

You could replace it with /* eslint-disable required-modules */ at the top of the file the way it is in test/common/wpt.js. I think I'd prefer that. If it's leaking globals, then any tests that use it will complain, so we're covered without importing the common module.

Sorry, something went wrong.

Uh oh!

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

All reactions

@addaleax addaleax Jul 6, 2017

Copy link
Copy Markdown
Member Author

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.

@Trott Sorry for being so late, I’ve updated this PR with eslint-disable required-modules

Sorry, something went wrong.

Uh oh!

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

All reactions
@refack refack mentioned this pull request Jul 5, 2017
test: investigate flakiness of parallel/test-async-wrap-getasyncid #13020
Closed
@addaleax
addaleax force-pushed the dns-tests branch from 3f21f79 to fe457ba Compare July 6, 2017 22:40
refack
refack approved these changes Jul 6, 2017
View reviewed changes
@jasnell

jasnell commented Jul 10, 2017

Copy link
Copy Markdown
Member

I'm generally +1 on having this (and love seeing this kind of work) but my one concern is that there are no tests to verify that the common/dns.js script is itself exhibiting correct behavior. That is, if the test server is buggy, we could end up with buggy dns tests if we are not careful. I'm not sure if there's anything we can actually do about it, but it is definitely something to keep in mind.

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@refack

refack commented Jul 10, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I'm not sure if there's anything we can actually do about it, but it is definitely something to keep in mind.

If we run /test/internet/ on a regular basis we could infer that the net-less tests are correct.

All reactions

Sorry, something went wrong.

Uh oh!

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

@addaleax

addaleax commented Jul 10, 2017

Copy link
Copy Markdown
Member Author

Just to mention it again, the main scenario that prompted this is checking how our code handles invalid responses, something we won’t be able to do with a “proper” DNS server.

refack and jasnell reacted with thumbs up emoji richardlau reacted with laugh emoji
All reactions
  • 👍 2 reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

@jasnell

jasnell commented Jul 10, 2017

Copy link
Copy Markdown
Member

Yeah, good point :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@addaleax

addaleax commented Jul 12, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/11081/

All reactions

Sorry, something went wrong.

Uh oh!

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

jasnell
jasnell approved these changes Jul 14, 2017
View reviewed changes
@addaleax
addaleax force-pushed the dns-tests branch from fe457ba to 340c986 Compare July 15, 2017 04:26
addaleax added 2 commits July 15, 2017 18:59
@addaleax
deps: cherry-pick 18ea996 from c-ares upstream
185b5d5
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
@addaleax
test: add non-internet resolveAny tests
9c1e270
This is a bit of a check to see how people feel about having this kind
of test.

Ref: nodejs#13137
PR-URL: nodejs#13883
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax
addaleax force-pushed the dns-tests branch from 340c986 to 9c1e270 Compare July 15, 2017 11:00
@addaleax

addaleax commented Jul 15, 2017

Copy link
Copy Markdown
Member Author

Added c-ares/c-ares@18ea996 because both the test proposed here and our existing internet tests (make test-internet) are currently broken after the security release.

CI is green: https://ci.nodejs.org/job/node-test-commit/11128/

jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@addaleax addaleax added lts-watch-v4.x labels Jul 23, 2017
@addaleax

addaleax commented Jul 23, 2017

Copy link
Copy Markdown
Member Author

Landed in e76c49d, 69f653d

@nodejs/lts The first commit here (e76c49d) should make it to v4.x and v6.x, it fixes a bug from this month’s security release. I’ve added lts-watch labels, is that enough? Unfortunately the tests here themselves depend on v8.x-only features of Node. (Sorry for the mess, I didn’t plan it that way when I opened the PR. 😄)

All reactions

Sorry, something went wrong.

Uh oh!

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

@gibfahn gibfahn removed the dont-land-on-v6.x label Jul 24, 2017
addaleax added a commit that referenced this pull request Jul 24, 2017
@addaleax
deps: cherry-pick 18ea996 from c-ares upstream
d9273ed
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit that referenced this pull request Jul 24, 2017
@addaleax
test: add non-internet resolveAny tests
0418a70
This is a bit of a check to see how people feel about having this kind
of test.

Ref: #13137
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
v8.3.0 proposal #14452
Closed
@addaleax addaleax mentioned this pull request Aug 2, 2017
v8.3.0 proposal #14594
Merged
@MylesBorins MylesBorins added backported-to-v4.x and removed lts-watch-v4.x labels Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@addaleax @MylesBorins
deps: cherry-pick 18ea996 from c-ares upstream
dabc7c3
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@addaleax @MylesBorins
deps: cherry-pick 18ea996 from c-ares upstream
75b4e30
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
v6.11.3-proposal #14852
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@addaleax @MylesBorins
deps: cherry-pick 18ea996 from c-ares upstream
6e65501
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
@addaleax @MylesBorins
deps: cherry-pick 18ea996 from c-ares upstream
e16ad2e
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@addaleax @MylesBorins
deps: cherry-pick 18ea996 from c-ares upstream
987332a
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
This was referenced Sep 5, 2017
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#139
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#140
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#141
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#142
Closed
2017-09-05 Version 6.11.3 'Boron' (LTS) @MylesBorins beevelop/docker-android-nodejs#143
Closed
@rvagg rvagg mentioned this pull request Sep 13, 2017
cares: sync with upstream, fully adopt v1.13.0 #15378
Closed
rvagg pushed a commit to rvagg/io.js that referenced this pull request Sep 13, 2017
@addaleax @rvagg
deps: cherry-pick 18ea996 from c-ares upstream
2e1ba55
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: nodejs#13883
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
v4.8.5 proposal #15507
Closed
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
@addaleax @BridgeAR
deps: cherry-pick 18ea996 from c-ares upstream
9a0631d
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
@addaleax
deps: cherry-pick 18ea996 from c-ares upstream
b4c79a2
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: nodejs/node#13883
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@addaleax @MylesBorins
deps: cherry-pick 18ea996 from c-ares upstream
ed6298c
Original commit message:

    ares_parse_naptr_reply: make buffer length check more accurate

    9478908a490a6bf009ba58d81de8c1d06d50a117 introduced a length check
    for records parsed by `ares_parse_naptr_reply()`. However, that
    function is designed to parse replies which also contain non-NAPTR
    records; for A records, the `rr_len > 7` check will fail as there
    are only 4 bytes of payload.
    In particular, parsing ANY replies for NAPTR records was broken
    by that patch.

    Fix that by moving the check into the case in which it is already
    known that the record is a NAPTR record.

Ref: c-ares/c-ares@18ea996
PR-URL: #13883
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 25, 2017
v4.8.6 proposal #16500
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
@jasnell jasnell jasnell approved these changes
@cjihrig cjihrig cjihrig approved these changes
+1 more reviewer
@refack refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

dns Issues and PRs related to the dns subsystem. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

8 participants

@addaleax @refack @Trott @gibfahn @jasnell @cjihrig @MylesBorins @nodejs-github-bot
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.