deps: update c-ares to v1.34.8 - #34007
Conversation
|
Updated 10:27 PM PT - Aug 8th, 2026
✅ @robobun, your commit ddb9b5dab942a8357303dd99726755016f23f6e0 passed in 🧪 To try this PR locally: bunx bun-pr 34007That installs a local version of the PR into your bun-34007 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe c-ares dependency now applies a parser patch that accepts compressed RDATA names. DNS integration tests verify compressed SRV target resolution, and the process version test records the new c-ares revision. Changesc-ares compressed RDATA support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
69263b2 to
08eea0d
Compare
f4b7ed8 to
ebd29aa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/deps/cares.ts`:
- Line 15: Investigate the c-ares update identified by CARES_COMMIT and measure
exact binary-size deltas for each target against the 524,288-byte gate,
attributing changes to c-ares objects or symbols. Reduce or revert the change if
the regression is unintentional; if intentional, document the cause and only
then add the size-check bypass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39bfb5db-e743-4288-a7d0-e0327d234e7b
📒 Files selected for processing (1)
scripts/build/deps/cares.ts
|
@robobun adopt |
|
✅ Adopted. c-ares v1.34.8 started rejecting DNS name compression in SRV RDATA (c-ares/c-ares#1190), which breaks Note: the weekly update workflow force-pushed this branch on Aug 9 and wiped the fix; re-applied in ddb9b5d.
Previous full run (build #87633) was green on all c-ares-related tests across every lane including macOS. |
f5fc1bc to
414036c
Compare
c-ares 1.34.8 (c-ares/c-ares#1190) started rejecting DNS name compression in RDATA for non-RFC1035 types. That is the letter of RFC 3597, but a lot of deployed resolvers and caches still compress SRV/NAPTR targets, so dns.resolveSrv() fails with EBADRESP against them (this is what hits the macOS lanes on the mongodb and node-dns tests). Add a patch that keeps the parser lenient while leaving the writer strict, update the expected process.versions hash, and add a local-server test that feeds a compressed SRV target so the regression is caught without needing a specific upstream resolver.
There was a problem hiding this comment.
All three issues I flagged earlier are now resolved in ddb9b5d — the process.versions.ares hash is updated, patches/cares/accept-rdata-compression.patch is present and wired via the patches: field (same pattern as highway/libuv/tinycc/etc.), and the compressed-SRV local-UDP regression test is in place. No new issues found. Since this carries a downstream patch to c-ares's DNS wire-format parser, a human sign-off on the decision to diverge from upstream would still be worthwhile.
What was reviewed:
- Patch restores pre-1.34.8 receive-side behavior (
ARES_TRUEforallow_compression); writer stays gated onares_dns_rec_allow_name_comp, matching RFC 3597 §4's sender/receiver split. patches:field consumed atscripts/build/source.ts:731— patch will be applied on fetch.- New test's DNS packet is well-formed (header/question/answer with
0xc0 0x0cpointer in RDATA); mirrors the existing CAA local-server test pattern, hermetic,try/finallycloses the socket. node-dns.test.jsalready importsdgramandonce;isWindowsskip matches the sibling CAA test.
Extended reasoning...
Overview
This PR bumps the vendored c-ares from 3ac47ee4 to c7a3138d (v1.34.8), adds a downstream patch patches/cares/accept-rdata-compression.patch to keep the DNS RDATA name parser lenient about compression pointers (reverting the receive-side half of c-ares/c-ares#1190), wires the patch into scripts/build/deps/cares.ts via the standard patches: field, updates the expected process.versions.ares hash in process.test.js, and adds a hermetic local-UDP-server regression test in node-dns.test.js that serves a compressed SRV target.
I previously flagged three issues on this PR across earlier revisions: (1) the missing process.versions hash update, (2) the SRV/NAPTR RDATA-compression regression from c-ares PR #1190, and (3) the fact that the fix commits had been dropped by a workflow force-push. All three are verifiably present at the current head (ddb9b5d): the patch file exists, cares.ts:61 has the patches: entry, process.test.js:584 has the new hash, and the compressed-SRV test is in the diff.
Security risks
The patch touches parsing of untrusted DNS wire data. However, it does not introduce new parsing logic — it restores the exact behavior c-ares had prior to v1.34.7 by passing ARES_TRUE to ares_dns_name_parse()'s allow_compression parameter. That code path (following compression pointers with loop detection) is the same one that still runs unconditionally for CNAME/MX/NS/SOA/PTR RDATA and for every record's owner name, so it is not new attack surface. The reasoning is sound per RFC 3597 §4 (receivers SHOULD decompress SRV/NAPTR; only senders are restricted). No injection, auth, or data-exposure surface is touched.
Level of scrutiny
Medium-to-high. The version bump, hash update, and patches: wiring are mechanical and follow established patterns (verified against highway, libuv, tinycc, boringssl, lshpack, zlib, lsquic, libarchive — all use the same patches: [...] shape consumed at source.ts:731). The new test is hermetic and mirrors the existing CAA local-server test in the same file. However, the repo's review guidance explicitly flags "Dependencies & vendoring" as a situational area, and carrying a downstream patch that diverges from upstream's deliberate RFC-strictness is a maintenance decision (future c-ares bumps will need to keep or rebase this patch) that a maintainer should ratify.
Other factors
Jarred engaged with this PR (@robobun adopt), and robobun reports build #87633 was green on all c-ares lanes including macOS. The DNS packet construction in the new test checks out: header (QR/RD/RA set, QDCOUNT=1, ANCOUNT=1), question echoed from the query, answer RR with name pointer 0xc0 0x0c, TYPE=33 (SRV), CLASS=IN, TTL=60, and RDATA containing priority/weight/port plus a "srv" label followed by a 0xc0 0x0c compression pointer back to the QNAME — decompressing to srv._test._tcp.example.test as asserted. Socket cleanup is in finally, resolver has timeout: 1000, tries: 1 so it fails fast rather than hanging. The skipIf(isWindows) matches the neighboring CAA test's gate.
## What does this PR do? Updates c-ares to v1.34.8. Compare: c-ares/c-ares@3ac47ee...c7a3138 ### Compatibility patch v1.34.8 started rejecting DNS name compression pointers inside RDATA for non-RFC1035 types (c-ares/c-ares#1190, per RFC 3597). That is correct for writers, but a lot of deployed resolvers and caches (older BIND, dnsmasq, mDNSResponder on macOS, assorted corporate forwarders) still compress SRV/NAPTR targets, so `dns.resolveSrv()` would fail with `EBADRESP` against them. In CI this showed up as `node-dns.test.js` and `mongodb.test.ts` failures on macOS. `patches/cares/accept-rdata-compression.patch` keeps the parser lenient (always follows compression in RDATA) while the writer stays RFC-strict via `ares_dns_rec_allow_name_comp`. ### Other changes - `test/js/node/process/process.test.js`: expected `process.versions.ares` hash updated - `test/js/node/dns/node-dns.test.js`: new local-UDP-server test that serves a compressed SRV target so this regression is caught without depending on a specific upstream resolver Note: the weekly update workflow force-pushed this branch on Aug 9 and dropped these changes once; they were re-applied in ddb9b5d. ## How did you verify your code works? ``` # without patch (c-ares 1.34.8 vanilla) bun bd test test/js/node/dns/node-dns.test.js -t "compressed target" DNSException: querySrv EBADRESP _test._tcp.example.test (fail) dns.resolveSrv accepts compressed target in RDATA # with patch bun bd test test/js/node/dns/node-dns.test.js -t "compressed target" (pass) dns.resolveSrv accepts compressed target in RDATA bun bd test test/js/node/process/process.test.js -t "process.versions" (pass) process.versions ``` Build #87633 (previous full run with these changes) was green on all c-ares-related tests across every lane including macOS. Auto-updated by [this workflow](https://github.com/oven-sh/bun/actions/workflows/update-cares.yml) <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end --> --------- Co-authored-by: Jarred-Sumner <709451+Jarred-Sumner@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
…ailure (#37915) ### Problem - Build lanes die in a dep fetch: `error: Failed to download after 5 attempts: https://github.com/oven-sh/lol-html/archive/725ce499....tar.gz`, `cause: fetch failed`. Build 93382 lost linux x64-asan on lolhtml. Build 93392 (a one-file PR) lost four lanes: x64-musl and x64-android on cares, mimalloc and the WebKit tarball, freebsd aarch64 on lolhtml, windows aarch64 on cares, libuv, mimalloc and WebKit. - Those are the downloads that miss the image's prefetch cache (everything else in the same logs says `using prefetch cache`): the deps whose pins moved after the images were baked in #34782 (lolhtml #36733, mimalloc #36431, c-ares #34007, libuv #36839, WebKit several times a week), plus WebKit on every lane other than linux arm64, because `prefetch-deps.ts` only enumerates the bake host's own target (handed off separately). Each build makes on the order of a hundred live github.com downloads. - `downloadWithRetry` (`scripts/build/download.ts:156`) made 5 attempts with 2+4+8+16s of backoff, about 30s in total. The logs show agent-wide outages longer than that: on the x64-musl lane three downloads that started together failed on all five attempts over roughly two minutes, after lolhtml had succeeded on the same agent seconds earlier; on the freebsd lane lolhtml failed five times in a row while cares and mimalloc went through and WebKit succeeded on its second try. - `BuildError.format()` (`scripts/build/error.ts:33`) prints one level of cause. node's fetch throws `TypeError: fetch failed` and keeps the real error (DNS, connect timeout, reset) in `.cause`, so every one of these logs says only `cause: fetch failed`, and the retry lines say nothing about what failed. ### Fix - `downloadRetry`: 10 attempts, backoff doubling from 2s and capped at 30s, 180s of backoff in total instead of 30s. Exported as a `RetryPolicy` (optional last parameter of `downloadWithRetry`) so the test can run the production attempt count with the backoff zeroed. - 408 and 429 are retried along with 5xx and network errors. Other 4xx still fail on the first attempt and are still thrown unwrapped, which `prefetch-deps.ts` relies on to tell a 404 (variant not published) from a transient failure. - Each retry line names the failure it is retrying, e.g. `retry 2/10 in 2000ms (fetch failed: other side closed)`, and `format()` prints the whole cause chain through the new `describeError()`, so the next one of these says what the network did. - Why here: the prefetch cache goes stale by design as soon as a pin moves, and WebKit moves faster than images get rebaked (the images were rebaked on July 21 and this came back within two weeks; #30095 was an earlier rebake for the same symptom), so the live path is permanently on every build's critical path and has to outlast the outages CI actually sees. The wider window only costs time while github.com is actually down, when the lane would otherwise have failed; build-bun's step timeout is 60 minutes. - Verified with `test/internal/build-download-retry.test.ts` (`bun bd test`, 7 pass). It drives the real `downloadWithRetry` against a local server that drops connections or returns scripted statuses, checks the retry lines and `format()` output, and pins the schedule's total backoff at two minutes or more. Against the previous `download.ts`/`error.ts` the file fails at import (`downloadRetry` and `describeError` did not exist); each behavioral case is something the old loop did not do (10 attempts, 429 retried, reason on the retry line, cause chain in `format()`). - Also ran the loop under node, the runtime CI builds with, against a dropping server: retry lines read `(fetch failed: other side closed)` and the final report prints `cause: fetch failed: other side closed`. `bunx tsc -p scripts/build/tsconfig.json` reports nothing for these files. ### Background - Dep fetching: configure emits one ninja `dep_fetch` edge per vendored dep, which runs `scripts/build/fetch-cli.ts`; that calls `downloadWithRetry` on `https://github.com/<repo>/archive/<commit>.tar.gz`, and `fetchPrebuilt` uses the same function for release tarballs such as WebKit. Both URL kinds start with a 302 from github.com (to codeload.github.com and objects.githubusercontent.com respectively), which is why one github.com problem takes out both kinds at once. - Prefetch cache: CI images run `scripts/prefetch-deps.ts` at bake time, storing each tarball under `/opt/bun-prefetch/by-url/<sha256(url)>`. `downloadWithRetry` looks there before touching the network, so a dep is served from the image only while its pinned URL is the one that was current at bake time; anything bumped later downloads live until the next `[publish images]` rebake. - `BuildError` is the build system's error type; `fetch-cli.ts` and `build.ts` print failures through its `format()`, which produces the `error:` / `hint:` / `cause:` lines seen in the build log.
2 participants
What does this PR do?
Updates c-ares to v1.34.8.
Compare: c-ares/c-ares@3ac47ee...c7a3138
Compatibility patch
v1.34.8 started rejecting DNS name compression pointers inside RDATA for non-RFC1035 types (c-ares/c-ares#1190, per RFC 3597). That is correct for writers, but a lot of deployed resolvers and caches (older BIND, dnsmasq, mDNSResponder on macOS, assorted corporate forwarders) still compress SRV/NAPTR targets, so
dns.resolveSrv()would fail withEBADRESPagainst them. In CI this showed up asnode-dns.test.jsandmongodb.test.tsfailures on macOS.patches/cares/accept-rdata-compression.patchkeeps the parser lenient (always follows compression in RDATA) while the writer stays RFC-strict viaares_dns_rec_allow_name_comp.Other changes
test/js/node/process/process.test.js: expectedprocess.versions.areshash updatedtest/js/node/dns/node-dns.test.js: new local-UDP-server test that serves a compressed SRV target so this regression is caught without depending on a specific upstream resolverNote: the weekly update workflow force-pushed this branch on Aug 9 and dropped these changes once; they were re-applied in ddb9b5d.
How did you verify your code works?
Build #87633 (previous full run with these changes) was green on all c-ares-related tests across every lane including macOS.
Auto-updated by this workflow
no test proof · iteration 4 · Platform-specific test-only change; deferring to CI.