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: add verbatim option to dns.lookup() #14731

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/api/dns.md
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,12 @@ changes:
flags may be passed by bitwise `OR`ing their values.
- `all` {boolean} When `true`, the callback returns all resolved addresses in
an array. Otherwise, returns a single address. Defaults to `false`.
- `verbatim` {boolean} When `true`, the callback receives IPv4 and IPv6
addresses in the order the DNS resolver returned them. When `false`,
IPv4 addresses are placed before IPv6 addresses.
Default: currently `false` (addresses are reordered) but this is expected
to change in the not too distant future.
New code should use `{ verbatim: true }`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on preferring verbatim: true, but shouldn't we add a runtime deprecation of the old default?

Copy link
Member

Choose a reason for hiding this comment

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

That seems like a separate question to "should this be an option". This is semver-minor (I think), and could be backported to v6.x. A runtime deprecation would be major.

Copy link
Member

Choose a reason for hiding this comment

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

Documentation-only deprecation would be better, I think. We're not likely to get rid of the default, just change it. A runtime deprecation would be too noisy.

- `callback` {Function}
- `err` {Error}
- `address` {string} A string representation of an IPv4 or IPv6 address.
Expand Down
4 changes: 3 additions & 1 deletion lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ function lookup(hostname, options, callback) {
var hints = 0;
var family = -1;
var all = false;
var verbatim = false;

// Parse arguments
if (hostname && typeof hostname !== 'string') {
Expand All @@ -141,6 +142,7 @@ function lookup(hostname, options, callback) {
hints = options.hints >>> 0;
family = options.family >>> 0;
all = options.all === true;
verbatim = options.verbatim === true;

if (hints !== 0 &&
hints !== cares.AI_ADDRCONFIG &&
Expand Down Expand Up @@ -181,7 +183,7 @@ function lookup(hostname, options, callback) {
req.hostname = hostname;
req.oncomplete = all ? onlookupall : onlookup;

var err = cares.getaddrinfo(req, hostname, family, hints);
var err = cares.getaddrinfo(req, hostname, family, hints, verbatim);
if (err) {
process.nextTick(callback, errnoException(err, 'getaddrinfo', hostname));
return {};
Expand Down
89 changes: 33 additions & 56 deletions src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,15 +186,23 @@ void ChannelWrap::New(const FunctionCallbackInfo<Value>& args) {

class GetAddrInfoReqWrap : public ReqWrap<uv_getaddrinfo_t> {
public:
GetAddrInfoReqWrap(Environment* env, Local<Object> req_wrap_obj);
GetAddrInfoReqWrap(Environment* env,
Local<Object> req_wrap_obj,
bool verbatim);
~GetAddrInfoReqWrap();

size_t self_size() const override { return sizeof(*this); }
bool verbatim() const { return verbatim_; }

private:
const bool verbatim_;
};

GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env,
Local<Object> req_wrap_obj)
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETADDRINFOREQWRAP) {
Local<Object> req_wrap_obj,
bool verbatim)
: ReqWrap(env, req_wrap_obj, AsyncWrap::PROVIDER_GETADDRINFOREQWRAP)
, verbatim_(verbatim) {
Wrap(req_wrap_obj, this);
}

Expand Down Expand Up @@ -1808,70 +1816,38 @@ void AfterGetAddrInfo(uv_getaddrinfo_t* req, int status, struct addrinfo* res) {
};

if (status == 0) {
// Success
struct addrinfo *address;
int n = 0;

// Create the response array.
Local<Array> results = Array::New(env->isolate());

char ip[INET6_ADDRSTRLEN];
const char *addr;

// Iterate over the IPv4 responses again this time creating javascript
// strings for each IP and filling the results array.
address = res;
while (address) {
CHECK_EQ(address->ai_socktype, SOCK_STREAM);

// Ignore random ai_family types.
if (address->ai_family == AF_INET) {
// Juggle pointers
addr = reinterpret_cast<char*>(&(reinterpret_cast<struct sockaddr_in*>(
address->ai_addr)->sin_addr));
int err = uv_inet_ntop(address->ai_family,
addr,
ip,
INET6_ADDRSTRLEN);
if (err)
auto add = [&] (bool want_ipv4, bool want_ipv6) {
for (auto p = res; p != nullptr; p = p->ai_next) {
CHECK_EQ(p->ai_socktype, SOCK_STREAM);

const char* addr;
if (want_ipv4 && p->ai_family == AF_INET) {
addr = reinterpret_cast<char*>(
&(reinterpret_cast<struct sockaddr_in*>(p->ai_addr)->sin_addr));
} else if (want_ipv6 && p->ai_family == AF_INET6) {
addr = reinterpret_cast<char*>(
&(reinterpret_cast<struct sockaddr_in6*>(p->ai_addr)->sin6_addr));
} else {
continue;
}

// Create JavaScript string
Local<String> s = OneByteString(env->isolate(), ip);
results->Set(n, s);
n++;
}

// Increment
address = address->ai_next;
}

// Iterate over the IPv6 responses putting them in the array.
address = res;
while (address) {
CHECK_EQ(address->ai_socktype, SOCK_STREAM);

// Ignore random ai_family types.
if (address->ai_family == AF_INET6) {
// Juggle pointers
addr = reinterpret_cast<char*>(&(reinterpret_cast<struct sockaddr_in6*>(
address->ai_addr)->sin6_addr));
int err = uv_inet_ntop(address->ai_family,
addr,
ip,
INET6_ADDRSTRLEN);
if (err)
char ip[INET6_ADDRSTRLEN];
if (uv_inet_ntop(p->ai_family, addr, ip, sizeof(ip)))
continue;

// Create JavaScript string
Local<String> s = OneByteString(env->isolate(), ip);
results->Set(n, s);
n++;
}
};

// Increment
address = address->ai_next;
}
const bool verbatim = req_wrap->verbatim();
add(true, verbatim);
if (verbatim == false)
add(false, true);

// No responses were found to return
if (n == 0) {
Expand Down Expand Up @@ -1962,6 +1938,7 @@ void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsObject());
CHECK(args[1]->IsString());
CHECK(args[2]->IsInt32());
CHECK(args[4]->IsBoolean());
Local<Object> req_wrap_obj = args[0].As<Object>();
node::Utf8Value hostname(env->isolate(), args[1]);

Expand All @@ -1982,7 +1959,7 @@ void GetAddrInfo(const FunctionCallbackInfo<Value>& args) {
CHECK(0 && "bad address family");
}

GetAddrInfoReqWrap* req_wrap = new GetAddrInfoReqWrap(env, req_wrap_obj);
auto req_wrap = new GetAddrInfoReqWrap(env, req_wrap_obj, args[4]->IsTrue());

struct addrinfo hints;
memset(&hints, 0, sizeof(struct addrinfo));
Expand Down
2 changes: 1 addition & 1 deletion test/internet/test-dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ console.log('looking up nodejs.org...');

const cares = process.binding('cares_wrap');
const req = new cares.GetAddrInfoReqWrap();
cares.getaddrinfo(req, 'nodejs.org', 4);
cares.getaddrinfo(req, 'nodejs.org', 4, /* hints */ 0, /* verbatim */ true);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could compare results to python -c "import socket ; a = socket.getaddrinfo('nodejs.org', None); print(a)"

Copy link
Member Author

Choose a reason for hiding this comment

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

That assumes python's implementation is exactly the same as node's, though. If it's not, or worse, not always, it's going to be a right pain in the neck to debug.

If you still think it's a good idea, can you file a separate issue to hash out the details?


req.oncomplete = function(err, domains) {
assert.strictEqual(err, 0);
Expand Down