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

fix(node:dns): fix crash and compatibility issues with resolve #5200

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

Hanaasagi
Copy link
Collaborator

What does this PR do?

1. long string could lead to a segfault.

Segmentfault code:

const dns = require("node:dns");

dns.resolveCname("a".repeat(3000), (err, results) => {});

null should not be used here as it can lead to a crash. (Line 530)

bun/src/deps/c_ares.zig

Lines 526 to 541 in 3b2c094

pub fn resolve(this: *Channel, name: []const u8, comptime lookup_name: []const u8, comptime Type: type, ctx: *Type, comptime cares_type: type, comptime callback: cares_type.Callback(Type)) void {
var name_buf: [1024]u8 = undefined;
const name_ptr: ?[*:0]const u8 = brk: {
if (name.len == 0 or name.len >= 1023) {
break :brk null;
}
const len = @min(name.len, name_buf.len - 1);
@memcpy(name_buf[0..len], name[0..len]);
name_buf[len] = 0;
break :brk name_buf[0..len :0];
};
const field_name = comptime std.fmt.comptimePrint("ns_t_{s}", .{lookup_name});
ares_query(this, name_ptr, NSClass.ns_c_in, @field(NSType, field_name), cares_type.callbackWrapper(lookup_name, Type, callback), ctx);
}

2. Fix a compatibility issue (resolveNs and resolveSoa)

const dns = require("node:dns");
for (const f of [dns.resolveNs, dns.resolveSoa]) {
  f("", (err, results) => {
    console.log(f, results);
  });
}

// nodejs output
// [Function: bound queryNs] [
//   'h.root-servers.net',
//   'j.root-servers.net',
//   'k.root-servers.net',
//   'd.root-servers.net',
//   'b.root-servers.net',
//   'f.root-servers.net',
//   'l.root-servers.net',
//   'e.root-servers.net',
//   'i.root-servers.net',
//   'a.root-servers.net',
//   'm.root-servers.net',
//   'g.root-servers.net',
//   'c.root-servers.net'
// ]
// [Function: bound querySoa] {
//   nsname: 'a.root-servers.net',
//   hostmaster: 'nstld.verisign-grs.com',
//   serial: 2023091202,
//   refresh: 1800,
//   retry: 900,
//   expire: 604800,
//   minttl: 86400
// }

bun doesn't allow empty strings to be passed into these two functions.


  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote automated tests

  • I ran make js and committed the transpiled changes
  • I or my editor ran Prettier on the changed files (or I ran bun fmt)
  • I included a test for the new code, or an existing test covers it
  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I or my editor ran zig fmt on the changed files
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed

@@ -524,11 +524,12 @@ pub const Channel = opaque {
}

pub fn resolve(this: *Channel, name: []const u8, comptime lookup_name: []const u8, comptime Type: type, ctx: *Type, comptime cares_type: type, comptime callback: cares_type.Callback(Type)) void {
if (name.len >= 1023 or (name.len == 0 and !(bun.strings.eqlComptime(lookup_name, "ns") or bun.strings.eqlComptime(lookup_name, "soa")))) {
return cares_type.callbackWrapper(lookup_name, Type, callback).?(ctx, ARES_EBADNAME, 0, null, 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ARES_EBADNAME The query name name could not be encoded as a domain name, either because it contained a zero-length label or because it contained a label of more than 63 characters.

Ref: https://c-ares.org/ares_query.html

if (name.len >= 1023 or (name.len == 0 and !(bun.strings.eqlComptime(lookup_name, "ns") or bun.strings.eqlComptime(lookup_name, "soa")))) {
return cares_type.callbackWrapper(lookup_name, Type, callback).?(ctx, ARES_EBADNAME, 0, null, 0);
}

var name_buf: [1024]u8 = undefined;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe a smaller size here

@Jarred-Sumner Jarred-Sumner merged commit 03d9bcd into oven-sh:main Sep 14, 2023
@Jarred-Sumner
Copy link
Collaborator

Thank you

paperdave pushed a commit to SuperAuguste/bun that referenced this pull request Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants