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

test: add non-internet resolveAny tests #13883

Closed
wants to merge 2 commits into from

Conversation

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

@addaleax addaleax added dns Issues and PRs related to the dns subsystem. dont-land-on-v4.x test Issues and PRs related to the tests. labels Jun 22, 2017
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 22, 2017
@refack
Copy link
Contributor

refack commented Jun 22, 2017

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

@Trott
Copy link
Member

Trott commented Jun 23, 2017

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!

@addaleax
Copy link
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.

@gibfahn
Copy link
Member

gibfahn commented Jun 23, 2017

+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).

} else {
// Pointer to another part of the packet.
assert.strictEqual(length & 0xC0, 0xC0);
// eslint-disable-next-line
Copy link
Contributor

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?

Copy link
Member

@Trott Trott Jun 23, 2017

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.

Copy link
Member Author

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”).

Copy link
Contributor

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 😅


buffers.push(new Uint16Array([
parsed.id,
// default to “standard response”
Copy link
Contributor

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?

Copy link
Member Author

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

switch (rr.type) {
case 'A':
rdLengthBuf[0] = 4;
buffers.push(new Uint8Array(rr.address.split(/\./g)));
Copy link
Contributor

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.

Copy link
Member Author

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

@refack
Copy link
Contributor

refack commented Jun 26, 2017

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

} else {
// Pointer to another part of the packet.
assert.strictEqual(length & 0xC0, 0xC0);
// eslint-disable-next-line
Copy link
Contributor

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 😅

buffers.push(new Uint16Array([
parsed.id,
// default to “standard response”
parsed.flags === undefined ? 0x8180 : parsed.flags,
Copy link
Contributor

Choose a reason for hiding this comment

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

😵

Copy link
Member Author

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.

Copy link
Contributor

@refack refack Jun 26, 2017

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.


// Naïve DNS parser/serializer.

require('../common');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this?

Copy link
Member Author

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.

Copy link
Member

@Trott Trott Jun 27, 2017

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.

Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

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.

@refack
Copy link
Contributor

refack commented Jul 10, 2017

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.

@addaleax
Copy link
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.

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

Yeah, good point :-)

@addaleax
Copy link
Member Author

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

@addaleax
Copy link
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. 😄)

addaleax added a commit that referenced this pull request Jul 24, 2017
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
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
@addaleax addaleax mentioned this pull request Aug 2, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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
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
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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
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
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]>
rvagg pushed a commit to rvagg/io.js that referenced this pull request Sep 13, 2017
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
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
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
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
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
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants