Skip to content

missing return in win32_kill? #58689

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

Closed
pitrou opened this issue Apr 3, 2012 · 23 comments
Closed

missing return in win32_kill? #58689

pitrou opened this issue Apr 3, 2012 · 23 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows type-bug An unexpected behavior, bug, or error

Comments

@pitrou
Copy link
Member

pitrou commented Apr 3, 2012

BPO 14484
Nosy @pitrou, @vstinner, @tjguk, @bitdancer, @asvetlov, @zware, @eryksun, @zooba, @iritkatriel

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2012-04-03.14:10:25.629>
labels = ['3.10', 'type-bug', '3.9', 'OS-windows', '3.11']
title = 'missing return in win32_kill?'
updated_at = <Date 2021-12-13.22:46:11.595>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2021-12-13.22:46:11.595>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Windows']
creation = <Date 2012-04-03.14:10:25.629>
creator = 'pitrou'
dependencies = []
files = []
hgrepos = []
issue_num = 14484
keywords = []
message_count = 20.0
messages = ['157419', '157421', '157422', '157424', '157426', '157428', '157429', '222918', '223684', '223739', '236320', '240809', '240815', '240819', '240824', '240912', '408328', '408337', '408454', '408487']
nosy_count = 10.0
nosy_names = ['jpe', 'pitrou', 'vstinner', 'tim.golden', 'r.david.murray', 'asvetlov', 'zach.ware', 'eryksun', 'steve.dower', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14484'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

@pitrou
Copy link
Member Author

pitrou commented Apr 3, 2012

Here is an excerpt from the os.kill implementation under Windows (in win32_kill(), posixmodule.c):

    if (sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT) {
        if (GenerateConsoleCtrlEvent(sig, pid) == 0) {
            err = GetLastError();
            PyErr_SetFromWindowsErr(err);
        }
        else
            Py_RETURN_NONE;
    }

It seems there is a missing return in the first branch, when GenerateConsoleCtrlEvent() fails.

@pitrou pitrou added OS-windows type-bug An unexpected behavior, bug, or error labels Apr 3, 2012
@asvetlov
Copy link
Contributor

asvetlov commented Apr 3, 2012

Antonie, you right.

@briancurtin
Copy link
Member

I can't find where we talked about this, maybe just IRC, but that's there (perhaps poorly) as a special case.

0 is 0, but signal.CTRL_C_EVENT is also 0. We try the signal version first then fall back to TerminateProcess.

I was just looking at this myself and it's not great. I actually wish we could change what signal.CTRL_C_EVENT means, or maybe add a flag to determine if the second parameter is supposed to be a return code or a signal? I'm open to anything.

@pitrou
Copy link
Member Author

pitrou commented Apr 3, 2012

I can't find where we talked about this, maybe just IRC, but that's
there (perhaps poorly) as a special case.

0 is 0, but signal.CTRL_C_EVENT is also 0. We try the signal version
first then fall back to TerminateProcess.

Then why set the error?
That said, abruptly killing another process and making it return 0
sounds a bit perverted.

@briancurtin
Copy link
Member

I don't remember exactly why, but it can be removed. It may have just been left in while I was debugging it.

As for the second point, why else are you calling os.kill if you don't want to kill the given process? I don't disagree that it's on the perverse side, but that's the functionality available. Perhaps we should *not* have the fallback and only operate on the signals?

@pitrou
Copy link
Member Author

pitrou commented Apr 3, 2012

As for the second point, why else are you calling os.kill if you don't
want to kill the given process? I don't disagree that it's on the
perverse side, but that's the functionality available. Perhaps we
should *not* have the fallback and only operate on the signals?

I just meant that exiting with 0 isn't exactly expected when a process
is forcibly killed (0 meaning success, as far as I know).

@bitdancer
Copy link
Member

I would think that if Windows doesn't support a specific signal, os.kill should raise a ValueError. But I'm an outsider here, I know nothing about how Windows works for this except what I'm reading here.

To answer your question: there are many reasons to call kill on unix, and only a few of them kill the process. Kill is just an historical name, it really means 'send a signal'.

In a broader picture, I think that os.kill calls should have the same "meaning", insofar as possible, on both linux and windows. Having a single API with the same syntax but different semantics on different platforms sounds bad to me.

@BreamoreBoy
Copy link
Mannequin

BreamoreBoy mannequin commented Jul 13, 2014

@zach can you add anything to this?

@zware
Copy link
Member

zware commented Jul 22, 2014

It looks like we have a bit of a mess here. 2.7 has a return there (and thus doesn't fall back to TerminateProcess if GenerateConsoleCtrlEvent fails), added 40 commits after the initial implementation in b1c00c7d3c85, but 3.x was never changed so 2.7 and 3.x have behaved differently from the the time it was implemented. Which version is right, or is it too late to change either one and 3.x should just remove the unused error setting? An interesting possibility might be to convert the signal.CTRL_* values to an enum, and use that as a way to distinguish between 0 and signal.CTRL_C_EVENT. I suspect that might become rather hairy, though.

Either way, I don't think os.kill can promise much more than "try to make the specified process die" on Windows; signals are just so crippled on Windows that that's about all you can do with them anyway. It might not hurt for the docs to try to make that clearer.

@vstinner
Copy link
Member

I understand that os.kill(pid, sig) should call TerminateProcess() or GenerateConsoleCtrlEvent() depending on the value of sig.

The problem is that these two functions are very different. A process can set a control handler for CTRL_C_EVENT and CTRL_BREAK_EVENT, so can decide how to handle GenerateConsoleCtrlEvent() event.

TerminateProcess() kills the process with the specified exit code.

To me it looks wrong to call TerminateProcess() with a signal number or event for the exit code!? We need to expose TerminateProcess() as a new Python function, os.TerminateProcess(pid, exitcode) for example.

os.kill(pid, sig) should raise a ValueError if sig is not CTRL_C_EVENT nor CTRL_BREAK_EVENT.

@BreamoreBoy
Copy link
Mannequin

BreamoreBoy mannequin commented Feb 20, 2015

bpo-14480 "os.kill on Windows should accept zero as signal" references this. It seems that we either go all the way and change the code as Victor has suggested or keep the status quo and change the docs as Zach has said. Thoughts?

@jpe
Copy link
Mannequin

jpe mannequin commented Apr 13, 2015

I think at a minimum, a return should be added in the cases that call GenerateConsoleCtrlEvent and it fails.

Here's a more radical proposal, though: deprecate kill() on Windows and add a function that calls GenerateConsoleCtrlEvent and another that calls TerminateProcess. The rationale is that the two do act quite a bit differently than kill does on non-Windows systems do and it's a bad idea to try to provide cross-platform functionality when it can't be done. kill() on non-Windows systems would be left alone.

@zware
Copy link
Member

zware commented Apr 14, 2015

I think what I'd rather do is deprecate the use of os.kill (on Windows) with a signal other than CTRL_C_EVENT and CTRL_BREAK_EVENT, document very clearly that os.kill is strictly for sending signals, and add os.terminate to implement TerminateProcess.

@jpe
Copy link
Mannequin

jpe mannequin commented Apr 14, 2015

A problem with os.kill with CTRL_C_EVENT is that CTRL_C_EVENT == 0 and on unix kill w/ a signal number of 0 is how you test to see if a process is running. This means that code written on unix to see if a process exists will try to send a ctrl-c to the other process; it will fail because GenerateConsoleCtrlEvent is so limited but the error message is likely to be confusing.

Not using the kill() name also means that developers coming from unix won't expect other signal numbers to work.

@zware
Copy link
Member

zware commented Apr 14, 2015

That's a fair point. So to make my plan work, we'd really need a good
way to differentiate CTRL_C_EVENT and 0. If anybody has any good
ideas on that, I still like my plan. If not, I'm afraid I have to
agree that os.kill will need to go down as doomed by Windows'
incompatibility with the rest of the world.

@jpe
Copy link
Mannequin

jpe mannequin commented Apr 14, 2015

I've created issue bpo-23948 for the idea of deprecating os.kill().

Is a patch needed for adding a return in the error case? It's that way in 2.7 and I'm struggling to come up with a reason why it shouldn't be added other than strict backward compatibility.

@iritkatriel
Copy link
Member

That piece of code is still there, the function is now called os_kill_impl.

@iritkatriel iritkatriel added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Dec 11, 2021
@eryksun
Copy link
Contributor

eryksun commented Dec 11, 2021

The details of os.kill() on Windows have been discussed extensively for years in various issues such as bpo-26350, bpo-23948, and bpo-42962. But the problem of the missing return statement is always overwhelmed by discussion of the egregiously bad design of this function and its misuse, which depends on bugs in WinAPI GenerateConsoleCtrlEvent() when passed a PID that is not a process group ID and/or not attached to the console.

@eryksun eryksun added 3.7 (EOL) end of life and removed 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Dec 11, 2021
@vstinner
Copy link
Member

IMO trying to mimic POSIX behavior on Windows in the single function os.kill() was a bad design idea. Windows should have its own specific function.

@zooba
Copy link
Member

zooba commented Dec 13, 2021

Windows should have its own specific function.

Either that or mimic it properly. Having a single function that requires
different parameters based on OS is a very user-hostile design.

If someone wants to shepherd it through the process, I'd support either
making the typical POSIX values do sensible equivalents on Windows (as
best possible), or adding a new function entirely and deprecating all
use of the current one on Windows. Those are the only ways to lead
people into writing code that works well on all platforms.

(My *real* preference is that people stop trying to kill applications ;)
and try to set up a safe communication channel instead, but I know it's
often unavoidable. Having common parameters that do the Right Things on
all platforms is next best.)

@iritkatriel iritkatriel added the 3.9 only security fixes label Dec 13, 2021
@iritkatriel iritkatriel added 3.10 only security fixes 3.11 only security fixes and removed 3.7 (EOL) end of life labels Dec 13, 2021
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
rkooo567 pushed a commit to ray-project/ray that referenced this issue Jan 18, 2023
The LogMonitor did not work correctly on windows.

It had two problems that this PR solves:

the code to add nice worker_pid names depended on posix path separators. This also caused the tests to fail on windows so they were skipped. Fixing this problem allowed enabling the tests
the way to check "liveness" of a PID used os.kill which would kill the process on windows, see os.kill on Windows should accept zero as signal python/cpython#58685 and missing return in win32_kill? python/cpython#58689. Since psutil is already installed, use it instead
In addition this PR has cleanups:

Avoid a warning when compiling regular expressions
Check for exceptions when using gputil, which raises on windows if drivers are installed but no graphics card is found (for instance, if the user disabled/removed the GPU).
andreapiso pushed a commit to andreapiso/ray that referenced this issue Jan 22, 2023
The LogMonitor did not work correctly on windows.

It had two problems that this PR solves:

the code to add nice worker_pid names depended on posix path separators. This also caused the tests to fail on windows so they were skipped. Fixing this problem allowed enabling the tests
the way to check "liveness" of a PID used os.kill which would kill the process on windows, see os.kill on Windows should accept zero as signal python/cpython#58685 and missing return in win32_kill? python/cpython#58689. Since psutil is already installed, use it instead
In addition this PR has cleanups:

Avoid a warning when compiling regular expressions
Check for exceptions when using gputil, which raises on windows if drivers are installed but no graphics card is found (for instance, if the user disabled/removed the GPU).

Signed-off-by: Andrea Pisoni <andreapiso@gmail.com>
@rruuaanng
Copy link
Contributor

Happy New Year, everyone! It's now 2025. Let's revisit the issue mentioned here. Should we start by adding a missing return? I have reviewed the current code, and it typically handles errors as follows:

    if (xxx == NULL) {
        err = GetLastError();
        return PyErr_SetFromWindowsErr(err);
    }

or

    if (xxx == 0) {
        err = GetLastError();
        return PyErr_SetFromWindowsErr(err);
    }

diff:

     if (sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT) {
         if (GenerateConsoleCtrlEvent(sig, (DWORD)pid) == 0) {
             err = GetLastError();
-            PyErr_SetFromWindowsErr(err);
+            return PyErr_SetFromWindowsErr(err);
         }
         else {
+            /* The generated signal has already been sent to the current console. */
             Py_RETURN_NONE;
         }
     }

@vstinner
Copy link
Member

Oh, this issue is still open in 2025? :-) I wrote a quick fix for it: PR gh-128932.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 17, 2025
)

(cherry picked from commit 939df0f)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jan 17, 2025
)

(cherry picked from commit 939df0f)

Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner
Copy link
Member

Windows os.kill() error handling was fixed by the change 939df0f.

I don't think that we can change os.kill() behavior on Windows. It's now too late :-( We should stick to the status quo. I close the issue.

Other issues (linked here) describes the behavior and how it could be changed.

vstinner added a commit that referenced this issue Jan 17, 2025
…128938)

gh-58689: Fix os.kill() error handling on Windows (GH-128932)
(cherry picked from commit 939df0f)

Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner added a commit that referenced this issue Jan 17, 2025
…128937)

gh-58689: Fix os.kill() error handling on Windows (GH-128932)
(cherry picked from commit 939df0f)

Co-authored-by: Victor Stinner <vstinner@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes OS-windows type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

10 participants