search

end18 / psutil

Automatically exported from code.google.com/p/psutil
Other
0 stars 0 forks source link

[OS X] getcmdargs() memory fragmentation #50

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Here are 7 issues with the psutil/arch/osx/process_info.c file:

line 134 -- val is unused.

line 135 -- val_start is unused.

line 167 -- pid is long while mib is int.

line 171 -- should the constant 22 not be EINVAL?

line 173 -- should -2 not be ARGS_ACCESS_DENIED?

line 242 -- np is never assigned any value.

line 254 -- ERROR_B falls thru to ERROR_C.

For your consideration, attached is a modified version of this file.

Original issue reported on code.google.com by MrJean1@gmail.com on 11 May 2009 at 8:28

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for the patch. This and the BSD process_info.c files will eventually 
need to
be properly cleaned up and have unused artifacts from the PSI code removed and 
the
code restructured. I'll check this in for now as r393 and hopefully I'll have a
chance to go through and really clean things up soon.

Thanks again for your input on this and the other issues you've identified. Code
review is always welcome, especially on the C code.

Original comment by jlo...@gmail.com on 12 May 2009 at 12:43

GoogleCodeExporter commented 9 years ago 
Attached is another version for the same psutil/arch/osx/process_info.c file 
with modifications in the GetBSDProcessList and getcmdargs functions.

The GetBSDProcessList functions is simpler and uses an increased buffer size to 
improve the chances for 
success on the second sysctl call.

Function getcmdargs uses the same double sysctl call method as 
GetBSDProcessList and only get the 
KERN_ARGMAX value as a last resort.  In addition, getcmdargs avoids fragmenting 
memory for smaller 
argument lists by using the space on the stack.

Original comment by MrJean1@gmail.com on 12 May 2009 at 5:21

Attachments:

GoogleCodeExporter commented 9 years ago 
Reopening.
In the meantime I ask you: every time you apply such modifications do you make 
sure
that both test_psutil.py and test_memory_leak.py test scripts run successfully?

Original comment by billiej...@gmail.com on 12 May 2009 at 5:28

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
getcmdargs() does not work correctly as defined in OSX2_process_info.c but I've
checked in the GetBSDProcessList() changes for now.

Original comment by jlo...@gmail.com on 12 May 2009 at 4:16

GoogleCodeExporter commented 9 years ago 
Both test cases work the same with the modified and original 0.1.2 code.  There 
is one failure, the same in both

