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

gh-89545: Adds internal _wmi module on Windows for directly querying OS properties #96289

Merged
merged 20 commits into from
Sep 7, 2022

Conversation

zooba
Copy link
Member

@zooba zooba commented Aug 25, 2022

@zooba zooba marked this pull request as ready for review August 30, 2022 23:17
@zooba zooba requested a review from a team as a code owner August 30, 2022 23:17
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice enhancement! Some remarks.

Lib/platform.py Outdated Show resolved Hide resolved
Lib/test/test_platform.py Outdated Show resolved Hide resolved
PC/_wmimodule.cpp Outdated Show resolved Hide resolved
PC/_wmimodule.cpp Outdated Show resolved Hide resolved
PC/_wmimodule.cpp Show resolved Hide resolved
PC/_wmimodule.cpp Show resolved Hide resolved
PC/_wmimodule.cpp Outdated Show resolved Hide resolved
Comment on lines +229 to +230
if (offset >= sizeof(buffer)) {
err = ERROR_MORE_DATA;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for the implementation to support a growable buffer with malloc(), realloc(), and free(). The 8 KiB limit is enough for Win32_OperatingSystem and Win32_Processor, but it's more useful in general if the result size isn't limited (e.g. a query that returns an arbitrary number of instances).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an internal API, so we can always add that in when we find we need it in the stdlib.

I don't know of any existing APIs we have that would be more correct using WMI and would need some sort of "query everything" API.

@eryksun
Copy link
Contributor

eryksun commented Sep 1, 2022

I think it's possible to plumb through Ctrl+C cancellation, but I haven't figured it out yet. Probably needs synchronous overlapped IO though, which isn't too bad to do.

I think the most straight-froward pipe-based implementation that supports cancel via Ctrl+C is to use a named pipe. For the read side, call CreateNamedPipeW(). Use the open mode PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED, and pass nOutBufferSize and nInBufferSize as 4096 (i.e. as CreatePipe() does). For the write side, call CreateFileW() with GENERIC_WRITE access, and use synchronous access. Wait for the completion of a read using the OVERLAPPED event. If called on the main thread, also include the Ctrl+C event in the wait. If the Ctrl+C event is signaled and PyErr_CheckSignals() is negative, close the read end of the pipe, free data.query, and return NULL. The worker thread will exit as soon as it tries to write to the broken pipe. Its exit status doesn't matter in this case, so there's no need to wait on it.

@eryksun
Copy link
Contributor

eryksun commented Sep 1, 2022

Note that the _wmi module depends on "ole32.dll" for COM support (i.e. CoInitializeEx, CoInitializeSecurity, CoCreateInstance, CoSetProxyBlanket, and CoUninitialize), which depends on "user32.dll". Loading the latter converts the current process to a GUI process1 and the current thread to a GUI thread. The GUI conversion extends the process and thread objects in the kernel with win32k data structures; connects the process to a window station in the current session (typically "WinSta0"); and connects the thread to a desktop on the window station (typically "Default"). After the conversion, calling GetGUIThreadInfo() on the ID of the converted thread will succeed, whereas prior to conversion it fails as an invalid parameter.

There's no significant performance cost if calling sys.getwindowsversion() converts to a GUI process and GUI thread. It's certainly insignificant compared to the cost of WMI. However, a GUI process doesn't get sent CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT events. Windows only sends these events to a process when none of its threads is connected to a desktop2. Instead, a GUI process is expected to create a window on the desktop (the window can be hidden) and handle WM_QUERYENDSESSION and WM_ENDSESSION messages. It isn't the end of the world to have to switch from handling simple logoff/shutdown events to running a message loop that handles window messages. It's just a design consequence to consider whenever a console application or background process has to use COM or the shell API.

Footnotes

  1. This conversion is unrelated to whether the subsystem of the executable is flagged IMAGE_SUBSYSTEM_WINDOWS_GUI or IMAGE_SUBSYSTEM_WINDOWS_CUI (console). Using the GUI subsystem flag doesn't make the system automatically connect a process to a desktop, unlike how using the console subsystem flag makes the system automatically attach a process to a console.

  2. Logoff and shutdown events are sent from and initiated by the session server, not from a console. Background processes usually depend on these control events. For example, when "services.exe" is sent CTRL_SHUTDOWN_EVENT, it initiates shutdown of system services.

zooba and others added 2 commits September 1, 2022 21:04
Co-authored-by: Eryk Sun <eryksun@gmail.com>
@zooba
Copy link
Member Author

zooba commented Sep 1, 2022

There's no significant performance cost if calling sys.getwindowsversion() converts to a GUI process and GUI thread. It's certainly insignificant compared to the cost of WMI. However, a GUI process doesn't get sent CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT events.

I did remove the places I found that may trigger this on normal load, but the point is well made.

However, I don't think we could reliably handle CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT anyway? Certainly it's a flimsy enough system that anyone could break it in any number of ways, and it's so much more reliable to set up a message loop that people should do that. If it's possible to set up a signal handler for those events, maybe we could warn/fail if user32 is already loaded? Or maybe it makes more sense to always spin up the hidden window and emulate those events... but loading user32 is slow, and I'd hesitate to do it every time.

In any case, I think all that belongs in a new issue. At worst, we can always make _wmi into some kind of helper process, provided it doesn't turn into a full wmic clone that is trivially abusable.

@eryksun
Copy link
Contributor

eryksun commented Sep 1, 2022

However, I don't think we could reliably handle CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT anyway?

Currently Python's standard library cannot use CTRL_CLOSE_EVENT (i.e. the console window or terminal tab was closed), CTRL_LOGOFF_EVENT, or CTRL_SHUTDOWN_EVENT to support a graceful exit. The C runtime maps these events, along with CTRL_BREAK_EVENT, to the non-standard signal SIGBREAK. However, the C signal handler in Python just sets a flag and returns. To the system, the event was handled. The session server terminates a process if it's still running after a close event, logoff event, or shutdown event was handled. The default system settings allow a process 5 seconds to handle these events and exit gracefully. Maybe for SIGBREAK, the signal thread could simply sleep for 5 seconds if it's not a Python thread.

ctypes could potentially be used to call SetConsoleCtrlHandler() with a handler that implements a graceful exit for these events. However, currently importing the _ctypes extension module also converts to a GUI process. It's due to rarely used COM support, which I assume is almost never used without help from comtypes. _ctypes could delay load "ole32.dll", and the COM support would no longer be a problem. Delay loading would also help with _ssl and _hashlib, which normally don't use any GUI functions.

it's so much more reliable to set up a message loop that people should do that.

Compared to a control event, it's a lot harder to create a hidden window, run a message loop, and handle WM_QUERYENDSESSION and WM_ENDSESSION. However, it should be easy with a GUI framework. Qt connects WM_QUERYENDSESSION to commitDataRequest and WM_ENDSESSION to aboutToQuit. Tk connects WM_QUERYENDSESSION to WM_SAVE_YOURSELF, but it doesn't support WM_ENDSESSION in case ending the session was actually canceled.

@eryksun
Copy link
Contributor

eryksun commented Sep 2, 2022

If I substitute "onecore_apiset.lib" for "ole32.lib" in "PCbuild/pyproject.props", "_wmi.pyd" and "_ctypes.pyd" end up linking to "api-ms-win-core-com-l1-1-0.dll". This API set is implemented by "combase.dll", which doesn't immediately load "user32.dll". That said, "user32.dll" still gets loaded as soon as CoInitializeEx() is called, so that's no help. A child process could be used, but it's more complicated and expensive. I'd rather that sys.getwindowsversion() continued to use only the "kernel32.dll" file version as the value of platform_version. This attribute is rarely used. Plus it's already documented that the platform module should be used instead.

FWIW, linking with "onecore_apiset.lib" does solve the ctypes problem. I think a purely console or purely background script should at least be able to set a handler via SetConsoleCtrlHandler() that implements a graceful exit when the console session or desktop session is closed. (It would be even better if SIGBREAK in our C signal handler was fixed.) One just has to be conscientious to avoid indirectly loading "user32.dll". It's simple enough to check whether it's loaded when testing.

@vstinner
Copy link
Member

vstinner commented Sep 2, 2022

Note that the _wmi module depends on "ole32.dll" for COM support (i.e. CoInitializeEx, CoInitializeSecurity, CoCreateInstance, CoSetProxyBlanket, and CoUninitialize), which depends on "user32.dll". Loading the latter converts the current process to a GUI process1 and the current thread to a GUI thread.

If it's an issue, I suggest to call the _wmi module in a subprocess in the platform module. I don't think that the performance of platform module is critical and this module already spawns subprocesses to retrieve some data. We just have to make sure that the result is cached to only do that once ;-) I don't expect the Windows version to change while a process is running.

@eryksun
Copy link
Contributor

eryksun commented Sep 2, 2022

If it's an issue, I suggest to call the _wmi module in a subprocess in the platform module. I

Sorry if the point was lost in side discussions, but the main concern was the change to use _wmi in sys.getwindowsversion(). I'd prefer for this call to not load "user32.dll", which converts to a GUI process and thread.

@zooba
Copy link
Member Author

zooba commented Sep 2, 2022

I don't expect the Windows version to change while a process is running.

Let's hope not 😆

Sorry if the point was lost in side discussions, but the main concern was the change to use _wmi in sys.getwindowsversion(). I'd prefer for this call to not load "user32.dll", which converts to a GUI process and thread.

Did we ever document the platform_version field? We can probably just deprecate it, have it return the same as the other fields, and warn that it may not be correct for versions of Windows released after us. Then we can drop the code that inspects the kernel32 version as well.

@zooba
Copy link
Member Author

zooba commented Sep 2, 2022

It's documented, but it's documented as being unreliable. So I removed the WMI check in getwindowsversion and will just leave it as is (though I like the little restructure so I'm leaving that).

@eryksun
Copy link
Contributor

eryksun commented Sep 2, 2022

I suggest to call the _wmi module in a subprocess in the platform module.

If this is implemented, I suggest using multiprocessing. It implements set_executable() for the embedding case, freeze_support(), and special cases for WINENV (virtual environment), WINEXE (frozen), and WINSERVICE ("pythonservice.exe").

If importing _wmi gets fixed to not load "user32.dll" on import (e.g. delay-load "ole32.dll"), then an _isguiprocess() function could be implemented to return False if GetModuleHandleW(L"user32.dll") is NULL, else True. If it's a GUI process, the platform module can execute the query directly. Else use multiprocessing. For example:

   def _wmi_query(table, *keys):
        table = {
            "OS": "Win32_OperatingSystem",
            "CPU": "Win32_Processor",
        }[table]
        query = "SELECT {} FROM {}".format(",".join(keys), table)
        if _wmi._isguiprocess():
            data = _wmi.exec_query(query)
        else:
            import multiprocessing
            p = multiprocessing.Pool(1)
            try:
                data = p.apply(_wmi.exec_query, (query,))
            finally:
                p.close()
        split_data = (i.partition("=") for i in data.split("\0"))
        dict_data = {i[0]: i[2] for i in split_data}
        return (dict_data[k] for k in keys)

A more elaborate implementation would only close the process pool after it hasn't been used for a minute or so, like how COM implements the local server context.

@zooba
Copy link
Member Author

zooba commented Sep 5, 2022

If this is implemented, I suggest using multiprocessing.

We can recommend it if people run into trouble. But I don't see any reason to do it preemptively. This is far from the only thing likely to load user32, and it should only impact people who are already doing things that we don't provide for them. Using the platform module is optional enough, and users can do that in a subprocess if they need. If we find a need to do it in platform, then we can also add that later on.

@vstinner
Copy link
Member

vstinner commented Sep 6, 2022

The effect of loading user32.dll and converting the current thread to a GUI thread is not trivial. Would it make sense to document the change in the sys.getwindowsversion() documentation?

@zooba
Copy link
Member Author

zooba commented Sep 6, 2022

Would it make sense to document the change in the sys.getwindowsversion() documentation?

Not now that getwindowsversion() doesn't load it anymore 😉

It probably makes more sense to document it in signal, which is what someone would have to use to take a reliance on the feature that loading user32 might disable. But that was always a risk, so I consider it unrelated to this issue.

Co-authored-by: Eryk Sun <eryksun@gmail.com>
@eryksun
Copy link
Contributor

eryksun commented Sep 7, 2022

It probably makes more sense to document it in signal, which is what someone would have to use to take a reliance on the feature that loading user32 might disable.

Currently the signal module doesn't support CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT, or CTRL_SHUTDOWN_EVENT. Of the three, CTRL_CLOSE_EVENT is probably the most important to support since it gets sent whether or not "user32.dll" is loaded. The C runtime maps these events to SIGBREAK, but the C signal_handler() function just sets a flag and returns. Then the system terminates the process.

These three control events could be supported if the signal module had its own control handler. Given a SIGBREAK handler is set, the control handler would call trip_signal(SIGBREAK) and wait forever, but only for the close, logoff, and shutdown events for which the process must exit. Otherwise return FALSE to have the system call the next control handler, which is probably the C runtime's control handler.


This is all for naught, however, whenever "python[w].exe" gets executed by "py[w].exe" or "venv[w]launcher.exe". The behavior of the launchers is probably worth fixing, but it's not critical. They're primarily intended for development, not deployment of applications and services.

"py[w].exe" and "venvwlauncher.exe" load "user32.dll". Since they don't create a hidden window that handles WM_QUERYENDSESSION and WM_ENDSESSION, the system immediately terminates them at logoff and shutdown. Because of the job object, the child "python[w].exe" process also gets terminated abruptly.

Also, the control handler for the launchers returns TRUE for CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT, and CTRL_SHUTDOWN_EVENT. The latter two events are only seen by "venvlauncher.exe", since it doesn't load "user32.dll". On the other hand, the control event from closing the console window or terminal tab is always seen. Returning true for these control events means the process is ready to be terminated, and the system does so immediately. Because of the job object, the child "python.exe" process also gets terminated abruptly.

A simple way to handle these cases is to remove the child process from the job object when either the session is known to be ending or the console window/tab is closing. In these cases, both the launcher and Python either have to exit on their own or eventually get terminated, so coupling them with a job object no longer matters.

@zooba
Copy link
Member Author

zooba commented Sep 7, 2022

A simple way to handle these cases is to remove the child process from the job object when either the session is known to be ending or the console window/tab is closing.

Basically, you mean it's to handle the signals and pass them along correctly. That's likely true, but it's not really that simple. The job object is already relied upon for termination by PID, so we need to keep the job object.

In any case, definitely not part of this change. So I'll merge what is here and we can figure out termination edge cases in all our other mixed up stuff separately. (I'm honestly inclined to just always load user32 and run the loop to handle it, apart from the performance overhead that entails...)

