classification
Title: _Py_CheckPython3 uses uninitialized dllpath when embedder sets module path with Py_SetPath
Type: behavior Stage: resolved
Components: Windows Versions: Python 3.9, Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Tibor Csonka, anthonywee, miss-islington, paul.moore, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords:

Created on 2017-03-10 04:58 by Tibor Csonka, last changed 2020-01-07 21:29 by vstinner.

Pull Requests
URL Status Linked Edit
PR 17818 merged anthonywee, 2020-01-04 02:13
PR 17871 merged miss-islington, 2020-01-06 16:59
Messages (14)
msg289334 - (view) Author: Tibor Csonka (Tibor Csonka) Date: 2017-03-10 04:58
When Py_SetPath is used to set up module path at initialization, the Py_SetPath causes getpathp.c::calculate_path not to be called. However, calculate path is the only function calling getpathp.c::get_progpath which initializes the local dllpath static variable.

Later the interpreter tries to load python3.dll and uses dllpath which is empty by default. This empty path gets joined with \python3.dll and \DLLs\python3.dll which is used in the LoadLibraryExW resulting in loading python3.dll from the root location of the windows drive the application is running from.

The behavior was reproduced using PyInstaller but it is present in any embedding application which uses Py_SetPath.
msg289364 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-03-10 14:23
I thought we'd documented that if you set the path when embedding you should also set the program name, but perhaps not (didn't check just now). If not, we should do that.

We shouldn't be loading python3.dll anywhere. Are you sure that's in CPython? Do you have a reference to the source file?
msg289412 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-03-10 21:50
Ah, I see. We force load it in PC/getpathp.c to ensure that it's ours and not another version's python3.dll.

We should probably refactor the GetModuleFileNameW call into its own function so we can call it from anywhere we need.
msg343638 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-27 15:11
> When Py_SetPath is used to set up module path at initialization, the Py_SetPath causes getpathp.c::calculate_path not to be called. However, calculate path is the only function calling getpathp.c::get_progpath which initializes the local dllpath static variable.

I fixed this issue in Python 3.8 with this commit:

commit 410759fba80aded5247b693c60745aa16906f3bb
Author: Victor Stinner <vstinner@redhat.com>
Date:   Sat May 18 04:17:01 2019 +0200

    bpo-36763: Remove _PyCoreConfig.dll_path (GH-13402)

I modified Py_SetPath() like that:

-    new_config.dll_path = _PyMem_RawWcsdup(L"");
+    new_config.dll_path = _Py_GetDLLPath();

Py_SetPath() no longer sets dll_path to an empty string.

Since we only got one bug report and I believe that Tibor Csonka found a way to workaround the issue since he reported it, I close the issue.

Please reopen/comment the issue if you would like to get this issue fixed in Python 3.7 as well.

--

Moreover, the PEP 587 now has a better API to configure embedded Python. I just implemented this PEP in bpo-36763.
msg359136 - (view) Author: Anthony Wee (anthonywee) * Date: 2020-01-01 00:00
It looks like there has been a regression in the fix for this issue. 

The commit below introduced a NULL check which causes a call to _PyPathConfig_Init() to be skipped if _Py_dll_path == NULL. It seems like the check should be "if (_Py_dll_path != NULL)"?

https://github.com/python/cpython/commit/c422167749f92d4170203e996a2c619c818335ea#diff-87aed37b4704d4e1513be6378c9c7fe6R169
msg359245 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2020-01-03 18:33
> It looks like there has been a regression in the fix for this issue.

You're right. Care to create a pull request to fix it?
msg359438 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2020-01-06 16:57
New changeset 7b79dc9200a19ecbac667111dffd58e314be02a8 by Steve Dower (Anthony Wee) in branch 'master':
bpo-29778: Fix incorrect NULL check in _PyPathConfig_InitDLLPath() (GH-17818)
https://github.com/python/cpython/commit/7b79dc9200a19ecbac667111dffd58e314be02a8
msg359439 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2020-01-06 16:58
Thanks, Anthony! And congratulations on becoming a CPython contributor!
msg359440 - (view) Author: miss-islington (miss-islington) Date: 2020-01-06 17:17
New changeset a9a43c221bf3896ed1d1c2eee2531b7121cf78e4 by Miss Islington (bot) in branch '3.8':
bpo-29778: Fix incorrect NULL check in _PyPathConfig_InitDLLPath() (GH-17818)
https://github.com/python/cpython/commit/a9a43c221bf3896ed1d1c2eee2531b7121cf78e4
msg359441 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-06 17:22
Oops, I'm guilty of pushing this change! Sorry & thanks for the fix.

    if (_Py_dll_path == NULL) {
        /* Already set: nothing to do */
        return _PyStatus_OK();
    }
msg359498 - (view) Author: Anthony Wee (anthonywee) * Date: 2020-01-07 09:01
Thank you Steve!

I'm still seeing python3.dll being loaded from \DLLs\python3.dll.

_Py_CheckPython3() uses Py_GetPrefix() as a prefix for \DLLs\python3.dll.

It looks like Py_SetPath() sets the _Py_path_config.prefix to "", but I'm not seeing anything else set it to a real value?

https://github.com/python/cpython/blob/7b79dc9200a19ecbac667111dffd58e314be02a8/Python/pathconfig.c#L508
msg359544 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-07 20:56
> It looks like Py_SetPath() sets the _Py_path_config.prefix to "", but I'm not seeing anything else set it to a real value?

In the master branch, _Py_CheckPython3() doesn't use _Py_path_config.prefix.

_PyPathConfig_InitDLLPath() calls GetModuleFileNameW(PyWin_DLLhModule, dll_path, MAXPATHLEN) if PyWin_DLLhModule is initialized.

For example, _PyPathConfig_InitDLLPath() is called by Py_Initialize() and Py_SetPath().

PyWin_DLLhModule is initialized by DllMain().

The code in the 3.8 branch looks very similar (I backported my "Remove _PyPathConfig.dll_path" change to 3.8: commit 9f3dcf802eefeb5ab821ce3c7204ab46557d53d7).
msg359549 - (view) Author: Anthony Wee (anthonywee) * Date: 2020-01-07 21:22
Hm, I'm seeing _Py_CheckPython3() use Py_GetPrefix(), which uses _Py_path_config.prefix?

https://github.com/python/cpython/blob/c02b41b1fb115c87693530ea6a480b2e15460424/PC/getpathp.c#L1185
msg359550 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-01-07 21:29
> Hm, I'm seeing _Py_CheckPython3() use Py_GetPrefix(), which uses _Py_path_config.prefix?

Oh right, that's the initial issue:

> Later the interpreter tries to load python3.dll and uses dllpath which is empty by default. This empty path gets joined with \python3.dll and \DLLs\python3.dll which is used in the LoadLibraryExW resulting in loading python3.dll from the root location of the windows drive the application is running from.

I reopen the issue.
History
Date User Action Args
2020-01-07 21:29:23vstinnersetstatus: closed -> open
resolution: fixed ->
messages: + msg359550
2020-01-07 21:22:58anthonyweesetmessages: + msg359549
2020-01-07 20:56:18vstinnersetmessages: + msg359544
2020-01-07 09:01:14anthonyweesetmessages: + msg359498
2020-01-06 17:22:26vstinnersetmessages: + msg359441
2020-01-06 17:17:43miss-islingtonsetnosy: + miss-islington
messages: + msg359440
2020-01-06 16:59:02miss-islingtonsetpull_requests: + pull_request17287
2020-01-06 16:58:32steve.dowersetversions: + Python 3.9
2020-01-06 16:58:20steve.dowersetmessages: + msg359439
2020-01-06 16:57:38steve.dowersetmessages: + msg359438
2020-01-04 02:13:52anthonyweesetpull_requests: + pull_request17244
2020-01-03 18:33:45steve.dowersetmessages: + msg359245
2020-01-01 00:00:46anthonyweesetnosy: + anthonywee
messages: + msg359136
2019-05-27 15:11:15vstinnersetstatus: open -> closed
stage: needs patch -> resolved
resolution: fixed
versions: + Python 3.8, - Python 3.5, Python 3.6, Python 3.7
2019-05-27 15:11:06vstinnersetnosy: + vstinner
messages: + msg343638
2017-03-10 21:50:35steve.dowersetstage: needs patch
type: behavior
versions: + Python 3.6, Python 3.7
2017-03-10 21:50:21steve.dowersetmessages: + msg289412
2017-03-10 14:23:36steve.dowersetmessages: + msg289364
2017-03-10 04:58:18Tibor Csonkacreate