From 8bd2405f2bcc647dce62b46d61437d0abef01817 Mon Sep 17 00:00:00 2001 From: Giampaolo Rodola Date: Wed, 2 Aug 2023 11:02:12 +0200 Subject: [PATCH] [Linux] correctly raise ZombieProcess on exe(), cmdline() and memory_maps() (#2288) --- HISTORY.rst | 2 ++ Makefile | 2 +- psutil/_psbsd.py | 2 +- psutil/_pslinux.py | 63 +++++++++++++++++++++--------------- psutil/_psosx.py | 2 +- psutil/tests/__init__.py | 23 ++++++++++++- psutil/tests/test_linux.py | 48 ++++++++------------------- psutil/tests/test_process.py | 15 --------- 8 files changed, 77 insertions(+), 80 deletions(-) diff --git a/HISTORY.rst b/HISTORY.rst index bb14e6cfd..91c37bcb5 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -29,6 +29,8 @@ XXXX-XX-XX - 2284_, [Linux]: `memory_full_info`_ may incorrectly raise `ZombieProcess`_ if it's determined via ``/proc/pid/smaps_rollup``. Instead we now fallback on reading ``/proc/pid/smaps``. +- 2288_, [Linux]: correctly raise `ZombieProcess`_ on `exe`_, `cmdline`_ and + `memory_maps`_ instead of returning a "null" value. 5.9.5 ===== diff --git a/Makefile b/Makefile index 0143abe08..a8274a649 100644 --- a/Makefile +++ b/Makefile @@ -180,7 +180,7 @@ test-memleaks: ## Memory leak tests. ${MAKE} build $(TEST_PREFIX) $(PYTHON) $(TSCRIPT) $(ARGS) psutil/tests/test_memleaks.py -test-failed: ## Re-run tests which failed on last run +test-last-failed: ## Re-run tests which failed on last run ${MAKE} build $(TEST_PREFIX) $(PYTHON) $(TSCRIPT) $(ARGS) --last-failed diff --git a/psutil/_psbsd.py b/psutil/_psbsd.py index 37433c2db..fb4217efe 100644 --- a/psutil/_psbsd.py +++ b/psutil/_psbsd.py @@ -553,7 +553,7 @@ def is_zombie(pid): try: st = cext.proc_oneshot_info(pid)[kinfo_proc_map['status']] return PROC_STATUSES.get(st) == _common.STATUS_ZOMBIE - except Exception: + except OSError: return False diff --git a/psutil/_pslinux.py b/psutil/_pslinux.py index 7ca6abcfc..0f102cbfa 100644 --- a/psutil/_pslinux.py +++ b/psutil/_pslinux.py @@ -1654,12 +1654,12 @@ def wrapper(self, *args, **kwargs): except PermissionError: raise AccessDenied(self.pid, self._name) except ProcessLookupError: + self._raise_if_zombie() raise NoSuchProcess(self.pid, self._name) except FileNotFoundError: + self._raise_if_zombie() if not os.path.exists("%s/%s" % (self._procfs_path, self.pid)): raise NoSuchProcess(self.pid, self._name) - # Note: zombies will keep existing under /proc until they're - # gone so there's no way to distinguish them in here. raise return wrapper @@ -1675,7 +1675,27 @@ def __init__(self, pid): self._ppid = None self._procfs_path = get_procfs_path() - def _assert_alive(self): + def _is_zombie(self): + # Note: most of the times Linux is able to return info about the + # process even if it's a zombie, and /proc/{pid} will exist. + # There are some exceptions though, like exe(), cmdline() and + # memory_maps(). In these cases /proc/{pid}/{file} exists but + # it's empty. Instead of returning a "null" value we'll raise an + # exception. + try: + data = bcat("%s/%s/stat" % (self._procfs_path, self.pid)) + except (IOError, OSError): + return False + else: + rpar = data.rfind(b')') + status = data[rpar + 2:rpar + 3] + return status == b"Z" + + def _raise_if_zombie(self): + if self._is_zombie(): + raise ZombieProcess(self.pid, self._name, self._ppid) + + def _raise_if_not_alive(self): """Raise NSP if the process disappeared on us.""" # For those C function who do not raise NSP, possibly returning # incorrect or incomplete result. @@ -1749,22 +1769,18 @@ def name(self): # XXX - gets changed later and probably needs refactoring return name + @wrap_exceptions def exe(self): try: return readlink("%s/%s/exe" % (self._procfs_path, self.pid)) except (FileNotFoundError, ProcessLookupError): + self._raise_if_zombie() # no such file error; might be raised also if the # path actually exists for system processes with # low pids (about 0-20) if os.path.lexists("%s/%s" % (self._procfs_path, self.pid)): return "" - else: - if not pid_exists(self.pid): - raise NoSuchProcess(self.pid, self._name) - else: - raise ZombieProcess(self.pid, self._name, self._ppid) - except PermissionError: - raise AccessDenied(self.pid, self._name) + raise @wrap_exceptions def cmdline(self): @@ -1772,6 +1788,7 @@ def cmdline(self): data = f.read() if not data: # may happen in case of zombie process + self._raise_if_zombie() return [] # 'man proc' states that args are separated by null bytes '\0' # and last char is supposed to be a null byte. Nevertheless @@ -1990,8 +2007,10 @@ def get_blocks(lines, current_block): yield (current_block.pop(), data) data = self._read_smaps_file() - # Note: smaps file can be empty for certain processes. + # Note: smaps file can be empty for certain processes or for + # zombies. if not data: + self._raise_if_zombie() return [] lines = data.split(b'\n') ls = [] @@ -2030,14 +2049,7 @@ def get_blocks(lines, current_block): @wrap_exceptions def cwd(self): - try: - return readlink("%s/%s/cwd" % (self._procfs_path, self.pid)) - except (FileNotFoundError, ProcessLookupError): - # https://github.com/giampaolo/psutil/issues/986 - if not pid_exists(self.pid): - raise NoSuchProcess(self.pid, self._name) - else: - raise ZombieProcess(self.pid, self._name, self._ppid) + return readlink("%s/%s/cwd" % (self._procfs_path, self.pid)) @wrap_exceptions def num_ctx_switches(self, @@ -2086,7 +2098,7 @@ def threads(self): ntuple = _common.pthread(int(thread_id), utime, stime) retlist.append(ntuple) if hit_enoent: - self._assert_alive() + self._raise_if_not_alive() return retlist @wrap_exceptions @@ -2179,12 +2191,11 @@ def rlimit(self, resource_, limits=None): "got %s" % repr(limits)) prlimit(self.pid, resource_, limits) except OSError as err: - if err.errno == errno.ENOSYS and pid_exists(self.pid): + if err.errno == errno.ENOSYS: # I saw this happening on Travis: # https://travis-ci.org/giampaolo/psutil/jobs/51368273 - raise ZombieProcess(self.pid, self._name, self._ppid) - else: - raise + self._raise_if_zombie() + raise @wrap_exceptions def status(self): @@ -2239,13 +2250,13 @@ def open_files(self): path, int(fd), int(pos), mode, flags) retlist.append(ntuple) if hit_enoent: - self._assert_alive() + self._raise_if_not_alive() return retlist @wrap_exceptions def connections(self, kind='inet'): ret = _connections.retrieve(kind, self.pid) - self._assert_alive() + self._raise_if_not_alive() return ret @wrap_exceptions diff --git a/psutil/_psosx.py b/psutil/_psosx.py index 893bdba18..8da2d9a32 100644 --- a/psutil/_psosx.py +++ b/psutil/_psosx.py @@ -332,7 +332,7 @@ def is_zombie(pid): try: st = cext.proc_kinfo_oneshot(pid)[kinfo_proc_map['status']] return st == cext.SZOMB - except Exception: + except OSError: return False diff --git a/psutil/tests/__init__.py b/psutil/tests/__init__.py index fe9b2fcc7..1cb9fffde 100644 --- a/psutil/tests/__init__.py +++ b/psutil/tests/__init__.py @@ -964,17 +964,38 @@ def assertProcessZombie(self, proc): self.assertEqual(proc.status(), psutil.STATUS_ZOMBIE) # It should be considered 'running'. assert proc.is_running() + assert psutil.pid_exists(proc.pid) # as_dict() shouldn't crash. proc.as_dict() # It should show up in pids() and process_iter(). self.assertIn(proc.pid, psutil.pids()) self.assertIn(proc.pid, [x.pid for x in psutil.process_iter()]) - # It cannot be signaled or terminated. + psutil._pmap = {} + self.assertIn(proc.pid, [x.pid for x in psutil.process_iter()]) + # Call all methods. + ns = process_namespace(proc) + for fun, name in ns.iter(ns.all): + with self.subTest(name): + try: + fun() + except (psutil.ZombieProcess, psutil.AccessDenied): + pass + if LINUX: + # https://github.com/giampaolo/psutil/pull/2288 + self.assertRaises(psutil.ZombieProcess, proc.cmdline) + self.assertRaises(psutil.ZombieProcess, proc.exe) + self.assertRaises(psutil.ZombieProcess, proc.memory_maps) + # Zombie cannot be signaled or terminated. proc.suspend() proc.resume() proc.terminate() proc.kill() assert proc.is_running() + assert psutil.pid_exists(proc.pid) + self.assertIn(proc.pid, psutil.pids()) + self.assertIn(proc.pid, [x.pid for x in psutil.process_iter()]) + psutil._pmap = {} + self.assertIn(proc.pid, [x.pid for x in psutil.process_iter()]) # Its parent should 'see' it (edit: not true on BSD and MACOS). # descendants = [x.pid for x in psutil.Process().children( diff --git a/psutil/tests/test_linux.py b/psutil/tests/test_linux.py index ac11017a3..adb6ff606 100755 --- a/psutil/tests/test_linux.py +++ b/psutil/tests/test_linux.py @@ -2060,24 +2060,10 @@ def open_mock_2(name, *args, **kwargs): def test_exe_mocked(self): with mock.patch('psutil._pslinux.readlink', - side_effect=OSError(errno.ENOENT, "")) as m1: - with mock.patch('psutil.Process.cmdline', - side_effect=psutil.AccessDenied(0, "")) as m2: - # No such file error; might be raised also if /proc/pid/exe - # path actually exists for system processes with low pids - # (about 0-20). In this case psutil is supposed to return - # an empty string. - ret = psutil.Process().exe() - assert m1.called - assert m2.called - self.assertEqual(ret, "") - - # ...but if /proc/pid no longer exist we're supposed to treat - # it as an alias for zombie process - with mock.patch('psutil._pslinux.os.path.lexists', - return_value=False): - self.assertRaises( - psutil.ZombieProcess, psutil.Process().exe) + side_effect=OSError(errno.ENOENT, "")) as m: + ret = psutil.Process().exe() + assert m.called + self.assertEqual(ret, "") def test_issue_1014(self): # Emulates a case where smaps file does not exist. In this case @@ -2096,23 +2082,15 @@ def test_rlimit_zombie(self): # happen in case of zombie process: # https://travis-ci.org/giampaolo/psutil/jobs/51368273 with mock.patch("psutil._pslinux.prlimit", - side_effect=OSError(errno.ENOSYS, "")) as m: - p = psutil.Process() - p.name() - with self.assertRaises(psutil.ZombieProcess) as exc: - p.rlimit(psutil.RLIMIT_NOFILE) - assert m.called - self.assertEqual(exc.exception.pid, p.pid) - self.assertEqual(exc.exception.name, p.name()) - - def test_cwd_zombie(self): - with mock.patch("psutil._pslinux.os.readlink", - side_effect=OSError(errno.ENOENT, "")) as m: - p = psutil.Process() - p.name() - with self.assertRaises(psutil.ZombieProcess) as exc: - p.cwd() - assert m.called + side_effect=OSError(errno.ENOSYS, "")) as m1: + with mock.patch("psutil._pslinux.Process._is_zombie", + return_value=True) as m2: + p = psutil.Process() + p.name() + with self.assertRaises(psutil.ZombieProcess) as exc: + p.rlimit(psutil.RLIMIT_NOFILE) + assert m1.called + assert m2.called self.assertEqual(exc.exception.pid, p.pid) self.assertEqual(exc.exception.name, p.name()) diff --git a/psutil/tests/test_process.py b/psutil/tests/test_process.py index 46daa9a53..3ab05ba0c 100755 --- a/psutil/tests/test_process.py +++ b/psutil/tests/test_process.py @@ -1320,23 +1320,8 @@ def assert_raises_nsp(fun, fun_name): @unittest.skipIf(not POSIX, 'POSIX only') def test_zombie_process(self): - def succeed_or_zombie_p_exc(fun): - try: - return fun() - except (psutil.ZombieProcess, psutil.AccessDenied): - pass - parent, zombie = self.spawn_zombie() self.assertProcessZombie(zombie) - ns = process_namespace(zombie) - for fun, name in ns.iter(ns.all): - succeed_or_zombie_p_exc(fun) - - assert psutil.pid_exists(zombie.pid) - self.assertIn(zombie.pid, psutil.pids()) - self.assertIn(zombie.pid, [x.pid for x in psutil.process_iter()]) - psutil._pmap = {} - self.assertIn(zombie.pid, [x.pid for x in psutil.process_iter()]) @unittest.skipIf(not POSIX, 'POSIX only') def test_zombie_process_is_running_w_exc(self):