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

Migrate all ctypes code to use pywin32 instead, if possible #1808

Closed
wants to merge 21 commits into from

Conversation

Athanasius
Copy link
Contributor

@Athanasius Athanasius commented Jan 12, 2023

This started out as properly prototyping the ctypes code in monitor.py, so that nothing gets confused by return types on 64-bit python/windows...

... but after struggling for far too long with GetWindowTextW(), due to its second (out of three) parameter being 'out'... and realising that ctypes 'helpfully' returns all 'out' as the function return, and that means you need to define a function to assign to the .errcheck property in order to actually check result (return) ... I came across how to do the same thing with pywin32 modules instead.

And now I've managed to eliminate all the ctypes code from monitor.py. I'll look into all the other ctypes code as well.

This does mean we'll start having pywin32 as a PIP-level dependency ... and I am wondering if py2exe will do the right thing with all the separate little modules it provides. They are explicit imports in our code, so here's hoping.

Close #1805

It's using the win32/user32 GetWindowTextLengthW() so name it appropriately.
After over an hour's struggle trying to get the win32/user32
`GetWindowTextW()` prototyped properly, and running into problem after problem:

    1. Once you properly prototype this, including defining the 2nd `lpString`
      parameter as 'out', ctypes wants to just *return* that.
    2. This leads to then implementing a `.errcheck` property so that you
      can *also* check the INT result for errors.
    3. But then you just run into another error.

I finally found <https://stackoverflow.com/a/10905155> which pointed out
you can simply use `win32gui.GetWindowText()` from `pywin32`.  Tested, it
simply works, no errors/exceptions.
* All of the implementation of the EnumWindows callback.

The documentation for pywin32 isn't great, in terms of finding the call
convention and necessary form of parameters, but once you do this is *much*
cleaner than using ctypes directly.

I'm not 100% certain that `win32api.OpenProcess()` has the same semantic as
the previous `GetProcessHandleFromHwnd()`, i.e. only getting the `handle`
back if the process is owned by the current user.  If the user has
Windows Administrator rights they can probably kill other users' processes ?
@Athanasius Athanasius added code cleanup Cleaning up code python Pull requests that update Python code OS Issue labels Jan 12, 2023
@Athanasius
Copy link
Contributor Author

Athanasius commented Jan 12, 2023

NB: I'm not 100% sure that:

handle = win32api.OpenProcess(win32con.PROCESS_TERMINATE, False, process_id)

will definitely only return a handle for the current user's processes. It might do so for other users' processes if the current one is a Windows Administrator ?

There's an alternative method of getting process info using the wmi module which can tell you the owner of it, but in testing it is v -e- rrrrr-yyyyyy s-lllll-ooooooo- wwww. We end up calling EDLogs.game_running() about once a second because we poll the current journal file, rather than trying to do anything clever with a file watcher, so that's not at all viable.

@Athanasius
Copy link
Contributor Author

**NB: I'm not 100% sure that:

handle = win32api.OpenProcess(win32con.PROCESS_TERMINATE, False, process_id)

will definitely only return a handle for the current user's processes. It might do so for other users' processes if the current one is a Windows Administrator ?

Not that it matters, because it turns out the old code comment is wrong. Running develop I find that if I have a process with title Elite - Dangerous (Client) (small python program so I don't have to run the game) running as a different user via runas, the code still says "yup, game is running".

So, although we'd actually like to have the check be 100% correct, only being positive in the "game is running as this user" case... not doing so won't actually be a regression.

@Athanasius
Copy link
Contributor Author

I also note that the whole of the old code only finds the Elite - Dangerous* process/window if it's running in the visible session. So it'll find such that was runas'd, but not if you actually switch user, run the process, then switch back to the one running EDMarketConnector.

The same as also true of the new code in this PR.

Both the current `develop` code and this PR's new code share this behaviour.
What the code does is look for windows/processes *in the current Windows
session*, no matter if they're owned by the session user or not.

Neither version will find a process, that would match, if it's running in
another session, i.e. you'd need to switch to it via the windows Login screen.
@Athanasius
Copy link
Contributor Author

I double checked both old and new code through runas'ing steam and then the game (that alt is my Steam one), and indeed that process check does not actually determine if the game is running as the current user, only if it's in the current user session.

This is something the old code *didn't actually do*.

Tested with a 'pseudo ed' process (same MainWindowTitle) running as myself
(test matches), and via `runas` as another user (doesn't match).
At this time this errors, but that might be due to mixing ctypes and
pywin32 code.
Somehow I didn't run into this with testing *this* invocation of
`win32gui.EnumWindows()`, but then I did over in EDMarketConnector.py.  So,
as I had to handle the exception there, let's also do it here.
I currently can't find where pywin32 defines COINIT_APARTMENTTHREADED and
COINIT_DISABLE_OLE1DDE so am still having to hard-define those.
This was to be sure *that* bit of code was being hit and working without
issues.  It is.
* Also actually define the 'mapping' constants, with URL reference.
@Athanasius Athanasius modified the milestone: 5.9.0 Jan 16, 2023
@Athanasius Athanasius added this to the Future Release with major code changes milestone Jan 21, 2023
@Rixxan Rixxan removed this from the Future Release with major code changes milestone Aug 4, 2023
@Rixxan Rixxan marked this pull request as draft May 6, 2024 01:31
@Rixxan Rixxan closed this Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Cleaning up code OS Issue python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants