-
Notifications
You must be signed in to change notification settings - Fork 5k
test: make the ported Node suite run leak-clean under the ASAN runner, and fix the teardown UAFs and leaks it surfaces #31833
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
Closed
Closed
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
8c373a0
Make the full Node parallel/sequential suite pass leak-clean under th…
cirospaciari 2aec5c2
[autofix.ci] apply automated fixes
autofix-ci[bot] ab7a099
Merge branch 'main' into claude/node-suite-asan-leak-clean
robobun 9b7a19b
boringssl: free SAN stacks with GENERAL_NAMES_free
alii 87ac1ab
url: return OwnedString from WTF::URL getters
alii 671eff8
child_process: read normalized stdio length; add explicit takeStdio
alii 8756b01
vm: consolidate pre-teardown Strong-handle release; call from Worker …
alii b340e97
test: narrow leaksan suppressions; scope FLAKY entry to ASAN; runner …
alii 1b7f0fb
Merge branch 'main' into claude/node-suite-asan-leak-clean
alii 89c5a16
Merge branch 'main' into claude/node-suite-asan-leak-clean
alii c9aaff8
[autofix.ci] apply automated fixes
autofix-ci[bot] 3df81f7
Merge branch 'main' into claude/node-suite-asan-leak-clean
alii d538d79
runner: keep NODE_TEST_DIR unset on Windows
alii 10af51d
Merge remote-tracking branch 'origin/main' into HEAD
alii f04711d
verify skill: use bun bd for probes to match CLAUDE.md build-then-exe…
alii 151cf3c
Merge remote-tracking branch 'origin/main' into claude/node-suite-asa…
cirospaciari a0d36f9
test: unquarantine test-worker-terminate-http2-respond-with-file
cirospaciari e06fa0a
vm: release Strong handles in destroy() too; strengthen child_process…
cirospaciari f7bd302
test: drop the Bun.main teardown smoke test
cirospaciari b16da77
Merge origin/main into claude/node-suite-asan-leak-clean
cirospaciari a9612fb
Merge remote-tracking branch 'origin/main' into claude/node-suite-asa…
robobun 6686a7f
trim comments to <=3 lines, cite spec/node source
robobun 703ab06
test: await stream finished() instead of asserting readableEnded at exit
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
The diff you're trying to view is too large. We only load the first 3000 changed files.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Regenerates test/expected-durations.json from recent Buildkite runs and | ||
| # uploads it as a build artifact. Attach a weekly schedule to this pipeline in | ||
| # the Buildkite UI (it is not wired into ci.mjs so it never runs on PRs). | ||
| # | ||
| # The runner uses the checked-in copy for sharding; refresh that copy by | ||
| # downloading this artifact and committing it when the shard balance drifts. | ||
| steps: | ||
| - label: ":stopwatch: update-test-durations" | ||
| if: build.source == "schedule" || build.source == "ui" | ||
| agents: | ||
| queue: build-linux | ||
| command: | | ||
| node scripts/update-test-durations.mjs --builds 5 | ||
| buildkite-agent artifact upload test/expected-durations.json | ||
| env: | ||
| # The script reads BUILDKITE_API_TOKEN; the agent environment hook | ||
| # already exports a read-scoped token under this name. | ||
| BUILDKITE_API_TOKEN: "$BUILDKITE_API_TOKEN" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| --- | ||
| description: Upgrade Bun's BoringSSL fork (oven-sh/boringssl) to the latest upstream google/boringssl | ||
| --- | ||
|
|
||
| Bun pins BoringSSL by **commit SHA** in `scripts/build/deps/boringssl.ts` (`BORINGSSL_COMMIT`). The build downloads a tarball from `oven-sh/boringssl` at that SHA — there is no submodule and `vendor/boringssl/` is git-ignored. | ||
|
|
||
| The fork carries a small patch set on top of upstream (see "Preserved patches" below). Upgrading means: merge `google/boringssl` into `oven-sh/boringssl@master`, push, then bump the SHA + regenerate source lists in Bun. | ||
|
|
||
| ## Steps | ||
|
|
||
| ### 1. Clone the fork and merge upstream | ||
|
|
||
| ```sh | ||
| git clone https://github.com/oven-sh/boringssl.git /tmp/boringssl | ||
| cd /tmp/boringssl | ||
| git remote add upstream https://github.com/google/boringssl.git | ||
| git fetch upstream | ||
| git log --oneline $(git merge-base HEAD upstream/main)..HEAD # our patches | ||
| git merge upstream/main | ||
| ``` | ||
|
|
||
| Resolve conflicts **preserving the fork's additions**. Most conflicts are upstream's periodic `|...|` → `` `...` `` doc-comment restyle landing adjacent to a line we added — keep our line + upstream's comment style. For `include/openssl/nid.h`, keep upstream's new NIDs **and** ours (our NID numbers are from OpenSSL's range and don't collide with BoringSSL's sequential allocation). | ||
|
|
||
| ### 2. Verify the merged tree builds | ||
|
|
||
| ```sh | ||
| cmake -B build -GNinja -DCMAKE_BUILD_TYPE=Release | ||
| ninja -C build crypto ssl decrepit | ||
| ``` | ||
|
|
||
| This catches mis-resolved conflicts before they reach Bun's CI. | ||
|
|
||
| ### 3. Push to the fork | ||
|
|
||
| The default branch is **`master`** (not `main`). | ||
|
|
||
| ```sh | ||
| git push origin HEAD:master | ||
| NEW_SHA=$(git rev-parse HEAD) | ||
| ``` | ||
|
|
||
| ### 4. Bump Bun | ||
|
|
||
| In the bun repo: | ||
|
|
||
| - `scripts/build/deps/boringssl.ts` — set `BORINGSSL_COMMIT` to `$NEW_SHA`. | ||
| - `test/js/node/process/process.test.js` — update the `boringssl:` entry in `expectedVersions` to `$NEW_SHA`. | ||
| - Regenerate the source lists (the file's header comment has the exact one-liner). Only `gen/sources.json` is authoritative — diff old vs new and apply the delta: | ||
|
|
||
| ```sh | ||
| rm -rf vendor/boringssl # force re-fetch on next build | ||
| bun bd --target=clone-boringssl | ||
| bun -e 'const j=require("./vendor/boringssl/gen/sources.json"); | ||
| const f=l=>l.map(JSON.stringify).join(", "); | ||
| for(const k of ["bcm","crypto","ssl","decrepit"]) console.log(k,"\n",f(j[k].srcs)); | ||
| console.log("asm\n",f([...j.bcm.asm,...j.crypto.asm])); | ||
| console.log("nasm\n",f([...j.bcm.nasm,...j.crypto.nasm]))' | ||
| ``` | ||
|
|
||
| ### 5. Build and test locally | ||
|
|
||
| ```sh | ||
| rm -rf vendor/boringssl | ||
| bun bd -p 'require("crypto").createHash("sha3-256").update("hi").digest("hex")' | ||
| bun bd test test/js/node/crypto/ test/js/bun/crypto/ | ||
| bun bd test test/js/node/tls/ test/js/web/fetch/fetch.tls.test.ts | ||
| ``` | ||
|
|
||
| ### 6. Open the Bun PR | ||
|
|
||
| ```sh | ||
| git checkout -b claude/boringssl-<upstream-short-sha> | ||
| git commit -am "deps: upgrade BoringSSL to <upstream-short-sha>" | ||
| git push -u origin HEAD | ||
| gh pr create | ||
| ``` | ||
|
|
||
| Then `bun run ci:watch` and fix anything that turns up. | ||
|
|
||
| ## Preserved patches (what conflicts to expect) | ||
|
|
||
| `git diff $(git merge-base HEAD upstream/main)..HEAD --stat` — currently ~35 files, ~550 insertions: | ||
|
|
||
| - **SHA-512/224** — `crypto/fipsmodule/sha/sha512.cc.inc`, `crypto/sha/sha512.cc`, `include/openssl/{sha2,nid,digest}.h` | ||
| - **SHA3-224/256/384/512 as `EVP_MD`** — `crypto/digest/digest_extra.cc`, `crypto/fipsmodule/{digest/digests.cc.inc,keccak/*}`, `include/openssl/{digest,nid}.h` | ||
| - **HMAC-SHA3** — `crypto/hmac/hmac_test*.{cc,txt}` | ||
| - **BLAKE2b-512** — `crypto/blake2/blake2.cc`, `include/openssl/blake2.h` | ||
| - **RIPEMD160 in `crypto/` (not `decrepit/`) + `EVP_ripemd160` lookup** — `crypto/ripemd/ripemd.cc` (moved), `crypto/digest/digest_extra.cc`, `include/openssl/digest.h`, `gen/sources.*`, `build.json` | ||
| - **`EVP_PBE_validate_scrypt_params`** — `crypto/evp/scrypt.cc`, `include/openssl/evp.h` | ||
| - **Electron `SSL_want` / `EVP_CIPHER_do_all_sorted`** — `ssl/ssl_lib.cc` (return `rwstate` directly), `ssl/ssl_test.cc` (drops the corresponding test block), `decrepit/evp/evp_do_all.cc`, `crypto/cipher/get_cipher.cc`, `include/openssl/cipher.h` | ||
| - **MLDSA stack-frame pragma** — `crypto/fipsmodule/mldsa/mldsa.cc.inc` | ||
|
|
||
| If upstream upstreams any of these (check `git grep` on `upstream/main` before re-applying), drop the fork's copy. | ||
|
|
||
| ## Things that have broken before | ||
|
|
||
| - **`SSL_CTX` / `SSL_ECH_KEYS` / `SSL_CREDENTIAL` made opaque** — Bun's Rust FFI (`src/boringssl_sys/boringssl.rs`) treats them as opaque already, so this is fine, but check `packages/bun-usockets/src/crypto/openssl.c` for any direct field access. | ||
| - **`BIO_read`/`BIO_write` error-value narrowing** — can change `SSL_read` error paths over memory BIOs (`SSLWrapper` for TLS-over-duplex). If `node-tls-connect.test.ts` crashes in `flush_pending_events`, see `src/runtime/socket/UpgradedDuplex.rs::teardown` and `WindowsNamedPipe.rs`'s `WRAPPER_BUSY` for the re-entrant-drop guard. | ||
| - **Per-handshake allocation churn (PQ key shares)** grows under ASAN quarantine; RSS-delta tests like `tls-connect-socket-churn.test.ts` may need their `isASAN` bound raised. The `sslCtxLiveCount` check is the real regression guard there — if that passes and LSAN is clean, raise the RSS bound. | ||
| - **`asn1_string_st` / `GENERAL_NAME_st` layout** — Bun mirrors these in `src/boringssl_sys/boringssl.rs`; diff `include/openssl/{asn1,x509v3}.h` for field changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.