classification
Title: [EASY C] inherit the py launcher's STARTUPINFO
Type: enhancement Stage: resolved
Components: Windows Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: GeekyShacklebolt, eryksun, miss-islington, paul.moore, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: easy (C), patch

Created on 2018-01-15 21:25 by eryksun, last changed 2019-02-02 19:38 by miss-islington. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9000 merged GeekyShacklebolt, 2018-08-29 17:13
PR 11739 merged miss-islington, 2019-02-02 19:21
PR 11739 merged miss-islington, 2019-02-02 19:21
PR 11739 merged miss-islington, 2019-02-02 19:21
Messages (12)
msg310016 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2018-01-15 21:25
I've occasionally seen requests to find the path of the LNK shortcut that was used to run a script -- for whatever reason. This can be reliably supported in most cases by calling WinAPI GetStartupInfo. If the flag STARTF_TITLEISLINKNAME is set, then the lpTitle field is the path to the LNK file. (This is how the console host process knows to use the shortcut file for its properties instead of the registry.)

However, if .py scripts are associated with the py launcher, the STARTUPINFO that has the required information is in the launcher process instead of the python.exe process. Since to some extent the launcher is acting as a proxy for python.exe, I think it should initialize Python's STARTUPINFO from a copy of its own values as returned by GetStartupInfo. This solves the dwFlags and lpTitle problem.

Additionally, the C runtime spawn family of functions supports inheriting file descriptors by passing them in the lpReserved2 field of STARTUPINFO. Currently this doesn't work when spawning Python via the launcher, since it calls CreateProcess instead of a spawn function. By inheriting the launcher's STARTUPINFO, this feature is restored as well. 

For example, in `run_child` in PC/launcher.c, I replaced `si.cb = sizeof(si)` with GetStartupInfoW(&si). Then I tested passing inherited file descriptors through the launcher as follows:

grandparent:

    >>> import os
    >>> os.pipe()
    (3, 4)
    >>> os.set_inheritable(4, True)
    >>> os.spawnl(os.P_WAIT, 'amd64/py_d.exe', 'py_d')

grandchild (py_d.exe => python.exe):

    Python 3.7.0a4 (v3.7.0a4:07c9d85, Jan  9 2018, 07:07:02)
    [MSC v.1900 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import os
    >>> os.write(4, b'spam')
    4
    >>> ^Z

grandparent:

    0
    >>> os.read(3, 4)
    b'spam'
msg310031 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-01-16 01:44
This sounds like a good idea to me - feel free to contribute your PR.
msg321541 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-12 10:56
"inherit the py launcher's STARTUPINFO"

I read the issue twice, and honestly I have no idea how to implement that. So I removed the "easy (C)" keyword.

If you still consider that it's an "easy (C)" issue, please describe step by step how to implement it:

* which files should be modified?
* which function can be used?
* how is STARTUPINFO supposed by inherited?
* do you want to expose STARTUPINFO at Python level?

If you plan to use a pipe, it's tricky to make sure that the pipe is closed properly on both sides once STARTUPINFO is transferred.
msg321546 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-07-12 13:04
The "for example" line is the fix, it really is very simple. That said, we have no automatic validation for the launcher, and coming up with a good set of manual tests to check it is certainly not easy.
msg321549 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-12 13:36
Ah, so the task is just open PC/launcher.c and replaced "si.cb = sizeof(si)" with "GetStartupInfoW(&si)".

Ok, I agree, that's easy and I would be happy to review such PR ;-) I add  the "[EASY C]" tag to the issue title in this case.
msg324340 - (view) Author: Shiva Saxena (GeekyShacklebolt) * Date: 2018-08-29 17:36
I am a new contributor, and happy to take this issue as my first contribution in cpython. I have sent a PR for the same. Please let me know if anything needs to be change.
msg334749 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 16:13
Hi Shiva, sorry for not noticing your PR earlier.

I have one question for Eryk, who may be able to answer more easily than I can: do we need to initialize cbSize before calling GetStartupInfoW()?

The docs[1] don't seem to suggest it, and I can't look at the implementation of that API until Monday, but perhaps you know?

1: https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-getstartupinfow
msg334760 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-02-02 18:05
It should be fine. If the docs don't require initializing cb, we can assume it's done for us. 

For example, msvcrt.dll calls GetStartupInfo without initializing this field:

    0:000> kc 3
    Call Site
    KERNELBASE!GetStartupInfoW
    msvcrt!ioinit
    msvcrt!__CRTDLL_INIT

In x64, the first argument (lpStartupInfo) is in rcx. We see the DWORD (dd) value of cb is initially 0:

    0:000> dd @rcx l1
    00000094`25ddefd0  00000000

Continue to the ret[urn] instruction via pt and check that the returned value of cb is sizeof(*lpStartupInfo):

    0:000> pt
    KERNELBASE!GetStartupInfoW+0xb2:
    00007fff`8ae41282 c3              ret
    0:000> dd 94`25ddefd0 l1
    00000094`25ddefd0  00000068
msg334761 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-02-02 18:17
Oops, I forgot the check:

    0:000> ?? sizeof(test!_STARTUPINFOW)
    unsigned int64 0x68
msg334764 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-02-02 18:59
Great, thanks. I'll go ahead and merge.
msg334765 - (view) Author: miss-islington (miss-islington) Date: 2019-02-02 19:21
New changeset cb0904762681031edc50f9d7d7ef48cffcf96d9a by Miss Islington (bot) (Shiva Saxena) in branch 'master':
bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000)
https://github.com/python/cpython/commit/cb0904762681031edc50f9d7d7ef48cffcf96d9a
msg334766 - (view) Author: miss-islington (miss-islington) Date: 2019-02-02 19:38
New changeset 04b2a5eedac7ac0fecdafce1bda1028ee55e2aac by Miss Islington (bot) in branch '3.7':
bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000)
https://github.com/python/cpython/commit/04b2a5eedac7ac0fecdafce1bda1028ee55e2aac
History
Date User Action Args
2019-02-02 19:38:18miss-islingtonsetnosy: + miss-islington
messages: + msg334766
2019-02-02 19:22:37steve.dowersetstatus: open -> closed
nosy: - miss-islington

resolution: fixed
stage: patch review -> resolved
2019-02-02 19:21:41miss-islingtonsetpull_requests: + pull_request11651
2019-02-02 19:21:30miss-islingtonsetpull_requests: + pull_request11650
2019-02-02 19:21:20miss-islingtonsetpull_requests: + pull_request11649
2019-02-02 19:21:09miss-islingtonsetnosy: + miss-islington
messages: + msg334765
2019-02-02 18:59:58steve.dowersetmessages: + msg334764
2019-02-02 18:17:33eryksunsetmessages: + msg334761
2019-02-02 18:05:49eryksunsetmessages: + msg334760
2019-02-02 16:13:54steve.dowersetmessages: + msg334749
2018-08-29 17:36:48GeekyShackleboltsetnosy: + GeekyShacklebolt
messages: + msg324340
2018-08-29 17:13:37GeekyShackleboltsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request8472
2018-07-12 13:36:54vstinnersetmessages: + msg321549
title: inherit the py launcher's STARTUPINFO -> [EASY C] inherit the py launcher's STARTUPINFO
2018-07-12 13:04:15steve.dowersetmessages: + msg321546
2018-07-12 10:56:55vstinnersetnosy: + vstinner
messages: + msg321541
2018-01-16 01:44:13steve.dowersetmessages: + msg310031
versions: + Python 3.7
2018-01-15 21:25:15eryksuncreate