Add a project that produces conpty.dll #2611

DHowett-MSFT · 2019-08-30T00:09:03Z

Summary of the Pull Request

This pull request introduces a copy of the code from kernel32.dll that implements CreatePseudoConsole, ClosePseudoConsole and ResizePseudoConsole. Apart from some light modifications to fit into the infrastructure in this project and support launching OpenConsole.exe, it is intended to be 1:1 with the code that ships in Windows.

Any guideline violations in this code are likely intentional. Since this was built into kernel32, it used the STL only very sparingly.

kernel32 is on the default link line, so we need to figure out how to get included before it. Either that, or we should export different function names.

References

#1130

PR Checklist

Closes #xxx
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already.

Validation Steps Performed

Launched vtpipeterm.

DHowett-MSFT · 2019-08-30T00:09:55Z

src/winconpty/winconpty.vcxproj

+    <ClCompile Include="precomp.cpp">
+      <PrecompiledHeader>Create</PrecompiledHeader>
+    </ClCompile>
+    <ClCompile Include="../server/DeviceHandle.cpp" />


not great, but necessary to avoid duplicating code.

Eh, good enough.

fcharlie · 2019-08-30T01:48:50Z

This PR is great. Regarding the export function with the same name as kernel32, my suggestion is that since the conpty code is not long, you can turn it into an implementation of header only. Then isolate it with a namespace.

driver1998 · 2019-08-30T23:50:40Z

If we are taking it out from Windows, will it be possible to backport ConPTY to earlier version of Windows?

fcharlie · 2019-08-31T00:01:40Z

@driver1998 I think it depends on conhost.exe. In this code, we can see that this is essentially conhost running in PTY mode, so the porting of conpty to the old Windows is based on the premise that the new version of conhost.exe can be used on older Windows. run. Having said that, the problem of porting the new conhost.exe to the old Windows may need to consider factors such as graphics and kernel console handles.

Biswa96 · 2019-08-31T08:39:04Z

will it be possible to backport ConPTY to earlier version of Windows?

Also it depends on the CreateProcess() because a process uses PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE flag for ConPTY. And the actual kernel mode driver ConDrv.sys.

Some queries:
- Is not this code proprietary (kernelbase.dll)?
- Will this code be somewhat stable among future Windows OS versions?

DHowett-MSFT · 2019-09-03T02:16:03Z

@Biswa96

The code was proprietary, but we're open-sourcing it as it's a minimal facade around code you can already find in other parts of this repository.
condrv is not involved in establishing a pseudoconsole session.
It will only remain stable to the extent that it needs to. As two examples:
- the signal pipe is an implementation detail
- the only thing that will need to remain stable between this repository's version and the OS version is the position of the reference handle in the HPCON.
- we reserve the right to change the internal conpty interface at any point within our compatibility guarantee.

zadjii-msft

Couple questions. Additionally:

should we also push the tests for this out?

src/winconpty/winconpty.cpp

zadjii-msft · 2019-09-03T14:59:43Z

src/winconpty/winconpty.cpp

+#pragma warning(push)
+#pragma warning(disable : 4273) // inconsistent dll linkage (we are exporting things kernel32 also exports)
+
+extern "C" __declspec(dllexport) wchar_t* _ConsoleHostPath()


maybe needs a doc comment

Didn't mean to export it, but yes, a doc comment would be good

zadjii-msft · 2019-09-03T15:02:40Z

src/winconpty/winconpty.cpp

+// These functions are defined in the console l1 apiset, which is generated from
+//      the consoleapi.apx file in minkernel\apiset\libs\Console.
+
+HRESULT CreatePseudoConsoleAsUser(_In_ HANDLE hToken,


Did we ever make this public before? I guess we're not __declspec(dllexport)ing it from this dll, so probably nbd.

It's not public in win32, but also not harmful. 😄

Biswa96 · 2019-09-03T15:50:43Z

Is it possible to add STDERR instead of STDOUT in siEx.StartupInfo.hStdError parameter?
Is it possible to make the write pipe handle inheritable also so that child process can change conpty size?

zadjii-msft · 2019-09-03T16:04:44Z

@Biswa96 I don't really want to make any code changes in this PR. This PR should exists solely as a port from OS code to OSS code. We can handle feature requests for the conpty API in their own separate issues.

DHowett-MSFT · 2019-09-03T16:11:02Z

@Biswa96

There is no meaningful error output from the pseudoconsole host process.
Why? There is already a better API for resizing the console in SetConsoleScreenBufferInfo, which wouldn’t require us to expose an implementation detail to a child process.

src/host/ft_host/Host.FeatureTests.vcxproj

src/winconpty/device.h

src/winconpty/winconpty.cpp

miniksa · 2019-09-03T22:53:54Z

src/winconpty/winconpty.vcxproj

+    <ClCompile Include="precomp.cpp">
+      <PrecompiledHeader>Create</PrecompiledHeader>
+    </ClCompile>
+    <ClCompile Include="../server/DeviceHandle.cpp" />


Eh, good enough.

OpenConsole.sln

Biswa96 · 2019-11-02T14:59:00Z

Will it be possible to add a option or flag not to append --headless option in the conhost.exe command line? Because without the --headless option, the hidden conhost window shows raw buffer. It may be useful for future debugging to normal users.

miniksa · 2019-11-04T17:16:53Z

Will it be possible to add a option or flag not to append --headless option in the conhost.exe command line? Because without the --headless option, the hidden conhost window shows raw buffer . It may be useful for future debugging to normal users.

We're not planning on adding debugging options like that to the actual public-facing API or DLL. It is a valid debugging scenario to just launch the conhost yourself manually passing the PTY handles and without the flag if you want to see the hidden window for debugging. I believe the vtpipeterm.exe tool in our project has the potential to do that for debugging purposes.

DHowett added 2 commits August 29, 2019 16:15

conpty: add winconpty (conpty.dll)!

80c481e

Add support for conpty.dll launching OpenConsole.exe

aa209e4

DHowett-MSFT requested a review from miniksa August 30, 2019 00:09

DHowett-MSFT commented Aug 30, 2019

View reviewed changes

zadjii-msft reviewed Sep 3, 2019

View reviewed changes

DHowett added 2 commits September 3, 2019 15:44

feedback, plus moving closer to inbox

60b123c

Add a dependency from ft-host -> conpty

39596da

miniksa approved these changes Sep 3, 2019

View reviewed changes

feedback part II: the feedbackening

c61690a

zadjii-msft approved these changes Sep 4, 2019

View reviewed changes

OpenConsole.sln Show resolved Hide resolved

DHowett-MSFT merged commit e0762f6 into master Sep 4, 2019

DHowett-MSFT deleted the dev/duhowett/winconpty branch September 4, 2019 19:03

Add a project that produces conpty.dll #2611

Add a project that produces conpty.dll #2611

Uh oh!

Conversation

DHowett-MSFT commented Aug 30, 2019

Summary of the Pull Request

References

PR Checklist

Validation Steps Performed

Uh oh!

DHowett-MSFT Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

miniksa Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

fcharlie commented Aug 30, 2019

Uh oh!

driver1998 commented Aug 30, 2019

Uh oh!

fcharlie commented Aug 31, 2019

Uh oh!

Biswa96 commented Aug 31, 2019

Uh oh!

DHowett-MSFT commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

DHowett-MSFT Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

Biswa96 commented Sep 3, 2019

Uh oh!

zadjii-msft commented Sep 3, 2019

Uh oh!

DHowett-MSFT commented Sep 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Biswa96 commented Nov 2, 2019

Uh oh!

miniksa commented Nov 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DHowett-MSFT commented Sep 3, 2019 •

edited

Loading