Skip to content

(terminal): close a pty once, and stop resizing or writing to a killed one (#405) - #408

Open
devsuitup wants to merge 3 commits into
mainfrom
fix/405-conpty-double-close
Open

devsuitup wants to merge 3 commits into
mainfrom
fix/405-conpty-double-close

Conversation

@devsuitup

@devsuitup devsuitup commented Oct 1, 2026 •

Copy link
Copy Markdown
Owner

Refs #405

Cause

ptyProcess.kill() with useConptyDll ends in ConptyClosePseudoConsole(hpc) (node-pty conpty.cc PtyKill). The native handle is only dropped from node-pty's table when the shell's exit thread runs, so a second kill issued before the exit event finds the handle still registered and calls ClosePseudoConsole on an already-freed HPCON. That is a double free: STATUS_HEAP_CORRUPTION (0xc0000374) in conpty.dll+0x6126 (inside ConptyClosePseudoConsole, export at +0x60b0) called from conpty.node kill.

killPty (pty-ops.js) had no memory of a previous kill; stop-session, the window closed handler and before-quit can all reach it for the same session.

Reproduction (isolated instance, throwaway HOME)

Open the panel shell, then stopSession(id) twice back to back: 5 of 5 runs crashed on the first iteration with the same signature. Same driver and scenario with this change: 5 runs x 8 iterations, 0 crashes. Panel toggle, resize-only, single kill, kill-then-resize and kill-while-streaming did not crash before either (8 iterations each).

Change

killPty closes a given pty once; resizePty and writePty do nothing on a pty that was killed. Unit tests in test/pty-ops.test.js.

Integration test

test/pty-ops-conpty-kill.test.js spawns a child node process with the real node-pty (useConptyDll), calls killPty twice back to back and asserts the child exits 0. With the guard removed it fails 3 of 3 runs with exit status 3221226356 (0xc0000374); with it, it passes.

What is not established

The production double-kill path is not identified; the dumps cannot tell. Candidates: window closed followed by before-quit, stop-session plus a panel toggle, a double stop-session. Hence Refs, not Closes.

A second, separate race exists upstream: node-pty #922 (merged 2026-05-13, shipped from 1.2.0-beta.13, not in the installed 1.1.0). Not addressed here.

…d one

A second kill issued before the shell's exit event reaches node-pty's
native kill again, which calls ConptyClosePseudoConsole on an already
freed pseudo console handle and corrupts the heap (0xc0000374), taking
the whole main process down with no JS error.

killPty now closes a given pty once; resizePty and writePty skip a pty
that was killed.

Closes #405
@devsuitup

Copy link
Copy Markdown
Owner Author

Reviewing 124deae (adversarial review in progress).

@devsuitup

Copy link
Copy Markdown
Owner Author

Adversarial review at 124deae: the fix is correct. +0x6126 lies inside ClosePseudoConsole (.pdata range [0x60b0, 0x612c)); PtyKill closes handle->hpc without clearing it, so a second kill before the exit thread frees it twice; every session kill goes through killPty (window closed, stop-session, remote-stop-session, before-quit — quit then closed was a real double path); the remote-attach raw.kill() is guarded by detaching; kill after a natural exit is a no-op. Being added: a windows integration test with a real node-pty in a child process (red without the guard), the context-doc section the code points to, and "Refs #405" until the production double-kill path is named. Upstream node-pty #922 (a separate exit-thread race) is tracked in a follow-up issue.

…nt it

A child node process loads the real node-pty with useConptyDll and kills
a pty twice back to back; with the guard removed it dies with
0xc0000374. The ipc-bridge context now records the once-only kill.

Refs #405
@devsuitup

Copy link
Copy Markdown
Owner Author

Re-review at c149c71: closed — test/pty-ops-conpty-kill.test.js spawns a real node-pty with useConptyDll in a child process and kills it twice: with pty-ops.js reverted the child dies of STATUS_HEAP_CORRUPTION (3/3), with the guard it exits 0 (3/3); skipped off win32 with the reason stated. The context section is updated, the body says "Refs #405" and names the candidate production paths; node-pty #922 is tracked in #409. Ready to merge once CI (windows-2022 runs the new test) is green on c149c71.

@devsuitup

Copy link
Copy Markdown
Owner Author

CI at c149c71: both windows-2022 test jobs fail in the new integration test — the child cannot find node_modules/node-pty/build/Release/conpty/conpty.dll because CI rebuilds node-pty from source and the conpty/ folder is only copied by afterPack at packaging time. Being fixed so the test runs against the packaged layout on CI (no silent skip).

…l test

On CI node-pty is built into build/Release, which has no conpty/ folder
(the afterPack hook adds it at packaging time), so the child could not
find conpty.dll. The child now loads a temp copy of node-pty that carries
only the prebuilds, the layout the packaged app resolves.

Refs #405
@devsuitup

Copy link
Copy Markdown
Owner Author

Re-review at 5332ec1: the CI layout gap is closed in the test itself — the child loads a temp copy of node-pty holding only lib/, package.json and prebuilds/win32-<arch>/, so conpty.dll resolves beside the loaded conpty.node (the packaged layout) whether or not CI rebuilt build/Release. Reported locally: passes with the guard in both layouts, fails with STATUS_HEAP_CORRUPTION in the CI layout without it. Ready to merge once both windows-2022 jobs are green on 5332ec1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant