32/64 bit confusion on Windows in Process.cmdline and Process.cwd

[fbenkstein]

While considering to restructure my Process.environ implementation I came across the following problem: In Process.cmdline and Process.cwd ReadProcessMemory is called to get at the RTL_USER_PROCESS_PARAMETERS structure that has the necessary information. The pointer to the environment variables is also located at the end of this structure. There are five combinations that have to be considered: the Python process can be a native 32bit (on a 32bit host OS), a WoW64 32bit process process or 64bit process and the target process can be 32bit or 64bit.

	Process 32 bit	Process 64 bit
Python 32 bit (native)	works	impossible
Python 32 bit (WoW64)	works	broken
Python 64 bit	broken	works

Currently only the straight cases 32->32 and 64->64 seem to work correctly. I am currently looking into fixing this but it doesn't seem straightforward and will make the current code even more complex. Please consider dropping support for these function at least on Wow64. This would leave three relatively straightforward cases. In the long run 32 bit native should probably dropped as well.
0001-test-ReadProcessMemory-functions.txt

[giampaolo]

So what you're saying is that we have issues, for example, if we use a Python 32 installation on a Windows 64 OS? What happens in that case? How is that broken?
This is definitively not my area of expertise but FWICT all the .exe and .wheels I distribute (both 32 and 64 bits) are built on a Windows 7 64-bit OS which I virtualize with VirtualBox so that probably means I am not understanding the issue at hand.
That aside, how does this impact on your environ implementation for Windows? Assuming there is a problem with cwd() and cmdline() can you perhaps ignore it for now and deal with it later as a separate PR?

[fbenkstein]

At the moment if the bitness does not match the addresses given to ReadProcessMemory are incorrect. This results in an invalid value read in one case (64 bit Python, 32 bit process, cwd is incorrect - see attached test case above) and ERROR_PARTIAL_READ which gets translated to AccessDenied in others. To contain the ugliness somewhat I still want to go ahead with merging all callers of ReadProcessMemory (cmdline()/cwd()/environ()) into a single function in my implementation of Process.environ for Windows. I'm hope that if I do that in smaller commits it is easier to review and can be merged. As you suggest, I'll try to ignore the mismatched bitness problems for now and hopefully address them later.

[giampaolo]

My main concern about your previous PR (other than being difficult to review) was that calculating 3 different things at the same time is slower (especially because environ requires constructing a big Python dict). You should calculate one at a time and put the shared code in a separate utility function (say it's called psutil_read_process_memory) similar to psutil_handle_for_pid and psutil_get_peb_address: https://github.com/giampaolo/psutil/blob/master/psutil/arch/windows/process_info.c.
Perhaps it might make sense to:

1 - refactor cwd() / cmdline() code so that they rely on a shared/utility function using ReadProcessMemory
2 - make a PR (optional)
3 - fix the ReadProcessMemory issue you're talking about (ERROR_PARTIAL_READ, erroneous AccessDenied exception etc.)
4 - make a PR
5 - implement environ and make a PR

Thoughts?

[fbenkstein]

1./2. Done -> #738

I'll try my hand at 3. now.

[giampaolo]

BTW, https://github.com/giampaolo/psutil/files/103277/0001-test-ReadProcessMemory-functions.txt is a precious set of test cases which should definitively be integrated once this is fixed.

[fbenkstein]

Yes, I'm integrating these tests in my work for 3.

[fbenkstein]

Can be closed.