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

fix handle inheritance for shells #334

Merged
merged 1 commit into from
Jul 26, 2019
Merged

fix handle inheritance for shells #334

merged 1 commit into from
Jul 26, 2019

Conversation

ZoeyR
Copy link
Contributor

@ZoeyR ZoeyR commented Jul 22, 2019

Right now the launched shell with inherit handles from the process that launches it. This causes issues if stdin/stdout/stderr are named pipes. Turning off handle inheritance fixes this issue.

@Tyriar
Copy link
Member

Tyriar commented Jul 22, 2019

This causes issues if stdin/stdout/stderr are named pipes.

Can you elaborate a little on this?

@ZoeyR
Copy link
Contributor Author

ZoeyR commented Jul 22, 2019

There was a long internal thread with the console team about this, but the short of it is that the PseudoConsole api doesn't handle shell applications with standard input handles that are not "console handles". If the application launching the shell has standard input handles that are named pipes, the shell application will behave oddly and in some circumstances crash.

I noticed this first when testing node-pty in Service Hub for VS 16.3. What would happen is that the stderr from cmd.exe would never get printed out. By setting STARTF_USESTDHANDLES and setting all the std handles to nullptr we fully isolate the shell from the application launching it.

@Tyriar
Copy link
Member

Tyriar commented Jul 23, 2019

@zadjii-msft a review would be appreciated 🙂

@Tyriar Tyriar added this to the 0.9.0 milestone Jul 23, 2019
@zadjii-msft
Copy link
Member

... I have no memory of this thread. This seems reasonable to me, and I do vaguely recall something weird happening if the shell inherits its parent's handle. IIRC this was a bug in the lead up to the original release of the conpty API, and I thought we fixed it for the actual API.

I don't think this will regress anything, so it's probably safe.

@ZoeyR
Copy link
Contributor Author

ZoeyR commented Jul 24, 2019

@zadjii-msft I think the fix was that if the shell inherits console handles that you close them, but in this case the shell is inheriting named pipes instead.

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.

None yet

3 participants