bpo-36085: Enable better DLL resolution on Windows #12302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eryksun
reviewed
Mar 13, 2019
eryksun
reviewed
Mar 13, 2019
eryksun
reviewed
Mar 13, 2019
eryksun
reviewed
Mar 14, 2019
Modules/_ctypes/callproc.c
Outdated
| */ | ||
| hMod = LoadLibraryExW(name, NULL, | ||
| LOAD_LIBRARY_SEARCH_DEFAULT_DIRS | | ||
| LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR); |
There was a problem hiding this comment.
We cannot use LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR as a default in ctypes. It will fail the call as an invalid parameter if name isn't fully qualified. This flag doesn't even allow paths that are explicitly relative to the working directory due to the way path searching works in Windows. For example, ".\spam\eggs.dll" is joined with every directory in the computed DLL search path (e.g. "C:\Windows\System32\.\spam\eggs.dll"). The secure search path never includes the current working directory, so the loader can't assume we want the given path to be resolved relative to the working directory in order to compute the DLL directory. It has to fail the call. (In contrast, with POSIX dlopen "./spam/eggs.so" is only resolved relative to the working directory.)
There was a problem hiding this comment.
Yes, you're right., I'll remove it again.
Making the second argument consistent with dlopen is a much bigger task that I can't afford to take on right now, so I'm going to leave that open. We can discuss on the bug whether that should block the entire change or not.
There was a problem hiding this comment.
Do you think something should be done about the relative-path issue? The behavior is documented:
If lpFileName specifies a relative path, the entire relative path is appended to every token in the DLL search path. To load a module from a relative path without searching any other path, use GetFullPathName to get a nonrelative path and call LoadLibraryEx with the nonrelative path.
IMO, this behavior is counter-intuitive and disagrees with POSIX. I'd expect CDLL("spam/eggs.dll") to resolve the path relative to the working directory. I don't think most people would intuit that the system actually searches relative to each directory in the computed DLL search path. This seems to work with the expected behavior when the current directory is in the DLL search path. Albeit, with the default safe order, the working directory is checked after the application, System32, and Windows directories, but a collision with a system DLL is unlikely. On the other hand, with the secure search path, which doesn't include the working directory, scripts will almost definitely fail if they depend on an explicit reference to the working directory (e.g. "./eggs.dll" or "spam/eggs.dll").
If we were Windows only, I'd point people to the documentation and tell them to adjust their expectations appropriately. But we're cross-platform, and the Windows behavior is the odd duck here. Maybe we should call GetFullPathNameW on any path that includes slash or backslash in order to match expected behavior.
There was a problem hiding this comment.
Alternatively, ctypes could fail the call as a ValueError if the name contains slash or backslash and isn't fully qualified. This avoids the unexpected search and enforces a policy that ctypes in Windows will no longer search the working directory for unqualified names nor load DLLs relative to the working directory. The downside with this approach is that it's inconsistent with POSIX.
Note that os.add_dll_directory won't allow adding ".", so there's no supported way for users to sneak the old behavior back in anyway. The working directory can be restored to the search path by using ctypes to call SetDllDirectoryW("."), but that's beyond our control.
|
@eryksun It looks like |
|
|
It does not appear to handle this. I added a spam service key that includes the "Performance" subkey and "Library", "Open", "Close", and "Collect" values. I set the library value to "spam.dll", in which I hard coded the exported Since it ended up in |
I took a look at |
|
So on one hand, it's a system misconfiguration, and on the other, we can't get this change past CI (because they have a misconfigured system). I guess we have to leave out the set-default call and live with "I'm not sure how my DLLs are going to be resolved" as a policy. |
eryksun
reviewed
Mar 27, 2019
mattip
reviewed
Mar 28, 2019
mattip
reviewed
Mar 28, 2019
|
The Pipelines failure is an unrelated Linux issue, so ignoring the failure to merge. |
|
lazka
added a commit
to lazka/MINGW-packages
that referenced
this pull request
Apr 16, 2019
…#5146 This is also a problem with meson which tries to execute things in the build directory using the build directory DLLs by prepending those to PATH. Since meson uses a Python helper the PATH gets updated to include the system prefix first which makes things link against the installed libraries instead and fail because symbols are missing. One of the reasons why this was added in the first place is that Python loads C extensions in lib-dynload which then can't find the libraries in prefix (e.g. "import tkinter") if it isn't in PATH. By moving the prefix at the end of PATH we make both cases work. Starting with Python 3.8 C extensions will no longer use PATH for loading DLL dependencies, see python/cpython#12302 so we will have to look into this again then.
lazka
added a commit
to lazka/MINGW-packages
that referenced
this pull request
Apr 16, 2019
…5146 This is also a problem with meson which tries to execute things in the build directory using the build directory DLLs by prepending those to PATH. Since meson uses a Python helper the PATH gets updated to include the system prefix first which makes things link against the installed libraries instead and fail because symbols are missing. One of the reasons why this was added in the first place is that Python loads C extensions in lib-dynload which then can't find the libraries in prefix (e.g. "import tkinter") if it isn't in PATH. By moving the prefix at the end of PATH we make both cases work. Starting with Python 3.8 C extensions will no longer use PATH for loading DLL dependencies, see python/cpython#12302 so we will have to look into this again then.
stahta01
pushed a commit
to stahta01/MINGW-packages
that referenced
this pull request
Apr 21, 2019
…5146 This is also a problem with meson which tries to execute things in the build directory using the build directory DLLs by prepending those to PATH. Since meson uses a Python helper the PATH gets updated to include the system prefix first which makes things link against the installed libraries instead and fail because symbols are missing. One of the reasons why this was added in the first place is that Python loads C extensions in lib-dynload which then can't find the libraries in prefix (e.g. "import tkinter") if it isn't in PATH. By moving the prefix at the end of PATH we make both cases work. Starting with Python 3.8 C extensions will no longer use PATH for loading DLL dependencies, see python/cpython#12302 so we will have to look into this again then.
3 tasks
jobovy
added a commit
to jobovy/galpy
that referenced
this pull request
Mar 18, 2020
…rch path for the DLLs (see python/cpython#12302)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://bugs.python.org/issue36085