1797391 - GeckoChildProcessHost destructor uses process handle after closing it

Status: RESOLVED FIXED

(Core :: IPC, defect, P2)

Description

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

Found while testing bug 1797175: these operations are in the wrong order, because ProcessWatcher takes ownership of the handle and may close it immediately if the process has already exited. GetProcessId returns 0 for errors, so the DeregisterChildCrashAnnotationFileDescriptor call will silently fail and leak resources.

Comment 1

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

Attachment: Bug 1797391 - Avoid process handle use-after-close in GeckoChildProcessHost dtor.

ProcessWatcher takes ownership of the handle and may close it
immediately if the process has already exited, so that needs to
happen last; currently, because GetProcessId returns 0 for errors,
DeregisterChildCrashAnnotationFileDescriptor will be passed 0 in that
case, and will silently fail and leak resources.

Comment 2

[Gabriele Svelto [:gsvelto]]

Bug 1776197 should make the leak go away as we'll remove that handle.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/221e6fabc890
Avoid process handle use-after-close in GeckoChildProcessHost dtor. r=nika

Comment 4

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

Looks like this was introduced in bug 1407693.

Comment 5

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

Set release status flags based on info from the regressing bug 1407693

Comment 6

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/221e6fabc890

Comment 7

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

The patch landed in nightly and beta is affected.
:jld, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

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

This bug has been around for 5 years and nobody noticed, and as far as I can tell from the code the impact is just a small resource leak (i.e., not security sensitive), so I think this doesn't need uplift.