-
Notifications
You must be signed in to change notification settings - Fork 5k
Create sockets with WSA_FLAG_NO_HANDLE_INHERIT on Windows #36938
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isWindows, tempDir } from "harness"; | ||
| import { connect } from "node:net"; | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/36936 | ||
| // On Windows, sockets created with plain socket() are inheritable, so a | ||
| // detached child spawned while a server is listening would duplicate the | ||
| // listen handle and keep the port open after the parent exits. | ||
| test.skipIf(!isWindows)("detached child does not inherit the parent's listening socket", async () => { | ||
| using dir = tempDir("socket-inherit", { | ||
| "parent.mjs": ` | ||
| import { spawn } from "node:child_process"; | ||
| import { createServer } from "node:http"; | ||
|
|
||
| const server = createServer((_request, response) => response.end("ok")); | ||
| await new Promise((resolve, reject) => { | ||
| server.once("error", reject); | ||
| server.listen(0, "127.0.0.1", resolve); | ||
| }); | ||
| const port = server.address().port; | ||
|
|
||
| // Spawn a detached child while the listen socket is open, then exit. | ||
| const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 100000)"], { | ||
| detached: true, | ||
| stdio: "ignore", | ||
| windowsHide: true, | ||
| }); | ||
| child.unref(); | ||
|
|
||
| await new Promise((resolve, reject) => server.close(error => (error ? reject(error) : resolve()))); | ||
|
Comment on lines
+22
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- candidate test files ---'
git ls-files 'test/js/node/child_process/child-process-socket-inherit.test.ts' 'packages/bun-usockets/src/bsd.c' 'REVIEW.md' '*landing*' | sed -n '1,80p'
printf '%s\n' '--- test outline ---'
ast-grep outline test/js/node/child_process/child-process-socket-inherit.test.ts --view expanded
printf '%s\n' '--- test source ---'
cat -n test/js/node/child_process/child-process-socket-inherit.test.ts
printf '%s\n' '--- bsd.c target area ---'
sed -n '850,930p' packages/bun-usockets/src/bsd.c
printf '%s\n' '--- relevant symbols and call sites ---'
rg -n -C 4 'bsd_accept_socket|accept_socket|child-process-socket-inherit|socket.inherit|inherit' \
test/js/node/child_process packages/bun-usockets/src packages/bun-usockets 2>/dev/null | sed -n '1,240p'
printf '%s\n' '--- repository guidance ---'
if [ -f REVIEW.md ]; then cat REVIEW.md; fiRepository: oven-sh/bun Length of output: 50367 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- relevant history and diff metadata ---'
git status --short
git diff --stat
git diff -- test/js/node/child_process/child-process-socket-inherit.test.ts packages/bun-usockets/src/bsd.c | sed -n '1,260p'
printf '%s\n' '--- adjacent child-process socket tests ---'
rg -n -C 8 'server\.listen|server\.close|spawn\(|ECONNREFUSED|accept|connection' \
test/js/node/child_process -g '*.test.ts' | sed -n '1,320p'Repository: oven-sh/bun Length of output: 27761 🏁 Script executed: #!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
test = Path("test/js/node/child_process/child-process-socket-inherit.test.ts").read_text()
bsd = Path("packages/bun-usockets/src/bsd.c").read_text()
parent = test.split('"parent.mjs": `', 1)[1].split('`,', 1)[0]
spawn_at = parent.index("spawn(process.execPath")
close_at = parent.index("server.close")
parent_connects = bool(re.search(r"\b(?:connect|fetch|http\.request|https\.request)\s*\(", parent[:spawn_at]))
test_connect_at = test.index("const socket = connect")
parent_exit_marker = test.index("const { port, childPid }")
print({
"parent_connects_before_spawn": parent_connects,
"parent_closes_server_after_spawn": close_at > spawn_at,
"external_connect_occurs_after_parent_output_is_read": test_connect_at > parent_exit_marker,
"windows_accepted_handle_guard_present": bool(
re.search(
r"`#ifdef` _WIN32\s*/\* accept\(\).*?SetHandleInformation"
r"\(\(HANDLE\) accepted_fd,\s*HANDLE_FLAG_INHERIT,\s*0\);",
bsd,
re.S,
)
),
"cleanup_swallows_process_kill_errors": bool(
re.search(r"try\s*\{\s*process\.kill\(childPid\);\s*\}\s*catch\s*\{\s*\}", test, re.S)
),
})
PY
printf '%s\n' '--- cross-platform landing guidance ---'
rg -n -A 45 -B 5 'Cross-platform|platform-sensitive|Windows' .claude/docs/landing-prs.md | sed -n '1,180p'Repository: oven-sh/bun Length of output: 24602 🌐 Web query:
💡 Result: GitHub Issue
[1] (github.com) Citations: Add coverage for accepted socket inheritance. The current test connects only after the parent exits, so it does not execute 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| console.log(JSON.stringify({ port, childPid: child.pid })); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "parent.mjs"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| const { port, childPid } = JSON.parse(stdout.trim()); | ||
|
Comment on lines
+41
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Validate Line 43 asserts the exit code before Line 44 validates the fixture output. Parse and validate Based on learnings, subprocess tests must assert stdout and stderr before the exit code. 🤖 Prompt for AI AgentsSources: Coding guidelines, Learnings |
||
|
|
||
| try { | ||
| // The parent has exited and closed its server, so nothing may be listening | ||
| // on the port even though the detached child is still alive. | ||
| const result = await new Promise<string>(resolve => { | ||
| const socket = connect({ port, host: "127.0.0.1" }); | ||
| socket.on("connect", () => { | ||
| socket.destroy(); | ||
| resolve("connected"); | ||
| }); | ||
| socket.on("error", error => resolve((error as NodeJS.ErrnoException).code ?? "error")); | ||
| }); | ||
| expect(result).toBe("ECONNREFUSED"); | ||
| } finally { | ||
| try { | ||
| process.kill(childPid); | ||
| } catch {} | ||
|
Comment on lines
+59
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -eu
target='test/js/node/child_process/child-process-socket-inherit.test.ts'
printf '%s\n' '--- target outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline "$target" --view expanded || true
fi
printf '%s\n' '--- target context ---'
cat -n "$target" | sed -n '1,130p'
printf '%s\n' '--- nearby files ---'
fd -i 'child.*process.*\.test\.(ts|tsx|js|jsx)$' test/js/node/child_process 2>/dev/null | head -40 || true
printf '%s\n' '--- kill cleanup patterns ---'
rg -n -C 5 'process\.kill|catch\s*\{\s*\}' test/js/node/child_process test 2>/dev/null | head -240 || true
printf '%s\n' '--- review guidance ---'
for f in REVIEW.md test/REVIEW.md; do
if [ -f "$f" ]; then
printf '%s\n' "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: oven-sh/bun Length of output: 44502 🏁 Script executed: #!/bin/bash
set -eu
target='test/js/node/child_process/child-process-socket-inherit.test.ts'
cat -n "$target" | sed -n '1,130p'
rg -n -C 5 'process\.kill|childPid|detached|unref' test/js/node/child_process test 2>/dev/null | head -260Repository: oven-sh/bun Length of output: 21105 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- process.kill implementations and bindings ---'
rg -n -C 8 'process\.kill|kill\(.*pid|ERRNO.*ESRCH|ESRCH|EPERM' src packages test/js | head -400
printf '%s\n' '--- platform-specific kill helpers ---'
rg -n -C 6 'killProcess|terminateProcess|TerminateProcess|kill.*process|process.*kill' src | head -400
printf '%s\n' '--- process.kill type/docs references ---'
rg -n -C 5 'kill\s*\(' packages/bun-types src/js test/js/node/child_process | head -300Repository: oven-sh/bun Length of output: 50367 🏁 Script executed: #!/bin/bash
set -eu
rg -n -C 8 'process\.kill|kill\(.*pid|ERRNO.*ESRCH|ESRCH|EPERM' src packages test/js | head -400
rg -n -C 6 'killProcess|terminateProcess|TerminateProcess|kill.*process|process.*kill' src | head -400Repository: oven-sh/bun Length of output: 50367 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- child-process kill implementation ---'
sed -n '1535,1605p' src/js/node/child_process.ts
printf '%s\n' '--- global process.kill references ---'
rg -n -C 12 'kill\s*:\s*|kill\(pid|fn.*kill|process_kill|uv_kill' src/js src | head -260
printf '%s\n' '--- direct uv_kill implementation references ---'
rg -n -C 10 'uv_kill|pub fn kill|fn kill' src/spawn src/js src/bun.js src | head -260Repository: oven-sh/bun Length of output: 35116 🏁 Script executed: #!/bin/bash
set -eu
sed -n '1535,1605p' src/js/node/child_process.ts
rg -n -C 12 'kill\s*:\s*|kill\(pid|process_kill|uv_kill' src/js src | head -260
rg -n -C 10 'uv_kill|pub fn kill|fn kill' src/spawn src/js src | head -260Repository: oven-sh/bun Length of output: 32407 🏁 Script executed: #!/bin/bash
set -eu
printf '%s\n' '--- process module files ---'
fd -i 'process' src/js src | head -120
printf '%s\n' '--- kill declarations in likely process modules ---'
for f in $(fd -i 'process' src/js src | head -80); do
if rg -q 'kill|Process' "$f" 2>/dev/null; then
printf '%s\n' "--- $f ---"
rg -n -C 5 'kill|Process' "$f" | head -100
fi
done
printf '%s\n' '--- read-only policy verifier ---'
python3 - <<'PY'
from pathlib import Path
kill_impl = Path("src/spawn/process.rs").read_text()
target = Path("test/js/node/child_process/child-process-socket-inherit.test.ts").read_text()
errno_map = Path("src/errno/windows_errno.rs").read_text()
assert 'if errno_ != bun_sys::E::ESRCH' in kill_impl
assert 'if err.errno != bun_sys::E::ESRCH as u16' in kill_impl
assert 'process.kill(childPid)' in target
assert 'catch {}' in target
assert 'EPERM = 1' in errno_map
print("target uses broad cleanup suppression: yes")
print("implementation treats only ESRCH as benign in POSIX and Windows child kill paths: yes")
print("Windows errno table defines EPERM: yes")
PYRepository: oven-sh/bun Length of output: 50367 🏁 Script executed: #!/bin/bash
set -eu
fd -i 'process' src/js src | head -120
for f in $(fd -i 'process' src/js src | head -80); do
if rg -q 'kill|Process' "$f" 2>/dev/null; then
printf '%s\n' "--- $f ---"
rg -n -C 5 'kill|Process' "$f" | head -100
fi
done
python3 - <<'PY'
from pathlib import Path
kill_impl = Path("src/spawn/process.rs").read_text()
target = Path("test/js/node/child_process/child-process-socket-inherit.test.ts").read_text()
errno_map = Path("src/errno/windows_errno.rs").read_text()
assert 'if errno_ != bun_sys::E::ESRCH' in kill_impl
assert 'if err.errno != bun_sys::E::ESRCH as u16' in kill_impl
assert 'process.kill(childPid)' in target
assert 'catch {}' in target
assert 'EPERM = 1' in errno_map
print("cleanup policy evidence is consistent")
PYRepository: oven-sh/bun Length of output: 50367 Report unexpected child cleanup failures. If 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| } | ||
|
Check warning on line 62 in test/js/node/child_process/child-process-socket-inherit.test.ts
|
||
|
Comment on lines
+41
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The Extended reasoning...What the bug isThe test spawns a parent process that in turn spawns a detached grandchild running expect(stderr).toBe(""); // line 42
expect(exitCode).toBe(0); // line 43
const { port, childPid } = JSON.parse(stdout.trim()); // line 44
try { ... } finally { process.kill(childPid); } // lines 46-62If any of lines 42-44 throw, control never reaches the Concrete trigger path (step-by-step)
The same applies if the parent crashes after Why existing code doesn't prevent itThe Why this matters (REVIEW.md)REVIEW.md's test-hygiene section states:
This is a direct instance of that rule: cleanup is registered after three assertions. ImpactTest-hygiene only. With this PR's fix applied, the leaked grandchild holds no inherited socket, port, or stdio pipe — it's just an idle process that self-terminates after 100s. Nothing user-facing breaks; the concern is orphan accumulation on persistent Windows CI runners across retries when the test fails. FixEither or both of:
|
||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 35768
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 50367
🌐 Web query:
Microsoft documentation SetHandleInformation HANDLE_FLAG_INHERIT accepted socket accept Windows handle inheritance closesocket GetLastError💡 Result:
While Windows documentation lists sockets among the objects supported by SetHandleInformation, attempting to manage socket handle inheritance via SetHandleInformation with the HANDLE_FLAG_INHERIT flag is often unreliable and frequently ineffective due to the architecture of Windows Sockets (Winsock) [1][2][3][4]. The primary issue is the involvement of Layered Service Providers (LSPs), which sit between your application and the underlying Winsock stack [2][5][6]. Even if you successfully mark a socket handle as non-inheritable using SetHandleInformation, the underlying handle used by the LSP may still be marked inheritable, causing child processes to inadvertently receive access to the socket [2]. Furthermore, even when inheritance is technically avoided, inherited socket handles often cannot be used in a child process because the LSP was never informed of the handle duplication, leading to errors such as WSAENOTSOCK (10038) or invalid handle errors [5]. To reliably prevent socket handle inheritance, consider the following approaches: 1. Use WSA_FLAG_NO_HANDLE_INHERIT: When creating a socket, use the WSASocket function with the WSA_FLAG_NO_HANDLE_INHERIT flag (available on Windows Vista/Server 2008 and later). This is the most reliable way to ensure a socket is not inherited [2]. 2. Use Explicit Handle Lists: Instead of setting bInheritHandles to TRUE in CreateProcess (which exposes all inheritable handles), you can use the PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute with UpdateProcThreadAttribute. This allows you to provide an explicit list of specific handles to be inherited, ensuring only the intended handles are passed to the child process [7][8]. 3. Avoid Relying on Inheritance for IPC: Because of the complexities surrounding socket inheritance, it is generally recommended to avoid using socket handle inheritance for interprocess communication. Instead, use explicit IPC mechanisms such as Named Pipes or sockets created with explicit non-inheritance flags [5]. Regarding closesocket and GetLastError: If you encounter an error with a socket handle—potentially because an inherited handle has become invalid—calling closesocket will return SOCKET_ERROR, and you can retrieve the specific error code using WSAGetLastError (or GetLastError, though WSAGetLastError is preferred for Winsock-specific errors) [5][6]. However, if the handle has been closed or invalidated by the system due to the inheritance issues described above, the error returned will typically indicate that the handle is no longer valid [5].
Citations:
🌐 Web query:
site:learn.microsoft.com Winsock accept accepted socket handle inheritance WSA_FLAG_NO_HANDLE_INHERIT accept💡 Result:
In Winsock, socket handles are inheritable by default when created via the socket or WSASocket functions [1][2]. The WSA_FLAG_NO_HANDLE_INHERIT flag (0x80) can be passed to the WSASocket function to create a socket that is explicitly non-inheritable [1][2]. Regarding connection acceptance: 1. The standard accept function creates and returns a new socket handle for an incoming connection [3][4]. This newly created handle generally inherits the inheritance property of the listening socket or follows the default behavior of being inheritable [1][2]. 2. The WSAAccept function, which allows for conditional acceptance, also returns a new socket handle [5][6]. 3. If you require a non-inheritable socket handle for a new connection accepted via these functions, you cannot directly pass a "non-inheritance" flag to accept or WSAAccept. Instead, you must use the SetHandleInformation function from the Windows API after the socket has been accepted to modify the handle's inheritance attribute (specifically, by clearing the HANDLE_FLAG_INHERIT flag). 4. For high-performance scenarios using AcceptEx, the socket passed into the sAcceptSocket parameter must be created beforehand using WSASocket [7]. You can (and should) use the WSA_FLAG_NO_HANDLE_INHERIT flag during the creation of this sAcceptSocket to ensure it is non-inheritable before it is even used in the AcceptEx operation [1][7][2].
Citations:
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 576
Fail closed when
SetHandleInformationfails. SaveGetLastError(), closeaccepted_fd, restore the error afterclosesocket, and returnLIBUS_SOCKET_ERROR; do not pass an accepted socket with inheritance still enabled.🤖 Prompt for AI Agents
Source: Coding guidelines