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

gh-101607 causes regression in third-party psutil package on macOS #102493

Closed
ned-deily opened this issue Mar 7, 2023 · 9 comments · Fixed by #102502
Closed

gh-101607 causes regression in third-party psutil package on macOS #102493

ned-deily opened this issue Mar 7, 2023 · 9 comments · Fixed by #102502
Assignees
Labels
3.12 bugs and security fixes release-blocker type-bug An unexpected behavior, bug, or error

Comments

@ned-deily
Copy link
Member

ned-deily commented Mar 7, 2023

As part of the cPython release process for macOS installers, besides the standard cPython test suite, we run a small set of additional smoke tests. One of these involves an install from source of the psutil package which includes the building of a C extension module. In the run up to the 3.12.0a6 release, we found that our long-unchanged simple test was now failing, a regression from 3.12.0a5 and 3.11.x. The commit causing the failure was bisected to feec49c from GH-101607, a part of issue #101578. From a cursory glance at the traceback and the psutil code, it appears that psutil uses a decorator to translate certain OSError exceptions from its C module into other Python exceptions that its calling code is prepared to handle or ignore. For some reason, that exception translation is now failing. FWIW, this is the first failure of this psutil smoke test in recent memory including previous 3.12.0 alphas. Also, FWIW, this same smoke test does not fail on a reasonably recent Debian Linux system but, of course, that exercises a different platform-dependent code path in psutil.

Because of its dependencies, it is a bit complicated to build and reproduce without changes. Here is one way to do it; it is likely not optimal. This assumes a fairly recent macOS system with Homebrew installed (as described in the devguide) and assumes git checkouts of the cpython and psutil Github repos.

brew install pkg-config openssl@1.1 xz

mkdir /tmp/test_psutil
cd /tmp/test_psutil
git clone https://github.com/giampaolo/psutil.git
git clone https://github.com/python/cpython.git # or from an existing repo

cd cpython
git checkout feec49c40736fc05626a183a8d14c4ebbea5ae28^ # last good commit
git clean -fdxq
./configure -q  --prefix=/tmp/none --with-pydebug \
              --with-openssl="$(brew --prefix openssl@1.1)"
make -s -j2
cd ..
git -C ./psutil clean -fdxq
rm -rf venv
./cpython/python.exe -m venv venv
venv/bin/python -m pip install --no-binary psutil ./psutil/
venv/bin/python -c 'import psutil, pprint; pprint.pprint(list(psutil.process_iter()))'

The expected result should be something like:

[psutil.Process(pid=0, name='kernel_task', status='running', started='03:25:52'),
 psutil.Process(pid=1, name='launchd', status='running', started='03:25:52'),
 psutil.Process(pid=75, name='logd', status='running', started='03:25:54'),
 psutil.Process(pid=76, name='UserEventAgent', status='running', started='03:25:54'),
 ...

With the failing commit applied (up to and including the current head of the main branch):

git checkout feec49c40736fc05626a183a8d14c4ebbea5ae28 # failing commit

and repeating the above build and test, the result is now something like:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/private/tmp/cpython/Lib/pprint.py", line 55, in pprint
    printer.pprint(object)
  File "/private/tmp/cpython/Lib/pprint.py", line 153, in pprint
    self._format(object, self._stream, 0, 0, {}, 0)
  File "/private/tmp/cpython/Lib/pprint.py", line 175, in _format
    rep = self._repr(object, context, level)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/cpython/Lib/pprint.py", line 455, in _repr
    repr, readable, recursive = self.format(object, context.copy(),
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/cpython/Lib/pprint.py", line 468, in format
    return self._safe_repr(object, context, maxlevels, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/cpython/Lib/pprint.py", line 619, in _safe_repr
    orepr, oreadable, orecur = self.format(
                               ^^^^^^^^^^^^
  File "/private/tmp/cpython/Lib/pprint.py", line 468, in format
    return self._safe_repr(object, context, maxlevels, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/cpython/Lib/pprint.py", line 629, in _safe_repr
    rep = repr(object)
          ^^^^^^^^^^^^
  File "/private/tmp/venv/lib/python3.12/site-packages/psutil/__init__.py", line 389, in __str__
    info["name"] = self.name()
                   ^^^^^^^^^^^
  File "/private/tmp/venv/lib/python3.12/site-packages/psutil/__init__.py", line 628, in name
    cmdline = self.cmdline()
              ^^^^^^^^^^^^^^
  File "/private/tmp/venv/lib/python3.12/site-packages/psutil/__init__.py", line 681, in cmdline
    return self._proc.cmdline()
           ^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/venv/lib/python3.12/site-packages/psutil/_psosx.py", line 346, in wrapper
    return fun(self, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/private/tmp/venv/lib/python3.12/site-packages/psutil/_psosx.py", line 404, in cmdline
    return cext.proc_cmdline(self.pid)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 1] Operation not permitted (originated from sysctl(KERN_PROCARGS2))

Note this seems to come from trying to inspect a privileged process and the point of the exception wrapping is to effectively ignore such errors.

Flagging as a potential release-blocker for @Yhg1s

Linked PRs

@ned-deily ned-deily added type-bug An unexpected behavior, bug, or error release-blocker 3.12 bugs and security fixes labels Mar 7, 2023
@ned-deily
Copy link
Member Author

@giampaolo FYI

@giampaolo
Copy link
Contributor

Hi @ned-deily. That indeed looks like a psutil bug because EPERM should be translated into AccessDenied. What psutil version is this?

@ned-deily
Copy link
Member Author

What psutil version is this?

I see it with current HEAD of cpython main branch and either the most recent PyPI psutil release or current HEAD of the psutil GitHub repo.

@markshannon
Copy link
Member

Is the behavior changed by #101607 keeping the version of psutil constant?

@giampaolo
Copy link
Contributor

giampaolo commented Mar 7, 2023

In psutil, error originates from:
https://github.com/giampaolo/psutil/blob/bea3cf2d16899251b4b5f6b2609db9881645ea2d/psutil/arch/osx/process_info.c#L144
...which uses PyObject_CallFunction + PyErr_SetObject to generate an OSError(EPERM) exception in Python:
https://github.com/giampaolo/psutil/blob/bea3cf2d16899251b4b5f6b2609db9881645ea2d/psutil/_psutil_common.c#L90-L91
Were there changes to PyErr_SetObject recently?

@markshannon
Copy link
Member

Were there changes to PyErr_SetObject recently?

Yes. see #101578

@markshannon
Copy link
Member

The implementation of PyErr_SetObject changed. The behavior should have remained the same.

@iritkatriel
Copy link
Member

iritkatriel commented Mar 7, 2023

I think this is the fix:

diff --git a/Python/errors.c b/Python/errors.c
index f573bed3d6..e62c6e5e71 100644
--- a/Python/errors.c
+++ b/Python/errors.c
@@ -208,7 +208,7 @@ _PyErr_SetObject(PyThreadState *tstate, PyObject *exception, PyObject *value)
     }
     if (value != NULL && PyExceptionInstance_Check(value))
         tb = PyException_GetTraceback(value);
-    _PyErr_Restore(tstate, Py_XNewRef(exception), value, tb);
+    _PyErr_Restore(tstate, Py_NewRef(Py_TYPE(value)), value, tb);
 }

We're now normalising twice - once in SetObejct and then again in PyErr_Restore.

@iritkatriel
Copy link
Member

iritkatriel commented Mar 7, 2023

And I think we also need to check subclass rather than equality when normalising.

-    if (value == NULL || (PyObject *)Py_TYPE(value) != exception) {
+    int is_subclass = 0;
+    if (value != NULL) {
+        is_subclass = PyObject_IsSubclass((PyObject*)Py_TYPE(value), exception);
+        if (is_subclass < 0) {
+            return;
+        }
+    }
+    if (value == NULL || !is_subclass) {

Yhg1s pushed a commit that referenced this issue Mar 7, 2023
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
carljm added a commit to carljm/cpython that referenced this issue Mar 7, 2023
* main:
  pythongh-102493: fix normalization in PyErr_SetObject (python#102502)
  pythongh-87092: compiler's CFG construction moved to after codegen stage (python#102320)
  pythongh-95913: Consolidate build requirements changes in 3.11 WhatsNew (pythonGH-98781)
  Remove redundant `_ensure_future` in favor of `ensure_future` in `asyncio` (python#102398)
  pythongh-95913: Edit Faster CPython section in 3.11 WhatsNew (pythonGH-98429)
  pythongh-90110: Fix the c-analyzer Tool (python#102483)
  pythongh-101759: Update macOS installer SQLite 3.40.1 checksum (pythongh-102485)
  Remove unused import of `warnings` from `unittest.loader` (python#102479)
  Add gettext support to tools/extensions/c_annotations.py (python#101989)
carljm added a commit to carljm/cpython that referenced this issue Mar 8, 2023
* main:
  pythongh-102493: fix normalization in PyErr_SetObject (python#102502)
  pythongh-87092: compiler's CFG construction moved to after codegen stage (python#102320)
  pythongh-95913: Consolidate build requirements changes in 3.11 WhatsNew (pythonGH-98781)
  Remove redundant `_ensure_future` in favor of `ensure_future` in `asyncio` (python#102398)
  pythongh-95913: Edit Faster CPython section in 3.11 WhatsNew (pythonGH-98429)
  pythongh-90110: Fix the c-analyzer Tool (python#102483)
  pythongh-101759: Update macOS installer SQLite 3.40.1 checksum (pythongh-102485)
  Remove unused import of `warnings` from `unittest.loader` (python#102479)
  Add gettext support to tools/extensions/c_annotations.py (python#101989)
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Mar 11, 2023
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Mar 11, 2023
iritkatriel added a commit that referenced this issue Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes release-blocker type-bug An unexpected behavior, bug, or error
Projects
Development

Successfully merging a pull request may close this issue.

4 participants