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

[Linux] Properly raise ZombieProcess in exe() method #2062

Closed
wants to merge 1 commit into from

Conversation

Jongy
Copy link

@Jongy Jongy commented Jan 17, 2022

Summary

  • OS: Linux
  • Bug fix: yes
  • Type: core
  • Fixes: None

Description

psutil.Process(pid).exe() returns "" for zombie processes, incorrectly. It should raise ZombieProcess, and return "" only for kernel threads.

This PR fixes the logic (I wrote something quick, I suppose that the logic can be reordered more nicely maybe).

Now we get:

  • Alive processes - their exe
  • Non-existing PIDs - NoSuchProcess
  • Kernel threads - ""
  • Zombie processes - ZombieProcess.

Tested on kernel 5.4.

Also fixed & improved the comment along the way.

Signed-off-by: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>
@Jongy
Copy link
Author

Jongy commented Jan 17, 2022

I will look into the failing test later today

@Jongy
Copy link
Author

Jongy commented Jan 17, 2022

I looked into the test. The logic here is incorrect:

                # ...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)

if /proc/pid no longer exists, it's not a zombie. Zombies have their /proc/pid. It's a zombie if /exe link is missing & .status() is "zombie".

How should I fix the test? The lexists patch is relevant for NoSuchProcess, so I can change that, & add another mock for status() which makes it look like a zombie?

@giampaolo
Copy link
Owner

giampaolo commented Oct 18, 2022

psutil.Process(pid).exe() returns "" for zombie processes, incorrectly. It should raise ZombieProcess, and return "" only for kernel threads.

Mmm why are you saying so? Can you provide a test case?
We do have a unit test for zombie processes (and it passes on Linux):

def test_zombie_process(self):

Also, if there really is a problem with zombie detection, then we probably also have a problem with other Process methods, not only exe().

With that said, by looking at the Linux code I also suspect we're not detecting/handling zombie properly, because BSD and OSX implementations do something different, similar to what you're doing in this PR:

  • OSX:

    psutil/psutil/_psosx.py

    Lines 331 to 336 in 1da9f79

    def is_zombie(pid):
    try:
    st = cext.proc_kinfo_oneshot(pid)[kinfo_proc_map['status']]
    return st == cext.SZOMB
    except Exception:
    return False
  • BSD:

    psutil/psutil/_psbsd.py

    Lines 545 to 550 in 1da9f79

    def is_zombie(pid):
    try:
    st = cext.proc_oneshot_info(pid)[kinfo_proc_map['status']]
    return st == cext.SZOMB
    except Exception:
    return False

@giampaolo
Copy link
Owner

OK, I see what you mean now. exe() returns an empty string and test_zombie_process is too relaxed and does not test this condition (assumes "" is a legit return value).

@Jongy
Copy link
Author

Jongy commented Oct 18, 2022

OK, I see what you mean now. exe() returns an empty string and test_zombie_process is too relaxed and does not test this condition (assumes "" is a legit return value).

Precisely :)

I'm available to work on the PR if you want any changes to get this merged.

@Jongy
Copy link
Author

Jongy commented Oct 18, 2022

We're using a patched version to work around this bug:

def process_exe(process: psutil.Process) -> str:
    """
    psutil.Process(pid).exe() returns "" for zombie processes, incorrectly. It should raise ZombieProcess, and return ""
    only for kernel threads.

    See https://github.com/giampaolo/psutil/pull/2062
    """
    exe = process.exe()
    if exe == "":
        if is_process_zombie(process):
            raise psutil.ZombieProcess(process.pid)
        raise MissingExePath(process)
    return exe

I'd love to remove it and get back to mainline haha.

@giampaolo
Copy link
Owner

giampaolo commented Oct 19, 2022

I'm afraid this may affect other methods other than exe() and requires a more generic solution. I still didn't properly understood the whole picture though. Zombie processes are hard to deal with, especially on Linux apparently.

@giampaolo
Copy link
Owner

As I suspected, it turns out the problem not only affects process exe(). We have it also in cmdline() and memory_maps(). I created #2288 which fixes the problem in a more generic way, and also expands tests for zombie processes.

@Jongy Jongy deleted the fix-pslinux-exe branch August 2, 2023 10:36
@Jongy
Copy link
Author

Jongy commented Aug 2, 2023

As I suspected, it turns out the problem not only affects process exe(). We have it also in cmdline() and memory_maps(). I created #2288 which fixes the problem in a more generic way, and also expands tests for zombie processes.

That's great! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants