Bug in handle duplication code causes crash in child process

[Michael Bikovitsky]

The current handle duplication code (here) has a bug that may cause the child process to crash, or just fail in unexpected ways.

I have a detailed explanation of the bug here, but the gist is that the code currently closes the standard I/O handles before launching the child process, but the array in lpReserved2 may still refer to those closed handles, so effectively the child gets garbage handle values.

In essence, the current code performs two tasks:

1. Duplicates the standard I/O handles, making them inheritable, and writes them into the STARTUPINFOW structure.
2. Closes the original standard I/O handles.

To fix the bug then, the code has to also modify the lpReserved2 array with the duplicated handles.

Additionally, the code currently does not close the duplicated handles before waiting for the child process to exit, which can cause the same bug described here (referenced by the comment in the code). This can be solved by closing the handles after launching the child process, but before waiting for it to exit.

[Vinay Sajip]

Thanks for the detailed analysis. I hope to get to working on the fix sometime near the end of March, due to other commitments before then.

[Michael Bikovitsky]

Thanks!

Actually, when I think about it, my proposed fix is wrong. The lpReserved2 array may have different handles than those in the usual STARTUPINFOW fields. So overwriting them with duplicates of whatever GetStdHandle returns is not good.

You could duplicate the handles in the lpReserved2 array too, before closing everything, but at this point I have to wonder - is the duplication really necessary? Why not pass the STARTUPINFOW structure as-is to CreateProcessW?

[Vinay Sajip]

Why not pass the STARTUPINFOW structure as-is to CreateProcessW?

I’ll have to try that. It’s a bit groping-about-in-the-dark, as this stuff seems to be poorly documented, or not at all. There was a lot of discussion on https://github.com/pypa/pip/issues/10444 but no-one has proposed any PRs with suggested working code! Only suggestions for approaches, which is great, but it’s not immediately clear what will actually work …

[Michael Bikovitsky]

And now there is pull request #2

I did some limited testing on my machine, and it seems to work. I’ve also posted comments in pypa/pip#10444 and in microsoft/vscode-python#18561, perhaps somebody else will be willing to test the new launchers as well.

[Vinay Sajip]

Thanks. I still don’t have much bandwidth to look at this until near the end of March