@zooba zooba merged commit de33df2 into python:main Sep 7, 2022
@zooba zooba deleted the gh-89545 branch September 7, 2022 20:09
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows10 3.x has failed when building commit de33df2.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/146/builds/3535) and take a look at the build logs.
  4. Check if the failure is related to this commit (de33df2) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/146/builds/3535

Summary of the results of the build (if available):

Click to see traceback logs
remote: Enumerating objects: 34, done.        
remote: Counting objects:   3% (1/31)        
remote: Counting objects:   6% (2/31)        
remote: Counting objects:   9% (3/31)        
remote: Counting objects:  12% (4/31)        
remote: Counting objects:  16% (5/31)        
remote: Counting objects:  19% (6/31)        
remote: Counting objects:  22% (7/31)        
remote: Counting objects:  25% (8/31)        
remote: Counting objects:  29% (9/31)        
remote: Counting objects:  32% (10/31)        
remote: Counting objects:  35% (11/31)        
remote: Counting objects:  38% (12/31)        
remote: Counting objects:  41% (13/31)        
remote: Counting objects:  45% (14/31)        
remote: Counting objects:  48% (15/31)        
remote: Counting objects:  51% (16/31)        
remote: Counting objects:  54% (17/31)        
remote: Counting objects:  58% (18/31)        
remote: Counting objects:  61% (19/31)        
remote: Counting objects:  64% (20/31)        
remote: Counting objects:  67% (21/31)        
remote: Counting objects:  70% (22/31)        
remote: Counting objects:  74% (23/31)        
remote: Counting objects:  77% (24/31)        
remote: Counting objects:  80% (25/31)        
remote: Counting objects:  83% (26/31)        
remote: Counting objects:  87% (27/31)        
remote: Counting objects:  90% (28/31)        
remote: Counting objects:  93% (29/31)        
remote: Counting objects:  96% (30/31)        
remote: Counting objects: 100% (31/31)        
remote: Counting objects: 100% (31/31), done.        
remote: Compressing objects:   5% (1/19)        
remote: Compressing objects:  10% (2/19)        
remote: Compressing objects:  15% (3/19)        
remote: Compressing objects:  21% (4/19)        
remote: Compressing objects:  26% (5/19)        
remote: Compressing objects:  31% (6/19)        
remote: Compressing objects:  36% (7/19)        
remote: Compressing objects:  42% (8/19)        
remote: Compressing objects:  47% (9/19)        
remote: Compressing objects:  52% (10/19)        
remote: Compressing objects:  57% (11/19)        
remote: Compressing objects:  63% (12/19)        
remote: Compressing objects:  68% (13/19)        
remote: Compressing objects:  73% (14/19)        
remote: Compressing objects:  78% (15/19)        
remote: Compressing objects:  84% (16/19)        
remote: Compressing objects:  89% (17/19)        
remote: Compressing objects:  94% (18/19)        
remote: Compressing objects: 100% (19/19)        
remote: Compressing objects: 100% (19/19), done.        
remote: Total 34 (delta 12), reused 16 (delta 12), pack-reused 3        
From https://github.com/python/cpython
 * branch            main       -> FETCH_HEAD
Note: checking out 'de33df27aaf930be6a34027c530a651f0b4c91f5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at de33df2... gh-89545: Updates platform module to use new internal _wmi module on Windows to directly query OS properties (GH-96289)
Switched to and reset branch 'main'

Could Not Find D:\buildarea\3.x.bolen-windows10\build\Lib\*.pyc
The system cannot find the file specified.
Could Not Find D:\buildarea\3.x.bolen-windows10\build\PCbuild\python*.zip

Could Not Find D:\buildarea\3.x.bolen-windows10\build\Lib\*.pyc
The system cannot find the file specified.
Could Not Find D:\buildarea\3.x.bolen-windows10\build\PCbuild\python*.zip

@zooba
Copy link
Member Author

zooba commented Sep 7, 2022

I'm on the failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants