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

[EASY C] inherit the py launcher's STARTUPINFO #76741

Closed
eryksun opened this issue Jan 15, 2018 · 12 comments
Closed

[EASY C] inherit the py launcher's STARTUPINFO #76741

eryksun opened this issue Jan 15, 2018 · 12 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes easy OS-windows type-feature A feature request or enhancement

Comments

@eryksun
Copy link
Contributor

eryksun commented Jan 15, 2018

BPO 32560
Nosy @pfmoore, @vstinner, @tjguk, @zware, @eryksun, @zooba, @miss-islington, @GeekyShacklebolt
PRs
  • bpo-32560: inherit the py launcher's STARTUPINFO #9000
  • [3.7] bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000) #11739
  • [3.7] bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000) #11739
  • [3.7] bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000) #11739
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2019-02-02.19:22:37.820>
    created_at = <Date 2018-01-15.21:25:15.553>
    labels = ['easy', '3.8', 'type-feature', '3.7', 'OS-windows']
    title = "[EASY C] inherit the py launcher's STARTUPINFO"
    updated_at = <Date 2019-02-02.19:38:18.853>
    user = 'https://github.com/eryksun'

    bugs.python.org fields:

    activity = <Date 2019-02-02.19:38:18.853>
    actor = 'miss-islington'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-02-02.19:22:37.820>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2018-01-15.21:25:15.553>
    creator = 'eryksun'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32560
    keywords = ['patch', 'easy (C)']
    message_count = 12.0
    messages = ['310016', '310031', '321541', '321546', '321549', '324340', '334749', '334760', '334761', '334764', '334765', '334766']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'miss-islington', 'GeekyShacklebolt']
    pr_nums = ['9000', '11739', '11739', '11739']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue32560'
    versions = ['Python 3.7', 'Python 3.8']

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Jan 15, 2018

    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'

    @eryksun eryksun added 3.8 only security fixes OS-windows easy type-feature A feature request or enhancement labels Jan 15, 2018
    @zooba
    Copy link
    Member

    zooba commented Jan 16, 2018

    This sounds like a good idea to me - feel free to contribute your PR.

    @zooba zooba added the 3.7 (EOL) end of life label Jan 16, 2018
    @vstinner
    Copy link
    Member

    "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.

    @zooba
    Copy link
    Member

    zooba commented Jul 12, 2018

    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.

    @vstinner
    Copy link
    Member

    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.

    @vstinner vstinner changed the title inherit the py launcher's STARTUPINFO [EASY C] inherit the py launcher's STARTUPINFO Jul 12, 2018
    @GeekyShacklebolt
    Copy link
    Mannequin

    GeekyShacklebolt mannequin commented Aug 29, 2018

    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.

    @zooba
    Copy link
    Member

    zooba commented Feb 2, 2019

    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

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Feb 2, 2019

    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
    

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Feb 2, 2019

    Oops, I forgot the check:

    0:000> ?? sizeof(test!_STARTUPINFOW)
    unsigned int64 0x68
    

    @zooba
    Copy link
    Member

    zooba commented Feb 2, 2019

    Great, thanks. I'll go ahead and merge.

    @miss-islington
    Copy link
    Contributor

    New changeset cb09047 by Miss Islington (bot) (Shiva Saxena) in branch 'master':
    bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000)
    cb09047

    @zooba zooba closed this as completed Feb 2, 2019
    @miss-islington
    Copy link
    Contributor

    New changeset 04b2a5e by Miss Islington (bot) in branch '3.7':
    bpo-32560: inherit the py launcher's STARTUPINFO (GH-9000)
    04b2a5e

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes easy OS-windows type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants