1568291 - Check for and deny special process identifiers in base::KillProcess

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This is part of bug 1314711 and bug 1564976: it's too easy to accidentally call base::KillProcess with a process handle that's actually an in-band null, like:

• Unix pid 0, which indicates the caller's process group (i.e., kill the whole browser)
• Windows INVALID_HANDLE_VALUE, which is not an invalid handle value and in fact is the same value returned by GetCurrentProcess()
• Unix pid -1, which would kill every other process owned by the user
• Windows nullptr / NULL / 0, which is an invalid handle value and should just fail

So we should check for that and not do it. We should also reconsider how we handle nullable process identifiers, but that's a larger project.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #1)

Bugbug thinks this bug is a task, but please change it back in case of error.

I already thought about the bug type. This is a fix or at least workaround (depending on how you look at it) for a browser crash — or, potentially, killing all the user's running apps and causing an OS-level logout — so I'm treating it as a defect.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1568291 - Be more defensive in base::KillProcess.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5333943843b
Be more defensive in base::KillProcess. r=froydnj

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a5333943843b