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.

Sign up for GitHub

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

Jump to bottom

Fix crash in dns.lookup, ensure getaddrinfo() only returns IPv4-only or IPv6-only results when it should, normalize node:dns errors #12223

Merged
merged 6 commits into from
Jun 29, 2024
Merged

Fix crash in dns.lookup, ensure getaddrinfo() only returns IPv4-only or IPv6-only results when it should, normalize node:dns errors #12223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
25fb7e8
Fix crash in dns.lookup
Jarred-Sumner Jun 28, 2024
bf9b3df
More comments
Jarred-Sumner Jun 28, 2024
abe847e
Update dns_resolver.zig
Jarred-Sumner Jun 28, 2024
60158f3
Normalize error handling logic
Jarred-Sumner Jun 28, 2024
c75d413
Try to fix ubuntu failure
Jarred-Sumner Jun 28, 2024
960b241
Merge branch 'main' into jarred/fix-getaddrinfo-crash
Jarred-Sumner Jun 29, 2024
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
123 changes: 33 additions & 90 deletions src/bun.js/api/bun/dns_resolver.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ pub fn normalizeDNSName(name: []const u8, backend: *GetAddrInfo.Backend) []const
} else if (strings.isIPV6Address(name)) {
backend.* = .system;
}
// getaddrinfo() is inconsistent with ares_getaddrinfo() when using localhost
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really explain anything, what exactly is inconsistent?

else if (strings.eqlComptime(name, "localhost")) {
backend.* = .system;
}
}

return name;
Expand Down Expand Up @@ -488,29 +492,15 @@ pub const CAresNameInfo = struct {
pub fn processResolve(this: *@This(), err_: ?c_ares.Error, _: i32, result: ?c_ares.struct_nameinfo) void {
if (err_) |err| {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("lookupService failed: {s}", .{err.label()});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init(err.code()).toValueGC(globalThis),
);

promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, err.toJS(globalThis));
this.deinit();
return;
}
if (result == null) {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("lookupService failed: No results", .{});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init("EUNREACHABLE").toValueGC(globalThis),
);

promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, c_ares.Error.ENOTFOUND.toJS(globalThis));
this.deinit();
return;
}
Expand Down Expand Up @@ -906,29 +896,15 @@ pub const CAresReverse = struct {
pub fn processResolve(this: *@This(), err_: ?c_ares.Error, _: i32, result: ?*c_ares.struct_hostent) void {
if (err_) |err| {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("reverse failed: {s}", .{err.label()});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init(err.code()).toValueGC(globalThis),
);

promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, err.toJS(globalThis));
this.deinit();
return;
}
if (result == null) {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("reverse failed: No results", .{});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init("EUNREACHABLE").toValueGC(globalThis),
);

promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, c_ares.Error.ENOTFOUND.toJS(globalThis));
this.deinit();
return;
}
Expand Down Expand Up @@ -985,28 +961,15 @@ pub fn CAresLookup(comptime cares_type: type, comptime type_name: []const u8) ty
pub fn processResolve(this: *@This(), err_: ?c_ares.Error, _: i32, result: ?*cares_type) void {
if (err_) |err| {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("{s} lookup failed: {s}", .{ type_name, err.label() });
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init(err.code()).toValueGC(globalThis),
);

promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, err.toJS(globalThis));
this.deinit();
return;
}
if (result == null) {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("{s} lookup failed: {s}", .{ type_name, "No results" });
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init("EUNREACHABLE").toValueGC(globalThis),
);
promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, c_ares.Error.ENOTFOUND.toJS(globalThis));
this.deinit();
return;
}
Expand Down Expand Up @@ -1070,19 +1033,14 @@ pub const DNSLookup = struct {
log("processGetAddrInfoNative: status={d}", .{status});
if (c_ares.Error.initEAI(status)) |err| {
var promise = this.promise;
var globalThis = this.globalThis;
const globalThis = this.globalThis;

const error_value = brk: {
if (err == .ESERVFAIL) {
break :brk bun.sys.Error.fromCode(bun.C.getErrno(@as(c_int, -1)), .getaddrinfo).toJSC(globalThis);
}
const error_value = globalThis.createErrorInstance("DNS lookup failed: {s}", .{err.label()});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init(err.code()).toValueGC(globalThis),
);
break :brk error_value;

break :brk err.toJS(globalThis);
};

this.deinit();
Expand All @@ -1096,28 +1054,17 @@ pub const DNSLookup = struct {
log("processGetAddrInfo", .{});
if (err_) |err| {
var promise = this.promise;
var globalThis = this.globalThis;
const error_value = globalThis.createErrorInstance("DNS lookup failed: {s}", .{err.label()});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init(err.code()).toValueGC(globalThis),
);
promise.rejectTask(globalThis, error_value);
const globalThis = this.globalThis;
promise.rejectTask(globalThis, err.toJS(globalThis));
this.deinit();
return;
}

if (result == null or result.?.node == null) {
var promise = this.promise;
var globalThis = this.globalThis;
const globalThis = this.globalThis;

const error_value = globalThis.createErrorInstance("DNS lookup failed: {s}", .{"No results"});
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init("EUNREACHABLE").toValueGC(globalThis),
);
const error_value = c_ares.Error.ENOTFOUND.toJS(globalThis);
promise.rejectTask(globalThis, error_value);
this.deinit();
return;
Expand Down Expand Up @@ -1396,7 +1343,14 @@ pub const InternalDNS = struct {
.addrlen = 0,
.canonname = null,
.family = std.c.AF.UNSPEC,
.flags = 0,
// If the system is IPv4-only or IPv6-only, then only return the corresponding address family.
// https://github.com/nodejs/node/commit/54dd7c38e507b35ee0ffadc41a716f1782b0d32f
// https://bugzilla.mozilla.org/show_bug.cgi?id=467497
// https://github.com/adobe/chromium/blob/cfe5bf0b51b1f6b9fe239c2a3c2f2364da9967d7/net/base/host_resolver_proc.cc#L122-L241
// https://github.com/nodejs/node/issues/33816
// https://github.com/aio-libs/aiohttp/issues/5357
// https://github.com/libuv/libuv/issues/2225
.flags = if (Environment.isPosix) bun.C.netdb.AI_ADDRCONFIG else 0,
.next = null,
.protocol = 0,
.socktype = std.c.SOCK.STREAM,
Expand Down Expand Up @@ -2411,13 +2365,8 @@ pub const DNSResolver = struct {
var channel: *c_ares.Channel = switch (resolver.getChannel()) {
.result => |res| res,
.err => |err| {
const system_error = JSC.SystemError{
.errno = -1,
.code = bun.String.static(err.code()),
.message = bun.String.static(err.label()),
};
defer ip_slice.deinit();
globalThis.throwValue(system_error.toErrorInstance(globalThis));
globalThis.throwValue(err.toJS(globalThis));
return .zero;
},
};
Expand Down Expand Up @@ -2796,13 +2745,7 @@ pub const DNSResolver = struct {
var channel: *c_ares.Channel = switch (this.getChannel()) {
.result => |res| res,
.err => |err| {
const system_error = JSC.SystemError{
.errno = -1,
.code = bun.String.static(err.code()),
.message = bun.String.static(err.label()),
};

globalThis.throwValue(system_error.toErrorInstance(globalThis));
globalThis.throwValue(err.toJS(globalThis));
return .zero;
},
};
Expand Down
4 changes: 4 additions & 0 deletions src/darwin_c.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,3 +816,7 @@ pub const CLOCK_UPTIME_RAW = 8;
pub const CLOCK_UPTIME_RAW_APPROX = 9;
pub const CLOCK_PROCESS_CPUTIME_ID = 12;
pub const CLOCK_THREAD_CPUTIME_ID = 1;

pub const netdb = @cImport({
@cInclude("netdb.h");
});
57 changes: 52 additions & 5 deletions src/deps/c_ares.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ pub const AddrInfo = extern struct {
addr_info: *AddrInfo,
globalThis: *JSC.JSGlobalObject,
) JSC.JSValue {
var node = addr_info.node.?;
var node = addr_info.node orelse return JSC.JSValue.createEmptyArray(globalThis, 0);
const array = JSC.JSValue.createEmptyArray(
globalThis,
node.count(),
Expand Down Expand Up @@ -1316,8 +1316,33 @@ pub const Error = enum(i32) {
ECANCELLED = ARES_ECANCELLED,
ESERVICE = ARES_ESERVICE,

pub fn toJS(this: Error, globalThis: *JSC.JSGlobalObject) JSC.JSValue {
const error_value = globalThis.createErrorInstance("{s}", .{this.label()});
error_value.put(
globalThis,
JSC.ZigString.static("name"),
JSC.ZigString.init("DNSException").toValueGC(globalThis),
);
error_value.put(
globalThis,
JSC.ZigString.static("code"),
JSC.ZigString.init(this.code()).toValueGC(globalThis),
);
error_value.put(
globalThis,
JSC.ZigString.static("errno"),
JSC.jsNumber(@intFromEnum(this)),
);
return error_value;
}

pub fn initEAI(rc: i32) ?Error {
if (comptime bun.Environment.isWindows) {
// https://github.com/nodejs/node/blob/2eff28fb7a93d3f672f80b582f664a7c701569fb/lib/internal/errors.js#L807-L815
if (rc == libuv.UV_EAI_NODATA or rc == libuv.UV_EAI_NONAME) {
return Error.ENOTFOUND;
}

// TODO: revisit this
return switch (rc) {
0 => null,
Expand All @@ -1339,18 +1364,35 @@ pub const Error = enum(i32) {
};
}

return switch (@as(std.posix.system.EAI, @enumFromInt(rc))) {
const eai: std.posix.system.EAI = @enumFromInt(rc);

// https://github.com/nodejs/node/blob/2eff28fb7a93d3f672f80b582f664a7c701569fb/lib/internal/errors.js#L807-L815
if (eai == .NODATA or eai == .NONAME) {
return Error.ENOTFOUND;
}

if (comptime bun.Environment.isLinux) {
switch (eai) {
.SOCKTYPE => return Error.ECONNREFUSED,
.IDN_ENCODE => return Error.EBADSTR,
.ALLDONE => return Error.ENOTFOUND,
.INPROGRESS => return Error.ETIMEOUT,
.CANCELED => return Error.ECANCELLED,
.NOTCANCELED => return Error.ECANCELLED,
else => {},
}
}

return switch (eai) {
@as(std.posix.system.EAI, @enumFromInt(0)) => return null,
.ADDRFAMILY => Error.EBADFAMILY,
.BADFLAGS => Error.EBADFLAGS, // Invalid hints
.FAIL => Error.EBADRESP,
.FAMILY => Error.EBADFAMILY,
.MEMORY => Error.ENOMEM,
.NODATA => Error.ENODATA,
.NONAME => Error.ENONAME,
.SERVICE => Error.ESERVICE,
.SYSTEM => Error.ESERVFAIL,
else => unreachable,
else => bun.todo(@src(), Error.ENOTIMP),
};
}

Expand Down Expand Up @@ -1411,6 +1453,11 @@ pub const Error = enum(i32) {
});

pub fn get(rc: i32) ?Error {
// https://github.com/nodejs/node/blob/2eff28fb7a93d3f672f80b582f664a7c701569fb/lib/internal/errors.js#L807-L815
if (rc == ARES_ENODATA or rc == ARES_ENONAME) {
return get(ARES_ENOTFOUND);
}

return switch (rc) {
0 => null,
1...ARES_ESERVICE => @as(Error, @enumFromInt(rc)),
Expand Down
10 changes: 8 additions & 2 deletions src/dns.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ pub const GetAddrInfo = struct {

pub const Options = packed struct {
family: Family = .unspecified,
socktype: SocketType = .unspecified,
/// Leaving this unset leads to many duplicate addresses returned.
/// Node hardcodes to `SOCK_STREAM`.
/// There don't seem to be any issues in Node's repo about this
/// So I think it's likely that nobody actually needs `SOCK_DGRAM` as a flag
/// https://github.com/nodejs/node/blob/2eff28fb7a93d3f672f80b582f664a7c701569fb/src/cares_wrap.cc#L1609
socktype: SocketType = .stream,
protocol: Protocol = .unspecified,
backend: Backend = Backend.default,
flags: i32 = 0,
Expand Down Expand Up @@ -171,7 +176,8 @@ pub const GetAddrInfo = struct {

pub fn fromJS(value: JSC.JSValue, globalObject: *JSC.JSGlobalObject) !SocketType {
if (value.isEmptyOrUndefinedOrNull())
return .unspecified;
// Default to .stream
return .stream;

if (value.isNumber()) {
return switch (value.to(i32)) {
Expand Down
26 changes: 21 additions & 5 deletions src/js/node/dns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ function lookup(domain, options, callback) {
}

dns.lookup(domain, options).then(res => {
throwIfEmpty(res);
res.sort((a, b) => a.family - b.family);

if (options?.all) {
Expand Down Expand Up @@ -333,18 +334,33 @@ var {
function setDefaultResultOrder() {}
function setServers() {}

const promisifyLookup = res => {
res.sort((a, b) => a.family - b.family);
const [{ address, family }] = res;
const mapLookupAll = res => {
const { address, family } = res;
return { address, family };
};

const mapLookupAll = res => {
const { address, family } = res;
function throwIfEmpty(res) {
if (res.length === 0) {
const err = new Error("No records found");
err.name = "DNSException";
err.code = "ENODATA";
// Hardcoded errno
err.errno = 1;
err.syscall = "getaddrinfo";
throw err;
}
}
Object.defineProperty(throwIfEmpty, "name", { value: "::bunternal::" });

const promisifyLookup = res => {
throwIfEmpty(res);
res.sort((a, b) => a.family - b.family);
const [{ address, family }] = res;
return { address, family };
};

const promisifyLookupAll = res => {
throwIfEmpty(res);
res.sort((a, b) => a.family - b.family);
return res.map(mapLookupAll);
};
Expand Down
4 changes: 4 additions & 0 deletions src/linux_c.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,3 +698,7 @@ pub const RENAME_WHITEOUT = 1 << 2;

pub extern "C" fn quick_exit(code: c_int) noreturn;
pub extern "C" fn memrchr(ptr: [*]const u8, val: c_int, len: usize) ?[*]const u8;

pub const netdb = @cImport({
@cInclude("netdb.h");
});
Loading
Loading