1662560 - VirtualProtectEx for a sandboxed process fails with ERROR_INVALID_ADDRESS

Status: RESOLVED FIXED

(Firefox :: Launcher Process, defect, P3)

Description

[Toshihito Kikuchi [:toshi]]

This is one of the top failures to launch a sandboxed process.

This can happen even when the launcher process is disabled, ending up with the no-content-process situation. So the severity may be a bit higher.

Comment 1

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1662560 - Retrieve the imagebase of the child process's executable from a process handle. r=mhowell

Our data indicates when the browser process populates a newly-created child process,
VirtualProtectEx may fail with ERROR_INVALID_ADDRESS for some unknown reason.

One possible cause is the parameter aRemoteExeImage of RestoreImportDirectory
was wrong i.e. pointing to an invalid address. We simply pass the local process's
imagebase as aRemoteExeImage based on the assumption that the same executable is
mapped onto the same address in a different process, but it may not be guaranteed.

To deal with that potential case, we could retrieve a correct imagebase from the handle
of a remote process as we do for the plugin process. Since we're not so sure about
the root cause or the effectiveness of this fix, we run it only when the first
attempt to VirtualProtectEx failed in Nightly. Once it's confirmed, we promote this
to a permanent fix.

Comment 2

[Molly Howell (she/her) (no longer active)]

I wonder if at least some of these failures are another symptom of the situation that's being encountered in (for example) bug 1120863, where we have one Firefox instance that starts up and applies an update while another instance was still running. If the preexisting instance tries to start a new content process after that happens, it would be using the updated image, so the old image which is still running in the parent wouldn't be mapped there at all. This also means the new process is not actually going to be usable later on, which is what the other bugs about that problem are seeing the effects of.

We do have a few efforts under way (in various stages of completion) which attempt to mitigate that problem, but I don't think we'll be able to eradicate it completely, so defensive efforts like this patch will still be warranted.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea452bb92e6a
Retrieve the imagebase of the child process's executable from a process handle.  r=mhowell

Comment 4

[Dorel Luca [:dluca]]

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

Comment 5

[Toshihito Kikuchi [:toshi]]

Attachment: 01-tab-launcher-failures-without-fix.png

Comment 6

[Toshihito Kikuchi [:toshi]]

Attachment: 02-tab-launcher-failures-with-fix.png

Our latest data shows no failures at VirtualProtectEx with this fix. This means relying on the assumption that the same executable will be mapped onto the same address is risky. A symptom could be worse if we modified data at an address in a child process that is valid but not the imagebase.

Comment 7

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1662560 - Always retrieve the imagebase of the child process's executable from a process handle. r=mhowell

The earlier fix ea452bb92e6a proved the executable's imagebase in a child
process is not always the same as the local imagebase. This patch applies
the new approach to retieve the imagebase from a handle to all channels.

Interestingly, we observed the launcher failures at VirtualProtectEx only
when launching a sandboxed process, not when launching the browser process.
In the long term, we may need to take care of all WriteProcessMemory calls
for a child process for greater safety, but given that observation, this
patch only updates RestoreImportDirectory for now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93bb035ef2c9
Always retrieve the imagebase of the child process's executable from a process handle. r=mhowell

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/93bb035ef2c9