diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index d933138e1e8e..91fee4826730 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -650,6 +650,29 @@ impl c_ares::HostentHandler for GetHostByAddrInfoRequest { } } +/// Resolve a DNS request promise with the converted result, or reject it with +/// the conversion error. An exception left pending by the settle is reported +/// as unhandled so the drain loops never carry it into the next settle. +fn settle_lookup_promise( + promise: &mut JSPromiseStrong, + global_this: &JSGlobalObject, + result: JsResult, +) { + let settled = match result { + Ok(value) => promise.resolve_task(global_this, value), + Err(err) => promise.swap().reject(global_this, Err(err)), + }; + if settled.is_err() { + global_this.report_active_exception_as_unhandled(jsc::JsError::Thrown); + } +} + +fn ensure_result_still_alive(result: JsResult) { + if let Ok(value) = result { + value.ensure_still_alive(); + } +} + // ────────────────────────────────────────────────────────────────────────── // CAresNameInfo // ────────────────────────────────────────────────────────────────────────── @@ -725,19 +748,18 @@ impl CAresNameInfo { } return; }; - let array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, global_this) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + let array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, global_this); // SAFETY: see fn contract. unsafe { Self::on_complete(this, array) }; } /// SAFETY: see `process_resolve`. - unsafe fn on_complete(this: *mut Self, result: JSValue) { + unsafe fn on_complete(this: *mut Self, result: JsResult) { // SAFETY: see fn contract — `this` is a live node. let mut promise = unsafe { core::mem::take(&mut (*this).promise) }; // SAFETY: see fn contract — `this` is a live node. let global_this = unsafe { (*this).global_this() }; - let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards + settle_lookup_promise(&mut promise, global_this, result); // SAFETY: see fn contract. unsafe { Self::destroy(this) }; } @@ -1504,19 +1526,18 @@ impl CAresReverse { return; }; // node is a valid c-ares hostent for the callback's duration - let array = super::cares_jsc::hostent_to_js_response(&mut *node, global_this, b"") - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + let array = super::cares_jsc::hostent_to_js_response(&mut *node, global_this, b""); Self::on_complete(this, array); } } /// SAFETY: see `process_resolve`. - unsafe fn on_complete(this: *mut Self, result: JSValue) { + unsafe fn on_complete(this: *mut Self, result: JsResult) { // SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request. unsafe { let mut promise = core::mem::take(&mut (*this).promise); let global_this = (*this).global_this(); - let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards + settle_lookup_promise(&mut promise, global_this, result); if let Some(resolver) = (*this).resolver.as_ref() { // IntrusiveRc holds a live ref; request_completed mutates pending_requests counter only. (*resolver.as_ptr()).request_completed(); @@ -1653,20 +1674,18 @@ impl CAresLookup { }; // node is a valid c-ares reply for the callback's duration; freed by `_free` guard. - let array = (*node) - .to_js_response(global_this, T::TYPE_NAME) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + let array = (*node).to_js_response(global_this, T::TYPE_NAME); Self::on_complete(this, array); } } /// SAFETY: see `process_resolve`. - unsafe fn on_complete(this: *mut Self, result: JSValue) { + unsafe fn on_complete(this: *mut Self, result: JsResult) { // SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request. unsafe { let mut promise = core::mem::take(&mut (*this).promise); let global_this = (*this).global_this(); - let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards + settle_lookup_promise(&mut promise, global_this, result); if let Some(resolver) = (*this).resolver.as_ref() { // IntrusiveRc holds a live ref; request_completed mutates pending_requests counter only. (*resolver.as_ptr()).request_completed(); @@ -1753,10 +1772,19 @@ impl DNSLookup { bun_output::scoped_log!(DNSLookup, "onCompleteNative"); // SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request. unsafe { - let array = super::options_jsc::result_any_to_js(result, (*this).global_this()) - .ok() - .flatten() - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + let Some(array) = + super::options_jsc::result_any_to_js(result, (*this).global_this()).transpose() + else { + error_to_deferred( + c_ares::Error::ENOTFOUND, + b"getaddrinfo", + None, + &mut (*this).promise, + ) + .reject_later((*this).global_this()); + Self::destroy(this); + return; + }; Self::on_complete_with_array(this, array); } } @@ -1827,20 +1855,19 @@ impl DNSLookup { // owned by the caller's scopeguard; JSGlobalObject outlives the request. unsafe { let array = - super::cares_jsc::addr_info_to_js_array(&mut *result, (*this).global_this()) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + super::cares_jsc::addr_info_to_js_array(&mut *result, (*this).global_this()); Self::on_complete_with_array(this, array); } } /// SAFETY: see `on_complete_native`. - unsafe fn on_complete_with_array(this: *mut Self, result: JSValue) { + unsafe fn on_complete_with_array(this: *mut Self, result: JsResult) { bun_output::scoped_log!(DNSLookup, "onCompleteWithArray"); // SAFETY: caller contract — `this` is live; JSGlobalObject outlives the request. unsafe { let mut promise = core::mem::take(&mut (*this).promise); let global_this = (*this).global_this(); - let _ = promise.resolve_task(global_this, result); // TODO: properly propagate exception upwards + settle_lookup_promise(&mut promise, global_this, result); if let Some(resolver) = (*this).resolver.as_ref() { // IntrusiveRc holds a live ref; request_completed mutates pending_requests counter only. (*resolver.as_ptr()).request_completed(); @@ -4185,30 +4212,27 @@ impl Resolver { unsafe { let mut pending = (*key.lookup).head.next; let mut prev_global = (*key.lookup).head.global_this(); - let mut array = (*addr) - .to_js_response(prev_global, T::TYPE_NAME) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + let mut array = (*addr).to_js_response(prev_global, T::TYPE_NAME); // SAFETY: addr is the c-ares-allocated reply; freed once after all consumers run. let _free_addr = scopeguard::guard(addr, |a| T::destroy(a)); - array.ensure_still_alive(); + ensure_result_still_alive(array); CAresLookup::::on_complete(ptr::addr_of_mut!((*key.lookup).head), array); drop(bun_core::heap::take(key.lookup)); - array.ensure_still_alive(); + ensure_result_still_alive(array); while let Some(value) = pending { let new_global = (*value.as_ptr()).global_this(); - if !core::ptr::eq(prev_global, new_global) { - array = (*addr) - .to_js_response(new_global, T::TYPE_NAME) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + // The settle consumed an Err's pending exception; never reuse it. + if array.is_err() || !core::ptr::eq(prev_global, new_global) { + array = (*addr).to_js_response(new_global, T::TYPE_NAME); prev_global = new_global; } pending = (*value.as_ptr()).next; - array.ensure_still_alive(); + ensure_result_still_alive(array); CAresLookup::::on_complete(value.as_ptr(), array); - array.ensure_still_alive(); + ensure_result_still_alive(array); } } } @@ -4251,29 +4275,28 @@ impl Resolver { unsafe { let mut pending = (*key.lookup).head.next; let mut prev_global = (*key.lookup).head.global_this(); - let mut array = super::cares_jsc::addr_info_to_js_array(&mut *addr, prev_global) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + let mut array = super::cares_jsc::addr_info_to_js_array(&mut *addr, prev_global); // SAFETY: addr is the c-ares-allocated AddrInfo; freed once after all consumers run. // Move the raw pointer into the guard so the loop body can keep borrowing `*addr`. let _free_addr = scopeguard::guard(addr, |a| c_ares::AddrInfo::destroy(a)); - array.ensure_still_alive(); + ensure_result_still_alive(array); DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), array); drop(bun_core::heap::take(key.lookup)); - array.ensure_still_alive(); + ensure_result_still_alive(array); while let Some(value) = pending { let new_global = (*value.as_ptr()).global_this(); - if !core::ptr::eq(prev_global, new_global) { - array = super::cares_jsc::addr_info_to_js_array(&mut *addr, new_global) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + // The settle consumed an Err's pending exception; never reuse it. + if array.is_err() || !core::ptr::eq(prev_global, new_global) { + array = super::cares_jsc::addr_info_to_js_array(&mut *addr, new_global); prev_global = new_global; } pending = (*value.as_ptr()).next; - array.ensure_still_alive(); + ensure_result_still_alive(array); DNSLookup::on_complete_with_array(value.as_ptr(), array); - array.ensure_still_alive(); + ensure_result_still_alive(array); } } } @@ -4291,33 +4314,25 @@ impl Resolver { // SAFETY: `self` is the live heap allocation; ref_scope keeps count > 0 across re-entrant callbacks. let _g = unsafe { Self::ref_scope(self.as_ctx_ptr()) }; - let mut array: JSValue = match super::options_jsc::result_any_to_js(result, global_object) - .unwrap_or(None) - { - // TODO: properly propagate exception upwards - Some(a) => a, - None => { - // SAFETY: `key.lookup` is the heap-allocated request stored in the - // pending-cache slot; consumed via `heap::take` below. - unsafe { - let mut pending = (*key.lookup).head.next; - // Consume the request and move `head` out by value; - // `ptr::read` + `heap::take` would double-Drop `DNSLookup`. - let owned = *bun_core::heap::take(key.lookup); - let mut head = owned.head; - DNSLookup::process_get_addr_info_native(&raw mut head, err, ptr::null_mut()); + let Some(mut array) = + super::options_jsc::result_any_to_js(result, global_object).transpose() + else { + // SAFETY: `key.lookup` is the heap-allocated request stored in the + // pending-cache slot; consumed via `heap::take` below. + unsafe { + let mut pending = (*key.lookup).head.next; + // Consume the request and move `head` out by value; + // `ptr::read` + `heap::take` would double-Drop `DNSLookup`. + let owned = *bun_core::heap::take(key.lookup); + let mut head = owned.head; + DNSLookup::process_get_addr_info_native(&raw mut head, err, ptr::null_mut()); - while let Some(value) = pending { - pending = (*value.as_ptr()).next; - DNSLookup::process_get_addr_info_native( - value.as_ptr(), - err, - ptr::null_mut(), - ); - } + while let Some(value) = pending { + pending = (*value.as_ptr()).next; + DNSLookup::process_get_addr_info_native(value.as_ptr(), err, ptr::null_mut()); } - return; } + return; }; // SAFETY: `key.lookup` is the heap-allocated request stored in the // pending-cache slot; consumed via `heap::take` below. @@ -4326,25 +4341,27 @@ impl Resolver { let mut prev_global = (*key.lookup).head.global_this(); { - array.ensure_still_alive(); + ensure_result_still_alive(array); DNSLookup::on_complete_with_array(ptr::addr_of_mut!((*key.lookup).head), array); drop(bun_core::heap::take(key.lookup)); - array.ensure_still_alive(); + ensure_result_still_alive(array); } while let Some(value) = pending { let new_global = (*value.as_ptr()).global_this(); pending = (*value.as_ptr()).next; - if !core::ptr::eq(prev_global, new_global) { + // The settle consumed an Err's pending exception; never reuse it. + if array.is_err() || !core::ptr::eq(prev_global, new_global) { + // The head conversion proved `result` is non-null. array = super::options_jsc::result_any_to_js(result, new_global) - .unwrap_or(None) - .unwrap(); // TODO: properly propagate exception upwards + .transpose() + .unwrap(); prev_global = new_global; } - array.ensure_still_alive(); + ensure_result_still_alive(array); DNSLookup::on_complete_with_array(value.as_ptr(), array); - array.ensure_still_alive(); + ensure_result_still_alive(array); } } } @@ -4390,26 +4407,25 @@ impl Resolver { // The callback need not and should not attempt to free the memory // pointed to by hostent; the ares library will free it when the // callback returns. - let mut array = super::cares_jsc::hostent_to_js_response(&mut *addr, prev_global, b"") - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards - array.ensure_still_alive(); + let mut array = super::cares_jsc::hostent_to_js_response(&mut *addr, prev_global, b""); + ensure_result_still_alive(array); CAresReverse::on_complete(ptr::addr_of_mut!((*key.lookup).head), array); drop(bun_core::heap::take(key.lookup)); - array.ensure_still_alive(); + ensure_result_still_alive(array); while let Some(value) = pending { let new_global = (*value.as_ptr()).global_this(); - if !core::ptr::eq(prev_global, new_global) { - array = super::cares_jsc::hostent_to_js_response(&mut *addr, new_global, b"") - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + // The settle consumed an Err's pending exception; never reuse it. + if array.is_err() || !core::ptr::eq(prev_global, new_global) { + array = super::cares_jsc::hostent_to_js_response(&mut *addr, new_global, b""); prev_global = new_global; } pending = (*value.as_ptr()).next; - array.ensure_still_alive(); + ensure_result_still_alive(array); CAresReverse::on_complete(value.as_ptr(), array); - array.ensure_still_alive(); + ensure_result_still_alive(array); } } } @@ -4453,26 +4469,25 @@ impl Resolver { let mut pending = (*key.lookup).head.next; let mut prev_global = (*key.lookup).head.global_this(); - let mut array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, prev_global) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards - array.ensure_still_alive(); + let mut array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, prev_global); + ensure_result_still_alive(array); CAresNameInfo::on_complete(ptr::addr_of_mut!((*key.lookup).head), array); drop(bun_core::heap::take(key.lookup)); - array.ensure_still_alive(); + ensure_result_still_alive(array); while let Some(value) = pending { let new_global = (*value.as_ptr()).global_this(); - if !core::ptr::eq(prev_global, new_global) { - array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, new_global) - .unwrap_or(JSValue::ZERO); // TODO: properly propagate exception upwards + // The settle consumed an Err's pending exception; never reuse it. + if array.is_err() || !core::ptr::eq(prev_global, new_global) { + array = super::cares_jsc::nameinfo_to_js_response(&mut name_info, new_global); prev_global = new_global; } pending = (*value.as_ptr()).next; - array.ensure_still_alive(); + ensure_result_still_alive(array); CAresNameInfo::on_complete(value.as_ptr(), array); - array.ensure_still_alive(); + ensure_result_still_alive(array); } } } diff --git a/test/js/bun/dns/resolve-dns.test.ts b/test/js/bun/dns/resolve-dns.test.ts index 45066473c96a..145b9e8bace4 100644 --- a/test/js/bun/dns/resolve-dns.test.ts +++ b/test/js/bun/dns/resolve-dns.test.ts @@ -223,6 +223,62 @@ describe("dns", () => { expect(isIP(result[0].address)).toBeGreaterThan(0); }); + // The native completion callbacks settle several coalesced lookup promises + // in one task. Resolving a promise reads `then` off the result array, so a + // hostile `Object.prototype.then` accessor throws inside every resolve in + // the loop; each promise must reject with that error and the next promise + // in the drain must still settle, with no pending exception carried over. + test.concurrent("coalesced lookups settle when the result array is a hostile thenable", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + Object.defineProperty(Object.prototype, "then", { + configurable: true, + get() { + throw new Error("boom"); + }, + }); + const lookups = []; + for (let i = 0; i < 8; i++) { + lookups.push(Bun.dns.lookup("localhost", { backend: "system" })); + lookups.push(Bun.dns.lookup("localhost", { backend: "libc" })); + lookups.push(Bun.dns.lookup("0.0.0.0", { backend: "c-ares" })); + } + // Promise.allSettled would resolve its result promise with an array + // and trip the accessor itself, so count rejections by hand. + let remaining = lookups.length; + for (const promise of lookups) { + promise.then( + () => { + console.log("expected rejection"); + process.exit(1); + }, + reason => { + if (reason?.message !== "boom") { + console.log("unexpected reason: " + reason); + process.exit(1); + } + if (--remaining === 0) { + delete Object.prototype.then; + console.log("ok"); + } + }, + ); + } + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(stdout.trim()).toBe("ok"); + expect(exitCode).toBe(0); + }); + describe("setServers", () => { test("triple with non-int32 family (double) throws TypeError", () => { // @ts-expect-error