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

WindowsError: [Error 0] The operation completed successfully for exe() after upgrading to 5.5.0 #1394

Closed
jablonskim opened this issue Jan 24, 2019 · 14 comments

Comments

@jablonskim
Copy link

Tested with Python 2.7.13 and 3.7.1, Windows 10 1809 (10.0.17763.0).
psutil version: 5.5.0

It fails on 'Secure System' process, only when Python was run as an Administrator:

Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:53:40) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import psutil
>>> [p for p in psutil.process_iter()]
[psutil.Process(pid=0, name='System Idle Process', started='2018-11-06 04:18:12'), psutil.Process(pid=4, name='System', started='2018-11-06 04:18:12'), Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python27\lib\site-packages\psutil\__init__.py", line 395, in __str__
    info["name"] = self.name()
  File "C:\Python27\lib\site-packages\psutil\__init__.py", line 609, in name
    name = self._proc.name()
  File "C:\Python27\lib\site-packages\psutil\_pswindows.py", line 635, in wrapper
    return fun(self, *args, **kwargs)
  File "C:\Python27\lib\site-packages\psutil\_pswindows.py", line 687, in name
    return py2_strencode(os.path.basename(self.exe()))
  File "C:\Python27\lib\site-packages\psutil\_pswindows.py", line 635, in wrapper
    return fun(self, *args, **kwargs)
  File "C:\Python27\lib\site-packages\psutil\_pswindows.py", line 701, in exe
    return py2_strencode(convert_dos_path(cext.proc_exe(self.pid)))
WindowsError: [Error 0] The operation completed successfully
@giampaolo
Copy link
Owner

Mmm not good, and I can't reproduce it. Do you have VS? Could you re-compile psutil from sources and see where exactly the error originates from?

@jablonskim
Copy link
Author

Yes, sure. I will try to provide more details till end of the day or next Monday.

About reproducing it: just running it from Administrator account is not enough. Try starting cmd or powershell using right click and 'Run as an Administrator' and then run Python from it. Without running as an Administrator it runs perfectly fine.

@jablonskim
Copy link
Author

After some investigation, I realized that the issue is related to Virtual Secure Mode in Windows (http://woshub.com/virtual-secure-mode-vsm-in-windows-10-enterprise/). When I disabled Hyper-V using bcdedit /set hypervisorlaunchtype off, process_iter() works correctly because there is no 'Secure System' at all.

The problem is in this line: https://github.com/giampaolo/psutil/blob/master/psutil/_psutil_windows.c#L767
The OpenProcess function returns valid handle for 'Secure System' process, GetProcessImageFileNameW also ends with success, so the GetLastError() returns 0. Unfortunatelly, the name returned from GetProcessImageFileNameW is empty, so the return value, which is the length of the name, is zero. According to the documentation it should be zero only in case of failure.
I tried to change the GetProcessImageFileNameW to QueryFullProcessImageNameW (with PROCESS_NAME_NATIVE flag) which returns BOOL and it works fine then, except that the process names for some secure processes are empty.

So the partial output looks like this:

In [2]: [p for p in psutil.process_iter()]
Out[2]:
[psutil.Process(pid=0, name='System Idle Process', started='13:53:01'),
 psutil.Process(pid=4, name='System', started='13:53:01'),
 psutil.Process(pid=56, name='', started='13:53:01'),
 psutil.Process(pid=72, name='svchost.exe', started='13:53:11'),
 psutil.Process(pid=104, name='', started='13:53:01'),
 ...]

@jablonskim
Copy link
Author

jablonskim commented Jan 25, 2019

I additionally tried to raise AccessDenied if returned length is zero, and then it's working like for other privileged processes:

[psutil.Process(pid=0, name='System Idle Process', started='13:53:01'),
 psutil.Process(pid=4, name='System', started='13:53:01'),
 psutil.Process(pid=56, name='Secure System', started='13:53:01'),
 psutil.Process(pid=72, name='svchost.exe', started='13:53:11'),
 psutil.Process(pid=104, name='Registry', started='13:53:01'),
 ...]

@giampaolo
Copy link
Owner

giampaolo commented Jan 25, 2019

Thanks for investigating this. It looks like the culprit is QueryFullProcessImageNameW which returns 0 (failure). The subsequent call to GetLastError() returns 0 but it should have been ERROR_ACCESS_DENIED instead (so that is the proper fix).

giampaolo added a commit that referenced this issue Jan 25, 2019
…NIED; errno 0 occurs when the Python process runs in 'Virtual Secure Mode'
@giampaolo
Copy link
Owner

giampaolo commented Jan 25, 2019

OK, I should have fixed this in 6f4a622. To check everything's fine could you please run the following script in "Virtual Secure Mode"? I want to make sure there are no other APIs returning errcode 0 instead of ERROR_ACCESS_DENIED.

import psutil
for p in psutil.process_iter():
    p.as_dict()

@jablonskim
Copy link
Author

Yes, the as_dict() works.

For now, there is a non-empty name for 'Secure System' process, but (for me) there are 2 more 'secure' processes and the name is empty for them:

[psutil.Process(pid=0, name='System Idle Process', started='13:53:01'),
 psutil.Process(pid=4, name='System', started='13:53:01'),
 psutil.Process(pid=56, name='Secure System', started='13:53:01'),
 psutil.Process(pid=72, name='svchost.exe', started='13:53:11'),
 psutil.Process(pid=104, name='', started='13:53:01'),
 ...]

I will try to check the GetLastError() value for them too.

For the previous comment: The QueryFullProcessImageNameW actually returns True (success) and copies a string to the name parameter, but the string is empty. The GetProcessImageFileNameW (currently used in the code) returns the length, which is zero - docs says that the return value is the length, or zero in case of failure, but in our case the length is also zero - the same as for failure.
But yes, in this situation both functions should probably fail and the GetLastError should return ERROR_ACCESS_DENIED.

@jablonskim
Copy link
Author

jablonskim commented Jan 25, 2019

The GetProcessImageFileNameW and QueryFullProcessImageNameW are somehow broken in this case. They should return full path like \Device\HarddiskVolume2\Windows\System32\svchost.exe but for process with PID 104 it returns now just Registry string, so the convert_dos_path cannot parse it properly and returns empty string.

Additionally, I rebooted my PC and now the returned string for 'Registry' process is empty and everything works fine... But I tested it also on Windows Server and the returned string is Registry. So generally, there is no exception but sometimes the name for some processes is empty.

EDIT: This was actually not true. I accidentally run my test compilation for the second test. My mistake.

@jablonskim
Copy link
Author

Ok, to sum up:

  1. For PID 75, GetProcessImageFileNameW returns 52 (length), QueryFullProcessImageNameW returns TRUE. The returned name is for both '\Device\HarddiskVolume2\Windows\System32\svchost.exe'.
  2. For PID 4, GetProcessImageFileNameW returns 0 (length), QueryFullProcessImageNameW returns TRUE. The returned name is for both '' (empty string).
  3. For PID 104, GetProcessImageFileNameW returns 8 (length), QueryFullProcessImageNameW returns TRUE. The returned name is for both 'Registry'.

In my opinion, the solution is to change GetProcessImageFileNameW to QueryFullProcessImageNameW (so it is possible to check the success/failure return value), remove the check for if (GetLastError() == 0) and check if returned strings starts with device path form: \Device\. What do you think about this idea?

Another thing to check: QueryFullProcessImageNameW with flags set to 0 according to the docs should return 'Win32 path format' so there is no need to convert it back to Windows version. I do not have access to my setup now, I can check it on Monday.

@giampaolo
Copy link
Owner

Avoiding to convert the DOS/win32 path format would be a clear win. Before proceeding though I would investigate how both APIs behave and if there are differences in terms of returned paths and (above all) permissions. QueryFullProcessImageNameW may work better but we should first make sure it won’t result in more AccessDenied exceptions.

@giampaolo
Copy link
Owner

giampaolo commented Feb 15, 2019

Since this issue was critical I decided to release psutil 5.5.1 which includes 6f4a622 which fixes it. I'm leaving this open tough so that we can use QueryFullProcessImageNameW instead of GetProcessImageFileNameW.

@EccoTheFlintstone
Copy link
Contributor

EccoTheFlintstone commented Feb 17, 2019

Stepping in the discussion :
there's still an ongoing issue with this (as reported by jablonskim), where admin can't retrieve names from some "special" processes that don't have an exe on the filesystem(Registry, Memory Compression, ...):
on a windows 10 box, PID 88 is Registry

from an elevated prompt :

>>> psutil.Process(88)
psutil.Process(pid=88, name='', started='2019-02-10 02:06:11')

from a normal prompt (standard user)

>>> psutil.Process(88)
psutil.Process(pid=88, name='Registry', started='2019-02-10 02:06:11')

The "admin" prompt succeeds in getting the name with GetProcessImageFileNameW but it is troncated in convert_dos_path
The normal user fails with GetProcessImageFileNameW (cannot open process) and falls back on using CreateToolhelp32Snapshot which works

A quick fix would be to something like this (before switching to QueryFullProcessImageNameW but I haven't tested it yet either in that case):

@lru_cache(maxsize=512)
def convert_dos_path(s):
    r"""Convert paths using native DOS format like:
        "\Device\HarddiskVolume1\Windows\systemew\file.txt"
    into:
        "C:\Windows\systemew\file.txt"
    """
    if s.lower().startswith(r'\device\'):
        rawdrive = '\\'.join(s.split('\\')[:3])
        driveletter = cext.win32_QueryDosDevice(rawdrive)
        return os.path.join(driveletter, s[len(rawdrive):])
    else:
        return s

The switch to QueryFullProcessImageNameW should be the right move, but this patch would be much needed temporary bandaid (as, right now, some processes are reported without any name if retrieved from an admin account)

@giampaolo giampaolo changed the title process_iter() fails on Windows after upgrading to 5.5.0 WindowsError: [Error 0] The operation completed successfully for exe() after upgrading to 5.5.0 Feb 17, 2019
@giampaolo
Copy link
Owner

giampaolo commented Feb 17, 2019

Done: #1413. AFAICT, it seems the number of AccessDenied exceptions is the same with QueryFullProcessImageNameW (ex GetProcessImageFileNameW). Also the returned paths look the same.

Note worth mentioning: this will definitively break backward compatibility with Win XP (this API is Vista+ only) and I'm a bit "meh" about it (but not completely opposed). As per #1379 (comment) it seems there's still some Win XP users out there. It would be nice to update my PR and do a runtime check or use pre-processor directives in order to use different code paths on XP vs. others.

giampaolo added a commit that referenced this issue Feb 17, 2019
…1413)

#1394 / win / exe: use QueryFullProcessImageNameW to get the exe()
@giampaolo
Copy link
Owner

OK, I also included Win XP support (not tested though). Closing this out as fixed. Thank you guys for the support.

nlevitt added a commit to nlevitt/psutil that referenced this issue Apr 9, 2019
* origin/master: (182 commits)
  giampaolo#1394 / windows / process exe(): convert errno 0 into ERROR_ACCESS_DENIED; errno 0 occurs when the Python process runs in 'Virtual Secure Mode'
  pre-release
  fix win num_handles() test
  update readme
  fix giampaolo#1111: use a lock to make Process.oneshot() thread safe
  pdate HISTORY
  giampaolo#1373: different approach to oneshot() cache (pass Process instances around - which is faster)
  use PROCESS_QUERY_LIMITED_INFORMATION also for username()
  Linux: refactor _parse_stat_file() and return a dict instead of a list (+ maintainability)
  fix giampaolo#1357: do not expose Process' memory_maps() and io_counters() methods if not supported by the kernel
  giampaolo#1376 Windows: check if variable is NULL before free()ing it
  enforce lack of support for Win XP
  fix giampaolo#1370: improper usage of CloseHandle() may lead to override the original error code resulting in raising a wrong exception
  update HISTORY
  (Windows) use PROCESS_QUERY_LIMITED_INFORMATION access rights (giampaolo#1376)
  update HISTORY
  revert 5398c48; let's do it in a separate branch
  giampaolo#1111 make Process.oneshot() thread-safe
  sort HISTORY
  give CREDITS to @EccoTheFlintstone for giampaolo#1368
  fix ionice set not working on windows x64 due to LENGTH_MISMATCH  (giampaolo#1368)
  make flake8 happy
  give CREDITS to @amanusk for giampaolo#1369 / giampaolo#1352 and update doc
  Add CPU frequency support for FreeBSD (giampaolo#1369)
  giampaolo#1359: add test case for cpu_count(logical=False) against lscpu utility
  disable false positive mem test on travis + osx
  fix PEP8 style mistakes
  give credits to @koenkooi for giampaolo#1360
  Fix giampaolo#1354 [Linux] disk_io_counters() fails on Linux kernel 4.18+ (giampaolo#1360)
  giampaolo#1350: give credits to @amanusk
  FreeBSD adding temperature sensors (WIP) (giampaolo#1350)
  pre release
  sensors_temperatures() / linux: convert defaultdict to dict
  fix giampaolo#1004: Process.io_counters() may raise ValueError
  fix giampaolo#1307: [Linux] disk_partitions() does not honour PROCFS_PATH
  refactor hasattr() checks as global constants
  giampaolo#1197 / linux / cpu_freq(): parse /proc/cpuinfo in case /sys/devices/system/cpu fs is not available
  fix giampaolo#1277 / osx / virtual_memory: 'available' and 'used' memory were not calculated properly
  travis / osx: set py 3.6
  travis: disable pypy; se py 3.7 on osx
  skip test on PYPY + Travis
  fix travis
  fix giampaolo#715: do not print exception on import time in case cpu_times() fails.
  fix different travis failures
  give CREDITS for giampaolo#1320 to @truthbk
  [aix] improve compilation on AIX, better support for gcc/g++ + fix cpu metrics (giampaolo#1320)
  give credits to @alxchk for giampaolo#1346 (sunOS)
  Fix giampaolo#1346 (giampaolo#1347)
  giampaolo#1284, giampaolo#1345 - give credits to @amanusk
  Add parsing for /sys/class/thermal (giampaolo#1345)
  Fix decoding error in tests
  catch UnicodeEncodeError on print()
  use memory tolerance in occasionally failing test
  Fix random 0xC0000001 errors when querying for Connections (giampaolo#1335)
  Correct capitalization of PyPI (giampaolo#1337)
  giampaolo#1341: move open() utilities/wrappers in _common.py
  Refactored ps() function in test_posix (giampaolo#1341)
  fix giampaolo#1343: document Process.as_dict() attrs values
  giampaolo#1332 - update HISTORY
  make psutil_debug() aware of PSUTIL_DEBUG (giampaolo#1332)
  also include PYPY (or try to :P)
  travis: add python 3.7 build
  add download badge
  remove failing test assertions
  remove failing test
  make test more robust
  pre release
  pre release
  set version to 5.4.7
  OSX / SMC / sensors: revert giampaolo#1284 (giampaolo#1325)
  setup.py: add py 3.7
  fix giampaolo#1323: [Linux] sensors_temperatures() may fail with ValueError
  fix failing linux tests
  giampaolo#1321 add unit tests
  giampaolo#1321: refactoring
  make disk_io_counters more robust (giampaolo#1324)
  fix typo
  Fix DeprecationWarning: invalid escape sequence (giampaolo#1318)
  remove old test
  update is_storage_device() docstring
  fix giampaolo#1305 / disk_io_counters() / Linux: assume SECTOR_SIZE is a fixed 512
  giampaolo#1313 remove test which no longer makes sense
  disk_io_counters() - linux: mimic iostat behavior (giampaolo#1313)
  fix wrong reference link in doc
  disambiguate TESTFN for parallel testing
  fix giampaolo#1309: add STATUS_PARKED constant and fix STATUS_IDLE (both on linux)
  give CREDITS to @sylvainduchesne for giampaolo#1294
  retain GIL when querying connections table (giampaolo#1306)
  Update index.rst (giampaolo#1308)
  fix giampaolo#1279: catch and skip ENODEV in net_if_stat()
  appveyor: retire 3.5, add 3.7
  revert file renaming of macos files; get them back to 'osx' prefix
  winmake: add upload-wheels cmd
  Rename OSX to macOS (giampaolo#1298)
  apveyor: reset py 3.4 and remove 3.7 (not available yet)
  try to fix occasional children() failure on Win: https://ci.appveyor.com/project/giampaolo/psutil/build/job/je3qyldbb86ff66h
  appveyor: remove py 3.4 and add 3.7
  giampaolo#1284: give credits to @amanusk + some minor adjustments
  little refactoring
  Osx temps (giampaolo#1284)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants