1733734 - Create private SxS assembly for VC runtime

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

It should be possible to prevent the issues of bug 1624546 by having firefox.exe depend on an assembly containing the Visual C++ runtime DLLs (vcruntime140.dll, msvcp140.dll), which will preempt loading these from a broken system32 (which our sandboxing mitigation is set to prefer). This was earlier used in bug 1496179 for mozglue.dll.

The only way I know of building an assembly from existing DLLs (which we are not allowed to modify) is to put them together in a subdirectory with a .manifest.

I propose to keep this relatively simple having the assembly only include the Visual C++ runtime DLLs, not the UCRT (which is now considered a Windows component), nor others (D3D compiler, DIA). Bug 1624546 really only shows up because Microsoft have made the 2015 through 2019 runtimes use the same base DLLs, with added extensions in vcruntime140_1.dll which can be accidentally uninstalled while e.g. concrt140.dll still depends on it.

Another simplification: This is only really needed for programs run with certain sandbox mitigations (prefer system32), so I propose to only modify the manifest of the two executables run that way (firefox.exe and plugin-container.exe) to depend on the assembly, and leave the existing loose DLLs in place for the others (crashreporter.exe, minidump-analyzer.exe, pingsender.exe, updater.exe, default-browser-agent.exe, maintenance service stuff, uninstaller and maybe its plugins). This will reduce the amount of risk, modification and testing, at the cost of 170 KB additional installer size and 700 KB on disk.

Possible issues:

• Feels like a hack, or deprecated. Microsoft used to ship vcruntime as assembly with manifests, but doesn't anymore. Why not?
• Opens new and strange vistas of the loader. (Though it seems to work fine with mozglue)
• May impact startup time, need to measure.
• arm64? x86 on arm64?
• Ok on older Windows versions? (maybe particularly issues with network drives?)
• There may be other ways to run into missing dependencies, perhaps with sloppy DLL injection?
• Can't depend on the assembly for local builds where we don't package the redist, so there will need to be some conditional preprocessing of the manifests.
• Won't get any fixes from updates to the system vcruntime.

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #0)

• Can't depend on the assembly for local builds where we don't package the redist, so there will need to be some conditional preprocessing of the manifests.

Simpler: Use an empty assembly with the same name for builds that don't package the redist.

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: WIP: Bug 1733734 WIP - Depend on private assembly with vcruntime

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #0)

The only way I know of building an assembly from existing DLLs (which we are not allowed to modify) is to put them together in a subdirectory with a .manifest.

Is a subdirectory really required? We can put a .manifest in the appdir according to the documentation:

1. Side-by-side searches the WinSxS folder.
2. \<appdir>\<assemblyname>.DLL
3. \<appdir>\<assemblyname>.manifest
4. \<appdir>\<assemblyname>\<assemblyname>.DLL
5. \<appdir>\<assemblyname>\<assemblyname>.manifest

[emphasis mine]

We did not use a subdirectory when we shipped MSVC 2005 CRT DLLs. Furthermore, it will avoid the network drive problem:

Another way to fix this is to put the VC runtime libraries directly into the application folder, not in the subdirectory Microsoft.VC90.CRT.

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Masatoshi Kimura [:emk] from comment #3)

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #0)

The only way I know of building an assembly from existing DLLs (which we are not allowed to modify) is to put them together in a subdirectory with a .manifest.

Is a subdirectory really required?

Looks like it's fine without the subdirectory, thanks! I'll update the patch.

Not sure why I remembered that not working. I might have let the loader see the manifest and DLLs in the subdir first, that gets cached very aggressively once it's been looked up, really need to reboot between tests.

---

Several updater tests failed on try, I'll be looking into those. I also need to figure out what the situation would be with artifact builds, they should be able to just copy over the right manifest but it may not work yet.

Comment 5

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I'm not going to be able to work out the update test issues, so I'm abandoning the patch. Improvements welcome.

Comment 6

[Nick Alexander :nalexander [he/him]]

I'm going to see about pushing this across the line. I can confirm that no subdirectory is needed; thanks, :emk! Notes for myself:

• updater tests failing
• ensure artifact builds succeed
• ensure that MSIX installations succeed
• consider including more of VC redist, not sure why we're not shipping all of the DLLs

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: WIP: Bug 1733734 WIP - Use private SxS assembly to lock down MSVCRT.

This introduces a SxS assembly to ensure that we use the Microsoft
Visual C++ runtime (MSVCRT) DLLs that ship with the application
(usually, Firefox) itself: namely, those sibling to firefox.exe in
the GRE binary directory. The goal is to prevent situations like
those described in Bug 1624546.

We include only the MSVCRT DLLs, not the UCRT (which is
now considered a Windows component), nor others (D3D compiler, DIA),
because Bug 1624546 really only shows up because Microsoft have made
the 2015 through 2019 runtimes use the same base DLLs, with added
extensions in vcruntime140_1.dll (not present in the version Mozilla
currently redistributes!) which can be accidentally uninstalled while
vcruntime140.dll (which is present in the version Mozilla currently
redistributes) still depends on it.

It's not clear to me why we do not, at this time, include additional
msvcp140_{1,2}.dll files; there are reports that we may sometimes
require functionality that dynamically loads these DLLs. I think it
makes sense to add them to the redistributed and required set but
should we choose to do that, I'll make it a separate commit.

In general, automation builds will include the MSVCRT redistributable
DLLs and local builds will not. To accommodate this, we need an
optional SxS assembly. We could either include a varying external
assembly .manifest file and require it to be sibling to
firefox.exe, or we could generate and make conditional the internal
firefox.exe.manifest. I've elected to include an external assembly
.manifest, since it's not clear that manifest generation is
supported by the build and this new .manifest file is conceptually
similar to its sibling MSVCRT .dll files. This impacts at least
artifact builds.

Comment 8

[Nick Alexander :nalexander [he/him]]

Unassigning myself since I'm not going to get this across the line. The patch above passed tests for me not that long ago; a mach try auto push is percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b25385bd8823f32ebacfa2729ea2bd1942cd4dc3.

Comment 10

[Nick Alexander :nalexander [he/him]]

rkraesig: Is there any chance you know about SxS assemblies and/or VC redistribution? We have a background hum of failing Firefox installs due to distributing the VC runtime, but not necessarily in the right configuration. If you know anything about these details, could you read the ticket and comment on the approach we propose here?

Comment 11

[Ray Kraesig [:rkraesig]]

(In reply to Nick Alexander :nalexander [he/him] from comment #10)

rkraesig: Is there any chance you know about SxS assemblies and/or VC redistribution?

I haven't touched SxS assemblies in ten years or more; and most of what I remember about them is that they caused more problems than they solved and I never wanted to touch them again.

I know a little about VC redistribution, but you'd still probably want to ask :glandium instead.

[...] could you read the ticket and comment on the approach we propose here?

I don't think it would have solved the problem described here: specifically, I'm not at all convinced that forcing our local version of vcruntime140.dll to load would have prevented vcruntime140_1.dll from being loaded!

Neither the VS2017 nor the VS2019 versions of vcruntime140.dll have vcruntime140_1.dll as a declared dependency, so it must have been loaded either by another binary or by magic shenanigans. Another binary would still have tried to load it, and shenanigans might have as well — indeed, almost certainly would, if the shenanigans in question are the dependency on the pseudo-DLL "api-ms-win-crt-runtime-l1-1-0.dll". (Which both the VS2017 and VS2019 versions have; and which is a mechanism that, per the usual documentation, is resolved before SxS manifests are considered.)

I also don't think it would solve the problem you're seeing in the linked bug 1749443; that smells like a red herring, especially if there are no other warnings about absent DLLs. (But I admittedly haven't seen the failures in question; I'm just going off of bug 1749443 comment 11.)

Comment 12

[Mike Hommey [:glandium]]

Note that since this bug was open, things have changed and we are shipping vcruntime140_1.dll now (since bug 1832467).