Dual process implementation for resolving AccessDenied #304

Closed
giampaolo opened this Issue May 23, 2014 · 6 comments

Comments

Projects
None yet
1 participant
@giampaolo
Owner

giampaolo commented May 23, 2014

From g.rodola on July 13, 2012 13:31:17

This is similar to issue 297 but for Windows.

On Windows we use native APIs to extract process information such as CPU times,
memory info etc.
Such APIs systematically fail with an AccessDenied exception for any process
owned by NT AUTHORITY SYSTEM user (system processes, typically) imposing a
severe usability issue for those who want to inspect processes not owned by the
current user (e.g. a task manager like app).
Example:

PID   NAME                 RSS MEM
0     System Idle Process  0
4     System               ACCESS DENIED
172   conhost.exe          3723264
248   svchost.exe          ACCESS DENIED
284   smss.exe             ACCESS DENIED
320   svchost.exe          ACCESS DENIED
328   TSVNCache.exe        7729152
376   csrss.exe            ACCESS DENIED
408   wininit.exe          ACCESS DENIED
424   csrss.exe            ACCESS DENIED
464   winlogon.exe         ACCESS DENIED
512   services.exe         ACCESS DENIED
520   lsass.exe            ACCESS DENIED
528   lsm.exe              ACCESS DENIED
564   python.exe           8675328
572   procexp64.exe        18870272
628   svchost.exe          ACCESS DENIED
688   VBoxService.exe      ACCESS DENIED
728   svchost.exe          ACCESS DENIED
796   explorer.exe         46014464
816   svchost.exe          ACCESS DENIED
856   svchost.exe          ACCESS DENIED
900   svchost.exe          ACCESS DENIED
968   svchost.exe          ACCESS DENIED
1064  dllhost.exe          5214208
1280  cmd.exe              6815744
1296  svchost.exe          ACCESS DENIED
1600  python.exe           5832704
1632  svchost.exe          ACCESS DENIED
1772  taskhost.exe         5554176
1840  dwm.exe              3375104
1852  TortoiseUDiff.exe    4263936
1952  VBoxTray.exe         3039232
2036  procexp.exe          5611520

I realized that a considerable amount of process information which we currently
extract by using documented Windows APIs is also stored in the
SYSTEM_PROCESS_INFORMATION structure, which can be obtained via
NtQuerySystemInformation():
http://undocumented.ntinternals.net/UserMode/Undocumented%20Functions/System%20Information/Structures/SYSTEM_PROCESS_INFORMATION.html
The peculiarity of NtQuerySystemInformation() is that it succeeds for
basically ALL processes except for PID 0!
On the other hand it is sensibly slower compared to using native APIs.
As such, a natural approach seems to be using the current, native APIs
implementation and fall back on using NtQuerySystemInformation in case of
permission error. Something like:

def get_process_cpu_times():
    try:
        return _psutil_mswindows.get_process_cpu_times()    # native API method
    except AccessDenied:
        return _psutil_mswindows.get_process_cpu_times_2()  # alternative method

Obviously, a robust set of unit tests will have to be written in order to make
sure that the two methods return the exact same value.
Follows a summary of Process methods which can apparently take benefit from
this approach:

 ---------------------------------------------------------------------------
| psutil method          | Win API              | PINFO struct              |
 ---------------------------------------------------------------------------
| create_time           | GetProcessTimes       | CreateTime                |
| nice/priority         | GetPriorityClass      | BasePriority              |
| get_cpu_times()       | GetProcessTimes       | UserTime, KernelTime      |
| get_cpu_percent()     | GetProcessTimes       | UserTime, KernelTime      |
| get_num_handles()     | GetProcessHandleCount | HandleCount               |
| get_memory_info()     | GetProcessMemoryInfo  | WorkingSetSize and others |
| get_ext_memory_info() | GetProcessMemoryInfo  | WorkingSetSize and others |
| get_memory_percent()  | GetProcessMemoryInfo  | WorkingSetSize and others |
| get_threads()         | GetThreadTimes        | Threads*                  |
| get_io_counters()     | GetProcessIoCounters  | *OperationCount           |
 ---------------------------------------------------------------------------

