bdist_wininst created binaries fail to start and find 32bit Pythons

[mhammond]

BPO	26071
Nosy	@mhammond, @pfmoore, @larryhastings, @tjguk, @ned-deily, @zware, @eryksun, @zooba
Files	bdist.patch bdist.patch

^{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 = 'https://github.com/zooba'
closed_at = <Date 2016-12-13.17:20:42.429>
created_at = <Date 2016-01-10.01:06:48.592>
labels = ['deferred-blocker', 'type-bug', '3.7', 'OS-windows']
title = 'bdist_wininst created binaries fail to start and find 32bit Pythons'
updated_at = <Date 2016-12-13.17:20:42.427>
user = 'https://github.com/mhammond'

bugs.python.org fields:

activity = <Date 2016-12-13.17:20:42.427>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2016-12-13.17:20:42.429>
closer = 'steve.dower'
components = ['Windows']
creation = <Date 2016-01-10.01:06:48.592>
creator = 'mhammond'
dependencies = []
files = ['41561', '41575']
hgrepos = []
issue_num = 26071
keywords = ['patch', '3.4regression']
message_count = 20.0
messages = ['257879', '257885', '257887', '257889', '257894', '257895', '257899', '257916', '257919', '257921', '257950', '257951', '257957', '258423', '258424', '283087', '283105', '283114', '283124', '283127']
nosy_count = 10.0
nosy_names = ['mhammond', 'paul.moore', 'larry', 'tim.golden', 'ned.deily', 'cgohlke', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower']
pr_nums = []
priority = 'deferred blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26071'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[mhammond]

Binaries created by bdist_wininst fail on 3.5+ for 2 reasons:

• The built binary links against the DLL version of the CRT. This means the binary will typically fail to start as the CRT DLL isn't found.

• When looking for 32bit Python versions, it fails to take the recent "-32" change for the registry key, so it tells you no Python is installed.

This patch:

• Uses the static CRT, which IIRC, was the case in previous Python versions.
• Changes directory to the directory with pythonxx.dll before attempting to loadlibrary it - this allows the loadlibrary to succeed as the CRT DLL in that directory is found.
• Appends "-32" to registry keys on 32bit builds.

With these patches I can successfully get a pywin32 build working and installing.

Steve (or anyone), what do you think?

(Note that the attached patch does *not* include a newly built wininst-14.exe, but that would obviously need to be updated before checking in.)

[zooba]

Looks good to me. Feel free to check it in, Mark, or one of us will get to it if you're not set up for committing right now.

[eryksun]

I would call SetDllDirectory instead of changing the current directory. This replaces the current directory in the DLL search. Then call SetDllDirectory(NULL) to restore the default before returning.

When you say "the CRT assembly", you're talking about vcruntime140.dll, right? This module implements exceptions, setjmp/longjmp, and other functions that are closely coupled with the VC++ version, but most of the universal CRT is implemented in the system DLL ucrtbase.dll.

vcruntime was initially linked statically everywhere, but that was a problem for extension modules, since each instance was taking a fiber locale storage slot.

However, it really should be static for the bdist_wininst installer. I believe Steve had the link configuration customized as a partially static build that linked dynamically with ucrtbase.dll. This benefits from continuous CRT bug fixes and security updates. To configure this he added ucrt[d].lib as a dependency and ignored the default library libucrt[d].lib. See the following diff for the changes that were made to revert this:

https://hg.python.org/cpython/diff/8374472c6a6e/PCbuild/pyproject.props

[zooba]

In this case, it needs to load vcruntime for the python##.dll, so linking isn't going to make any difference. We need to statically link the loader, since it will be run independently and can't have any dependencies, but when it finds a Python install and loads the DLL directly it's not finding the vcruntime adjacent to the DLL. We'd have to statically link python##.dll.

SetDllDirectory would also work, though it's a little more complicated to get right. It's probably worth it here though to set the search path around the load in this case, as we know that nobody else is expecting the value to not change. However, it may break other loads if python##.dll calls it or makes assumptions about the search path (which it almost certainly does). Setting the working directory is likely to lead to a more reliable initial state.

[eryksun]

We'd have to statically link python##.dll.

I was talking about the partially static build that you used for wininst-14.0-amd64.exe in rc3, in which vcruntime is linked statically but ucrt is linked dynamically (actually to the api-ms-win-crt-* API sets). But is there a bad assumption there? I was only thinking about installing to systems with Python 3.5+, on which ucrtbase.dll is guaranteed to be in System32, but this installer should work for older systems that don't have the universal CRT update, right?

However, it may break other loads if python##.dll calls it or
makes assumptions about the search path

I don't understand. It would be pretty broken if python##.dll depends on the current directory. All SetDllDirectory does is swap in the target directory in place of the current directory in the DLL search path. I made the suggestion because it's cleaner to surgically change only what the system loader sees, rather than changing the current directory, which affects everything. The LoadLibrary docs recommend using SetDllDirectory instead of changing the current directory. Using AddDllDirectory would be better, but that's only available on updated Vista/7 systems (KB2533623).

[eryksun]

Steve, I think I understand the linking problem now -- in terms of the bad assumption I mentioned in the previous message. Mark said the "built binary links against the DLL version of the CRT", but I just checked in 3.5.1 that you actually didn't switch that one to link against vcruntime140.dll like you did with everything else, so he's only referring to the api-set-ms-win-crt dependencies. This would only fail on systems that don't have the universal CRT update. I thought he meant the executable was typically failing on all systems that don't have Visual Studio, which installs vcruntime140.dll in System32.

[mhammond]

Mark said the "built binary links against the DLL version of the CRT
but I just checked in 3.5.1 that you actually didn't switch that
one to link against vcruntime140.dll like you did with everything else

Yeah, I didn't dig too much further here, but I'm fairly sure that "static crt" has been a thing for quite a while now for that stub (there are comments about that around the SetStdHandle calls in that file). I suspect it was accidentally lost in one of the build system fixups that happened (which seem great and a vast improvement!)

[zooba]

Ultimately, launching python.exe is better than loading python##.dll here because it avoids DLL planting vulnerabilities, but I'm honestly a little surprised still that LoadLibrary won't resolve vcruntime in this case. Been a while since I looked, and I haven't had a chance to check for this yet, but why isn't the directory containing the loaded DLL searched for dependencies?

[eryksun]

why isn't the directory containing the loaded DLL searched
for dependencies?

That's possible. The loader provides several ways to go about solving this problem. The directory of the DLL can be added to the search path if you use LoadLibraryEx with the flag LOAD_WITH_ALTERED_SEARCH_PATH and use an absolute path. Python already does this for loading extension modules.

If we didn't have to worry about older systems, in Windows 8/10 (and Vista/7 with KB2533623), the loader supports a set of flags that give detailed control of the DLL path, including LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR. Individual directories can be added via AddDllDirectory and enabled via the flag LOAD_LIBRARY_SEARCH_USER_DIRS. Default flags can be set via SetDefaultDllDirectories, which affects LoadLibrary as well as LoadLibraryEx.

On a related note, it would be nice to modify ctypes to call LoadLibraryEx instead of LoadLibrary. If the flags value is 0 the two calls are identical, so it wouldn't be a disruptive change. The existing dlopen "mode" parameter can be used for the flags, since it isn't used on Windows currently.

[zooba]

The directory of the DLL can be added to the search path if you use LoadLibraryEx with the flag LOAD_WITH_ALTERED_SEARCH_PATH and use an absolute path.

That sounds like the better fix then. I thought there was something along those lines available.

We already use the full path to load it - can someone test this change? If not I'll give it a go later today.

[mhammond]

> The directory of the DLL can be added to the search path if you
> use LoadLibraryEx with the flag LOAD_WITH_ALTERED_SEARCH_PATH and use an absolute path.

That sounds like the better fix then. I thought there was something along those lines available.

That does work. I didn't do it that way initially as I felt your earlier comment:

Setting the working directory is likely to lead to a more reliable initial state.

Was an additional nice outcome of just changing the cwd. This patch does the LoadLibraryEx() and works (but obviously leaves the CWD being the downloaded directory, probably %temp%)

I'll check this in once I get the green-light, thanks.

[mhammond]

welp, I hadn't actually tested the patch on x64 yet ;)

#ifdef WIN_64

should read:

#ifdef _WIN64

WIN_64 is a Python invented name and this stub doesn't Python pull the headers in...

[zooba]

The downside of the preprocessor - no errors when you spell an ifdef wrong ;)

It looks good to me.

[python-dev]

New changeset daa6fdaf9835 by Steve Dower in branch '3.5':
Issue bpo-26071: bdist_wininst created binaries fail to start and find 32bit Python
https://hg.python.org/cpython/rev/daa6fdaf9835

New changeset db74f3a4cbeb by Steve Dower in branch 'default':
Issue bpo-26071: bdist_wininst created binaries fail to start and find 32bit Python
https://hg.python.org/cpython/rev/db74f3a4cbeb

[zooba]

Applied the patch and rebuilt the stubs, so this will show up in 3.5.2 and 3.6.

[cgohlke]

The applied patch breaks bdist_wininst installers for 64-bit Python. See <http://bugs.python.org/issue28680\>.

[zooba]

Looks like I acknowledged the error in the patch and then didn't fix it when I applied it.

The change needed here is what Mark describes above, plus recompiling the executable stubs. Or alternatively, I can probably just edit out the bytes in the existing stub for 3.6.0 and fix it properly for 3.6.1.

Ned - thoughts? This regressed from 3.4 and has not worked properly for both architectures (simultaneously) since 3.5.

[ned-deily]

Since we are two days from the final release, the fix requires rebuilding and replacing a binary file in the source repo, and you state (in related bpo-28680): "Considering we've gone through all of 3.5 and 3.6 with very few people noticing (and as far as I'm aware, completely able to fix it themselves), it may not make the initial 3.6 release.", I would prefer to hold this for 3.6.1.

[python-dev]

New changeset c3da1ee47e6b by Steve Dower in branch '3.5':
Issue bpo-26071: Fixes preprocessor definition and rebuilds wininst-14.0[-amd64].exe
https://hg.python.org/cpython/rev/c3da1ee47e6b

New changeset 0528a6743018 by Steve Dower in branch '3.6':
Issue bpo-26071: Fixes preprocessor definition and rebuilds wininst-14.0[-amd64].exe
https://hg.python.org/cpython/rev/0528a6743018

New changeset 4e9d899fcb65 by Steve Dower in branch 'default':
Issue bpo-26071: Fixes preprocessor definition and rebuilds wininst-14.0[-amd64].exe
https://hg.python.org/cpython/rev/4e9d899fcb65

[zooba]

Fixed for 3.5.3, 3.6.1 and default.