1496050 - Nightly went into infinite process creation loop after launcher process is enabled

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masatoshi Kimura [:emk]]

I had to sign out to stop spawning firefox.exe processes. The current launcher process design is too danger without some safeguard IMO.

Also, I had to specify "ac_add_options --disable-launcher-process" to test my patch. Hopeflly this critical bug is fixed until the release...

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Fallback to GetFileInformationByHandle if GetFileInformationByHandleEx failed

Since some filesystems do not support FileIdInfo, we can't assume that it will
succeed even on Windows 8 or later.

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Make IsSameBinaryAsParentProcess more robust

We should not go into infinite process creation loop when we are unsure if
Firefox is the child of itself.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/831c1cae9dde
Fallback to GetFileInformationByHandle if GetFileInformationByHandleEx failed. r=mhowell

Comment 4

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 9014069 [details] [diff] [review]
Make IsSameBinaryAsParentProcess more robust

Review of attachment 9014069 [details] [diff] [review]:
-----------------------------------------------------------------

I don't feel great about this patch. Since the launcher process is largely a security feature, disabling it as an error handling strategy makes me a little uneasy. I think it's more likely that one of these functions would fail as a result of some action taken by an attacker in order to intentionally disable the launcher process than for any of them to fail for legitimate reasons (unless of course that's what you were seeing on your system; I'm making the assumption that the other patch addresses the problem you were running into).

But I do agree that trying again and therefore getting into an infinite loop isn't the right thing to do; you're right that these kinds of failures are not likely to be transient. I think I'd prefer to just abort and refuse to continue starting the browser at all. What do you think?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Yes, bypassing the launcher process completely defeats the purpose of having it in the first place.

Do we know what the GetLastError code was from GetFileInformationByHandleEx?

Comment 6

[Masatoshi Kimura [:emk]]

(In reply to Matt Howell [:mhowell] from comment #4)
> I don't feel great about this patch. Since the launcher process is largely a
> security feature, disabling it as an error handling strategy makes me a
> little uneasy. I think it's more likely that one of these functions would
> fail as a result of some action taken by an attacker in order to
> intentionally disable the launcher process than for any of them to fail for
> legitimate reasons (unless of course that's what you were seeing on your
> system; I'm making the assumption that the other patch addresses the problem
> you were running into).
> 
> But I do agree that trying again and therefore getting into an infinite loop
> isn't the right thing to do; you're right that these kinds of failures are
> not likely to be transient. I think I'd prefer to just abort and refuse to
> continue starting the browser at all. What do you think?

Makes sense. I'll change failure returns to MOZ_CRASH.

(In reply to (PTO Oct 2 - Oct 9) Aaron Klotz [:aklotz] from comment #5)
> Yes, bypassing the launcher process completely defeats the purpose of having
> it in the first place.
> 
> Do we know what the GetLastError code was from GetFileInformationByHandleEx?

The error was 87(ERROR_INVALID_PARAMETER) because FileIdInfo is not a valid function for exFAT (and probably FAT16/FAT32).

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Make IsSameBinaryAsParentProcess more robust

We should not go into infinite process creation loop when we are unsure if
Firefox is a child of itself.

Comment 8

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 9014220 [details] [diff] [review]
Make IsSameBinaryAsParentProcess more robust

Review of attachment 9014220 [details] [diff] [review]:
-----------------------------------------------------------------

This version of the patch generates a crash report instead of continuing on, which I think is fine. Thanks!

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/831c1cae9dde

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6af3691ec0d1
Make IsSameBinaryAsParentProcess more robust. r=mhowell

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6af3691ec0d1

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Matt Howell [:mhowell] from comment #8)
------------------------------------------------------------
> 
> This version of the patch generates a crash report instead of continuing on,
> which I think is fine. Thanks!

We won't get a breakpad crash report, we'll get a WER report, because our crash reporter is not initialized in the launcher process.

This is fine for now. We can follow up with more robust reporting in bug 1460433.