ERROR: test_types (__main__.TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/test_psutil.py", line 279, in test_types
    self.assert_(isinstance(p.get_cpu_times(), tuple))
  File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/site-packages/psutil/_psutil.py", line 281, in get_cpu_times
    return _platform_impl.get_cpu_times(self.pid)
  File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/site-packages/psutil/_psosx.py", line 104, in get_cpu_times
    return _psutil_osx.get_process_cpu_times(pid)
AccessDenied: task_for_pid() failed for pid 348 with error 5

Original comment by MrJean1@gmail.com on 12 May 2009 at 4:19

GoogleCodeExporter commented 9 years ago 
The tests mentioned before were run only with Python 2.6.2 on MacOS X 10.4.11 
Intel.

Original comment by MrJean1@gmail.com on 12 May 2009 at 4:21

GoogleCodeExporter commented 9 years ago 
Test suite should be run as root/sudo otherwise task_for_pid() will fail and 
you'll
get the above errors. I'm running the same, 10.4.11 with python 2.6 so I'm not 
sure
why it would have worked for you and not for me but in any case I'm going 
through now
and cleaning up a little of the getcmdargs() code since I'm in here.

Original comment by jlo...@gmail.com on 12 May 2009 at 4:27

GoogleCodeExporter commented 9 years ago 
With sudo python test ... all tests pass with the modified files.  With Python 
2.5.2, the unmodified code also 
passes but the modified version shows two failures "SystemError: getcmdargs(): 
args parsing".  I'll look into 
those.

Original comment by MrJean1@gmail.com on 12 May 2009 at 4:45

GoogleCodeExporter commented 9 years ago 
I've checked in a new process_info.c in r397 with an updated getcmdargs(), so 
don't
worry about it. I will have to go back in later to add in support for 
environment
variables anyway. Thanks.

Original comment by jlo...@gmail.com on 12 May 2009 at 4:56

GoogleCodeExporter commented 9 years ago 
This is quite bizarre.  But getcmdarg works fine for all tests if the size of 
args[2048] is increased to 
args[2049].  Also, if args[1024] is used, the tests fail since argsize is 2048. 
 That must be a special  value.  
here is what the top of getcmdargs looks like now

    ....
    char     *ap, *cp, *ep, *sp, *procargs, args[2050];  /* avoid 2048 */

    /* Get the size of the process arguments. */
    mib[0] = CTL_KERN;
    mib[1] = KERN_PROCARGS2;
    mib[2] = (int)pid;

    argsize = ~(size_t)0;
    if (sysctl(mib, 3, NULL, &argsize, NULL, 0) == -1) {
        /* Try to get the maximum size. */
        mib[0] = CTL_KERN;
        mib[1] = KERN_ARGMAX;

        size = sizeof(argsize);
        if (sysctl(mib, 2, &argsize, &size, NULL, 0) == -1) {
            PyErr_SetFromErrno(NULL);
            return 1;
        }
    } else {
        argsize += 2;  /* some margin */
    }
    ....

Original comment by MrJean1@gmail.com on 12 May 2009 at 5:15

GoogleCodeExporter commented 9 years ago 
OK.  But keep in mind argmax is typically 256+K.  Each getcmdargs call could 
fragment memory by another 
256+K.   Hopefully, the MacOS X memory manager avoids that.

Original comment by MrJean1@gmail.com on 12 May 2009 at 5:28

GoogleCodeExporter commented 9 years ago 
Thanks, I'll convert this issue over to a more specific reminder to take a look 
at
the getcmdargs() memory profiling more in depth later.

Original comment by jlo...@gmail.com on 12 May 2009 at 8:12

GoogleCodeExporter commented 9 years ago 
One final observation.  It seems that there is often a problem if the size 
value returned by the last sysctl call is 
the same as the argsize value passed in.   Perhaps, the argsize value is indeed 
too small to hold the results, 
but sysctl is supposed to return -1 and ser errno to ENOMEM in that case.  It 
looks like that is not happening.

Therefore, it probably a good idea to slightly increase the size returned by a 
NULL sysctl call, anywhere.  Here 
is the top of my getcmdargs function:

    ....
    char     *ap, *cp, *ep, *sp, *procargs, args[2048];

    /* Get the size of the process arguments. */
    mib[0] = CTL_KERN;
    mib[1] = KERN_PROCARGS2;
    mib[2] = (int)pid;

    argsize = ~(size_t)0;
    if (sysctl(mib, 3, NULL, &argsize, NULL, 0) == -1) {
        /* Try to get the maximum size. */
        mib[0] = CTL_KERN;
        mib[1] = KERN_ARGMAX;

        size = sizeof(argsize);
        if (sysctl(mib, 2, &argsize, &size, NULL, 0) == -1) {
            PyErr_SetFromErrno(NULL);
            return 1;
        }
    } else {  /* add small margin */
        size = argsize + 2 + (argsize >> 4);
        if (argsize < size) {
            argsize = size;
        }
    }
    ...

That is similar to the GetBSDProcessList above it.

Original comment by MrJean1@gmail.com on 12 May 2009 at 9:26

GoogleCodeExporter commented 9 years ago

Original comment by jlo...@gmail.com on 17 Sep 2009 at 6:32

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
Updated csets after the SVN -> Mercurial migration:
r393 == revision 3afcd3d9c60f
r397 == revision 80a3b7473daa

Original comment by g.rodola on 2 Mar 2013 at 11:49

GoogleCodeExporter commented 9 years ago 
I guess this can be closed as outdated.

Original comment by g.rodola on 30 Apr 2014 at 12:50