Set MaxLoaderThreads in registry for arm64 Windows install.

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P1
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla67
ARM64
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66- wontfix, firefox67 fixed)

Details

(Whiteboard: sb+)

Attachments

(1 attachment)

On arm64 Windows installs we need to set the following registry entries for the sandbox to work properly:
[HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution Options\firefox.exe]
"MaxLoaderThreads"=dword:00000001
[HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution Options\plugin-container.exe]
"MaxLoaderThreads"=dword:00000001

This is required because the multi-threaded DLL loading doesn't work with the chromium sandbox when the access token level is strong enough to prevent the reading of DLLs at start-up on anything other than the main thread.

I'm doing this in a separate bug to bug 1515088, because this doesn't actually fix things for the case where you've just unzipped a build somewhere under your Users folder.
It also doesn't fix it when you install without Administrator permissions, where we install into AppData\Local. This is because we don't have permission to write into HKEY_LOCAL_MACHINE and creating an equivalent entry in HKEY_CURRENT_USER doesn't seem to work for Image File Execution Options.

(See bug 1515088 and bug 1515826 for more details)

See Also: → 1515826
This Image File Execution Option is required for firefox.exe and
plugin-container.exe because the multi-threaded loading interferes
with the chromium sandbox.
Attachment #9035962 - Flags: review?(mhowell)
Comment on attachment 9035962 [details] [diff] [review]
Set MaxLoaderThreads to 1 in the registry for arm64 Windows installs

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

I'd have concerns about this as a permanent solution, but it's fine as a stopgap.
Attachment #9035962 - Flags: review?(mhowell) → review+

(In reply to Matt Howell (he/him) [:mhowell] from comment #3)

I'd have concerns about this as a permanent solution, but it's fine as a
stopgap.

Presumably the sandbox can hook registry functions, right? Could it be possible to force the sandbox to emit a fake MaxLoaderThreads value when it it is queried?

(In reply to Matt Howell (he/him) [:mhowell] from comment #3)
...

I'd have concerns about this as a permanent solution, but it's fine as a
stopgap.

Thanks.
Out of interest, is it the use of the registry keys to achieve the single threaded loading or the actual setting of these keys through the installer that you are concerned about?

(In reply to Aaron Klotz [:aklotz] from comment #4)

(In reply to Matt Howell (he/him) [:mhowell] from comment #3)

I'd have concerns about this as a permanent solution, but it's fine as a
stopgap.

Presumably the sandbox can hook registry functions, right? Could it be possible to force the sandbox to emit a fake MaxLoaderThreads value when it it is queried?

I thought maybe this wouldn't work (as in we wouldn't see the look-up user side), but a quick check on x64 seems to suggest it would, thanks!
In fact the sandbox already hooks NtOpenKey (and NtCreateKey) for brokering access.
Looks like we'd need to hook NtQueryValueKey as well, but that doesn't seem like a big problem.
I imagine it would make sense to keep this in the sandbox code as it relates to that and it already hooks NtOpenKey.

I think I'd normally slightly prefer using the official solution of setting the registry, but given that doesn't work in all cases, I think this may well be the way to go.

I'll check on arm64.

My reservations are about the use of the registry key at all; if we're going to do that, doing it through the installer is pretty much the only way, so that's alright.

My preference is for the registry function hook solution though. It would work for ZIP builds and non-admin installs and it doesn't leave anything permanent that might complicate uninstall. I'm also a little worried that overzealous security software (which is the only kind of security software) might take issue with us writing to the Image File Execution Options key.

Updated

5 months ago
Priority: -- → P1
Whiteboard: sb+

I've managed to get something working using the intercepted NtOpenKey (TargetNtOpenKey) that is already in the sandbox.
However the additional hoop that I've had to jump through to get that working, makes me think we should still set the proper registry entries when we can and only use the interception when we can't.

So, I'm going to land the installer patch and do the other fix in bug 1515088.

[Tracking Requested - why for this release]:
ARM 66 uplift candidate

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Can you request uplift when you're ready to try bringing this to beta? Would you need the work from the other patch too?

Flags: needinfo?(bobowencode)

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #11)

Can you request uplift when you're ready to try bringing this to beta? Would you need the work from the other patch too?

I've decided I'm actually going to back out this patch in bug 1515088, because in conversation with dmajor, we came up with a better and much simpler approach, which on balance I think we can use all the time instead of the registry keys.
I'll update the other bug with the details.

Flags: needinfo?(bobowencode)
You need to log in before you can comment on or make changes to this bug.