-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix: SSLConfig intern/deref race causing segfault in proxy tunnel setup #27838
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
001acd4
fix: SSLConfig intern/deref race causing segfault in proxy tunnel setup
cirospaciari f98c514
refactor: use Arc/Weak split refcounting for SSLConfig registry
cirospaciari f9870a5
test: address review comments on fetch-proxy-tls-intern-race
cirospaciari ce7c134
openssl: add NULL guard to add_ca_cert_to_ctx_store for consistency
cirospaciari 0a7a3e7
test: remove non-deterministic race condition stress test
claude d099559
test: add worker-based SSLConfig intern/deref race test
cirospaciari 0569a6b
test: increase concurrency for SSLConfig race reproduction
cirospaciari 50cd844
remove race test (reliable repro is debug-only, see #27863 for recipe)
cirospaciari 5376f1c
test: add SSLConfig intern/deref race regression test
cirospaciari 75dd278
test: add try-finally cleanup for workers and proxy server
cirospaciari de2872c
test: rewrite SSLConfig race test as subprocess + fixture
cirospaciari 767002a
test: remove workers from race fixture, use single-process setImmedia…
cirospaciari 4bf3f5e
refactor: use bun.ptr.shared for SSLConfig refcounting (#27872)
cirospaciari 4625a94
refactor: propagate SSLConfig.SharedPtr through fetch/http layers (#2…
cirospaciari File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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,102 @@ | ||
| // Regression test: segfault at 0x0 in create_ssl_context_from_bun_options during | ||
| // proxy tunnel setup. | ||
| // | ||
| // Root cause: SSLConfig.GlobalRegistry is a weak dedup cache but did not hold a | ||
| // strong ref on its entries. When the last external holder deref'd a config | ||
| // (HTTP thread) while a new fetch() with identical tls options interned the same | ||
| // content (JS thread), intern() could return a pointer whose refcount had already | ||
| // hit 0. The returned pointer was then destroyed concurrently, and the proxy | ||
| // tunnel later dereferenced freed cert/key memory -> strlen(NULL) -> segfault. | ||
| // | ||
| // Fix: registry now holds a +1 ref on every entry, so intern() always sees a | ||
| // live object. Entries are evicted when the external refcount drops to zero via | ||
| // a 2->1 transition check under the registry mutex. | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| // | ||
| // This test stresses the intern/deref race by firing overlapping waves of proxy | ||
| // requests with identical tls options. Each completing request derefs the | ||
| // config; each starting request interns an identical one. | ||
|
|
||
| import { expect, test } from "bun:test"; | ||
| import { tls as tlsCert } from "harness"; | ||
| import { once } from "node:events"; | ||
| import net from "node:net"; | ||
|
|
||
| async function createConnectProxy() { | ||
| const server = net.createServer(client => { | ||
| client.once("data", head => { | ||
| const text = head.toString("latin1"); | ||
| const nl = text.indexOf("\r\n"); | ||
| const [, hostPort] = text.slice(0, nl).split(" "); | ||
| const colon = hostPort.lastIndexOf(":"); | ||
| const host = hostPort.slice(0, colon); | ||
| const port = Number(hostPort.slice(colon + 1)); | ||
|
|
||
| const upstream = net.connect(port, host, () => { | ||
| client.write("HTTP/1.1 200 Connection Established\r\n\r\n"); | ||
| // Forward any bytes that arrived after the CONNECT header in the same packet | ||
| const headerEnd = text.indexOf("\r\n\r\n"); | ||
| const extra = head.subarray(headerEnd + 4); | ||
| if (extra.length > 0) upstream.write(extra); | ||
| client.pipe(upstream); | ||
| upstream.pipe(client); | ||
| }); | ||
| upstream.on("error", () => client.destroy()); | ||
| client.on("error", () => upstream.destroy()); | ||
| }); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| server.listen(0, "127.0.0.1"); | ||
| await once(server, "listening"); | ||
| const { port } = server.address() as net.AddressInfo; | ||
| return { server, url: `http://127.0.0.1:${port}` }; | ||
| } | ||
|
|
||
| test("concurrent proxy fetches with identical tls options do not race SSLConfig intern/deref", async () => { | ||
| using backend = Bun.serve({ | ||
| port: 0, | ||
| tls: tlsCert, | ||
| fetch() { | ||
| return new Response("ok"); | ||
| }, | ||
| }); | ||
|
|
||
| const proxy = await createConnectProxy(); | ||
| const target = `https://127.0.0.1:${backend.port}/`; | ||
|
|
||
| // The tls option object is rebuilt on every fetch call, so each call allocates | ||
| // a fresh SSLConfig and hits GlobalRegistry.intern(). Identical content means | ||
| // they all dedup to the same registry entry. | ||
| // keepalive:false forces each request to drop its ref immediately on | ||
| // completion instead of parking the socket in the keepalive pool (which | ||
| // would hold an extra ref and mask the race). | ||
| const makeRequest = () => | ||
| fetch(target, { | ||
| proxy: proxy.url, | ||
| keepalive: false, | ||
| tls: { | ||
| ca: tlsCert.cert, | ||
| rejectUnauthorized: false, | ||
| }, | ||
| }).then(r => r.text()); | ||
|
|
||
| try { | ||
| // Prime the registry so subsequent waves hit the found_existing path. | ||
| expect(await makeRequest()).toBe("ok"); | ||
|
|
||
| // Fire overlapping waves: start a new wave while the previous is still | ||
| // settling. This maximises the window where one request's deref (2->1, | ||
| // eviction attempt) races a new request's intern (find existing, ref). | ||
| const concurrency = 8; | ||
| const waves = 6; | ||
| let inFlight: Promise<string[]> = Promise.resolve([]); | ||
| for (let w = 0; w < waves; w++) { | ||
| const prev = inFlight; | ||
| inFlight = Promise.all(Array.from({ length: concurrency }, makeRequest)); | ||
| const results = await prev; | ||
| for (const r of results) expect(r).toBe("ok"); | ||
| } | ||
| const last = await inFlight; | ||
| for (const r of last) expect(r).toBe("ok"); | ||
| } finally { | ||
| proxy.server.close(); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }); | ||
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.