@wj32.64, given the massive amount of code rewriting involved, I'd like to hear
your opinion about this. Does it sounds reasonable and reliable to you?

Original issue: http://code.google.com/p/psutil/issues/detail?id=304

@giampaolo giampaolo self-assigned this May 23, 2014

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo May 23, 2014

Owner

From jlo...@gmail.com on July 13, 2012 05:43:55

The other way to handle this that comes to mind is similar to what we
originally did with the first incarnation of psutil; since multiple pieces of
information are retrieved with the same query, populate all those parts of the
process information structure simultaneously, and potentially also cache the
values when possible. Something like the deproxy lazy initialization we were
using in early versions of psutil.

I realize caching might not be feasible if we're looking at volatile
information like memory or CPU, so that would require some thinking. But if we
can fetch multiple pieces of data in one call to NtQuerySystemInformation()
then we're saving a lot of overhead in cases where someone is enumerating
multiple process properties.

My main question/issue with using NtQuerySystemInformation() so heavily is
reliability. Undocumented APIs can change at any time and may not be consistent
from release to release. I'd be concerned that we might run into
incompatibilities in structures across versions, but I don't know if that's the
case. If it's remained consistent across at least the last few Windows releases
then it's probably not a major concern.

Owner

giampaolo commented May 23, 2014

From jlo...@gmail.com on July 13, 2012 05:43:55

The other way to handle this that comes to mind is similar to what we
originally did with the first incarnation of psutil; since multiple pieces of
information are retrieved with the same query, populate all those parts of the
process information structure simultaneously, and potentially also cache the
values when possible. Something like the deproxy lazy initialization we were
using in early versions of psutil.

I realize caching might not be feasible if we're looking at volatile
information like memory or CPU, so that would require some thinking. But if we
can fetch multiple pieces of data in one call to NtQuerySystemInformation()
then we're saving a lot of overhead in cases where someone is enumerating
multiple process properties.

My main question/issue with using NtQuerySystemInformation() so heavily is
reliability. Undocumented APIs can change at any time and may not be consistent
from release to release. I'd be concerned that we might run into
incompatibilities in structures across versions, but I don't know if that's the
case. If it's remained consistent across at least the last few Windows releases
then it's probably not a major concern.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo May 23, 2014

Owner

From wj32...@gmail.com on July 13, 2012 05:53:52

I don't think you have to worry about NtQuerySystemInformation being
"undocumented". The chance of it changing is pretty low, since there are a lot
of developers who use it. To address your concerns about performance - you have
to choose between these two:

  • NtQuerySystemInformation - a bit slower and requires memory allocation, but
    bypasses permissions
  • Normal APIs (which all use NtQueryInformationProcess) - a bit faster, but
    requires a handle to the process

I like jloden's idea of caching properties for multiple processes, but of
course you have to decide when to update your cached data. That could be a bit
of a problem.

Owner

giampaolo commented May 23, 2014

From wj32...@gmail.com on July 13, 2012 05:53:52

I don't think you have to worry about NtQuerySystemInformation being
"undocumented". The chance of it changing is pretty low, since there are a lot
of developers who use it. To address your concerns about performance - you have
to choose between these two:

  • NtQuerySystemInformation - a bit slower and requires memory allocation, but
    bypasses permissions
  • Normal APIs (which all use NtQueryInformationProcess) - a bit faster, but
    requires a handle to the process

I like jloden's idea of caching properties for multiple processes, but of
course you have to decide when to update your cached data. That could be a bit
of a problem.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo May 23, 2014

Owner

From g.rodola on July 13, 2012 07:26:24

Since we're talking about volatile info I can't see any consistent way to cache
it except introducing a brand new CachedProcess class providing a refresh()
method. Please note that we now have an as_dict() method though, which can be
used to implement caching pretty flexibly by hand, as in:

>>> p = psutil.Process(pid)
>>> p._info = p.as_dict()   # save all current process info
>>> p._info['cpu_percent']  # access cached info

...later on:

>>> p._info = p.as_dict()   # update() cache

Also, note that I've already introduced caching where possible in latest
release (ppid, name, exe, cmdline and create_time, process_iter() - issue 281
and issue 301 ).

As for populating the struct simultaneously please note that using
NtQuerySystemInformation() is a lot slower than using documented APIs (about
-6x) so I'm not sure how much grouping would help.

Owner

giampaolo commented May 23, 2014

From g.rodola on July 13, 2012 07:26:24

Since we're talking about volatile info I can't see any consistent way to cache
it except introducing a brand new CachedProcess class providing a refresh()
method. Please note that we now have an as_dict() method though, which can be
used to implement caching pretty flexibly by hand, as in:

>>> p = psutil.Process(pid)
>>> p._info = p.as_dict()   # save all current process info
>>> p._info['cpu_percent']  # access cached info

...later on:

>>> p._info = p.as_dict()   # update() cache

Also, note that I've already introduced caching where possible in latest
release (ppid, name, exe, cmdline and create_time, process_iter() - issue 281
and issue 301 ).

As for populating the struct simultaneously please note that using
NtQuerySystemInformation() is a lot slower than using documented APIs (about
-6x) so I'm not sure how much grouping would help.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo May 23, 2014

Owner

From g.rodola on July 13, 2012 13:25:20

Ok, this is now fixed.
The Process methods affected by this change are:

  • create_time r1449 - get_cpu_times() r1448 - get_cpu_percent() r1448 -
    get_memory_info() r1452 - get_memory_percent() r1452 - get_num_handles() r1450
  • get_io_counters() r1451 Note that we're now able to determine meaningful info
    even for PID 4, which return value was historically hard-coded in the python layer.

Status: FixedInSVN

Owner

giampaolo commented May 23, 2014

From g.rodola on July 13, 2012 13:25:20

Ok, this is now fixed.
The Process methods affected by this change are:

  • create_time r1449 - get_cpu_times() r1448 - get_cpu_percent() r1448 -
    get_memory_info() r1452 - get_memory_percent() r1452 - get_num_handles() r1450
  • get_io_counters() r1451 Note that we're now able to determine meaningful info
    even for PID 4, which return value was historically hard-coded in the python layer.

Status: FixedInSVN

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo May 23, 2014

Owner

From g.rodola on August 13, 2012 09:25:13

Fixed in version 0.6.0, released just now.

Status: Fixed
Labels: -Milestone-1.0.0 Milestone-0.6.0

Owner

giampaolo commented May 23, 2014

From g.rodola on August 13, 2012 09:25:13

Fixed in version 0.6.0, released just now.

Status: Fixed
Labels: -Milestone-1.0.0 Milestone-0.6.0

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo May 23, 2014

Owner

From g.rodola on March 02, 2013 04:11:03

Updated csets after the SVN -> Mercurial migration: r1448 == revision
5c5b03f3643f r1449 == revision 2dcc29d215a7 r1450 == revision 6960659dac04 r1451 == revision 48a6b054f238 r1452 == revision 85677a2caf85

Owner

giampaolo commented May 23, 2014

From g.rodola on March 02, 2013 04:11:03

Updated csets after the SVN -> Mercurial migration: r1448 == revision
5c5b03f3643f r1449 == revision 2dcc29d215a7 r1450 == revision 6960659dac04 r1451 == revision 48a6b054f238 r1452 == revision 85677a2caf85

@giampaolo giampaolo closed this May 23, 2014

@giampaolo giampaolo changed the title from Resolve AccessDenied exception problems occurring most of the times on Windows to Dual process implementation for resolving AccessDenied Nov 5, 2016

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