fix: close pipe handles and free attribute list when conpty spawn fails by codebytere-ant · Pull Request #935 · microsoft/node-pty · GitHub
Skip to content

fix: close pipe handles and free attribute list when conpty spawn fails#935

Open
codebytere-ant wants to merge 1 commit into
microsoft:mainfrom
codebytere-ant:fix/conpty-handle-leak-on-spawn-failure
Open

fix: close pipe handles and free attribute list when conpty spawn fails#935
codebytere-ant wants to merge 1 commit into
microsoft:mainfrom
codebytere-ant:fix/conpty-handle-leak-on-spawn-failure

Conversation

@codebytere-ant

@codebytere-ant codebytere-ant commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

PtyConnect only closed handle->hIn / handle->hOut after a successful CreateProcessW. When spawn fails (e.g. ERROR_FILE_NOT_FOUND 2, ERROR_ACCESS_DISABLED_BY_POLICY 1260), the function threw before reaching those CloseHandle calls, leaking both named-pipe server handles for every failed spawn. Long-running hosts that retry spawns steadily leak kernel handles.

Separately, the heap-allocated PROC_THREAD_ATTRIBUTE_LIST buffer (attrList) was never freed on any path — success or failure — and DeleteProcThreadAttributeList was never called.

Changes

  • Close hIn/hOut and null them on the CreateProcessW failure path; also null them after the existing success-path close so PtyKill can tell they are already released.
  • Call DeleteProcThreadAttributeList + delete[] attrList on every PtyConnect exit (the two attribute-setup failures, the CreateProcessW failure, and success).
  • Build the Napi::Error before running cleanup on failure paths so the reported GetLastError() is the real spawn error rather than a side effect of CloseHandle / DeleteProcThreadAttributeList.
  • Add a defensive close of hIn/hOut in PtyKill for the case where PtyConnect was never called.

cc @deepak1556

@codebytere-ant codebytere-ant force-pushed the fix/conpty-handle-leak-on-spawn-failure branch from d4fc305 to 6e4d68b Compare June 25, 2026 12:32
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