Skip to content
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

ClosePseudoConsole does not close the process handle to conhost #17903

Closed
asmichi opened this issue Sep 11, 2024 · 3 comments
Closed

ClosePseudoConsole does not close the process handle to conhost #17903

asmichi opened this issue Sep 11, 2024 · 3 comments
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@asmichi
Copy link

asmichi commented Sep 11, 2024

Windows Terminal version

N/A

Windows build number

10.0.22631.4037 (Windows 11 Pro, 23H2)

Other Software

(None, just Kernel32)

Steps to reproduce

Run the EchoCon sample and observe its handles before/after the ClosePseudoConsole call.

  • Add a breakpoint at ClosePseudoConsole(hPC)
  • Start debugging
  • Run handle.exe -p EchoCon.exe -a | findstr Process when the program breaks at ClosePseudoConsole(hPC)
  • Step over
  • Run handle.exe -p EchoCon.exe -a | findstr Process

Expected Behavior

ClosePseudoConsole closes the handle to conhost.exe.

Actual Behavior

ClosePseudoConsole does not close the handle to conhost.exe (leaks the handle).

This looks like a Kernel32 version of #8706.

D:\>REM break just before ClosePseudoConsole(hPC)

D:\>handle -p EchoCon.exe -a | findstr Process
   E0: Process       conhost.exe(34388)

D:\>REM step over ClosePseudoConsole(hPC)

D:\>handle -p EchoCon.exe -a | findstr Process
   E0: Process       <Nonexistent process>(34388)
@asmichi asmichi added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 11, 2024
@lhecker
Copy link
Member

lhecker commented Sep 11, 2024

I checked the corresponding branch internally and it doesn't include the CloseHandle fix. The good news is that 24h2 doesn't have this issue (its ClosePseudoConsole implementation is way simpler and more robust).

@zadjii-msft
Copy link
Member

We can probably close this as fix-checked-in, right? I'm not sure we can backport that...

(if you want to backport it, go for it)

@zadjii-msft zadjii-msft added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Resolution-Fix-Available It's available in an Insiders build or a release Product-Conpty For console issues specifically related to conpty labels Sep 12, 2024
@asmichi
Copy link
Author

asmichi commented Sep 14, 2024

Thank you! I'm glad to hear this is already fixed in the insider build. I have confirmed the fix with 10.0.26120.1542 (Dev Channel).

If backporting is not planned, please feel free to close this as fixed.

asmichi added a commit to asmichi/ChildProcess that referenced this issue Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

No branches or pull requests

4 participants