cppwinrt should not call LoadLibrary anywhere unless new WINRT_REG_FREE define is defined, re-activating regfree behavior

[dmachaj]

(This is a fresh attempt at what #1414 was doing)

This is a behavioral breaking change

Unfortunately the behavior change is only visible at runtime (the compiler will not tell you about it). If you upgrade to a newer cppwinrt version and start observing REGDB_E_CLASSNOTREG exceptions then that means you were relying on this behavior.

There are two possible resolutions to that problem:

1. [Less preferred] Add #define WINRT_REG_FREE before #include <winrt/base.h> to re-enable regfree activation behavior. This only needs to be defined in one translation unit contributing to your binary.
2. [More preferred] Use a registration file for the activatable class so that the activation behavior is deterministic. Unpackaged applications can use a fusion manifest. Packaged applications can use InProcessServer ActivatableClass

If you would like assurance that the regfree behavior is disabled in your binary you can define WINRT_NEVER_REG_FREE before including <winrt/base.h>. If any translation unit in the binary has enabled regfree then the never-regfree define will break the build, identifying the problem.
 

Why is this change being made?

There are a few categories of problems that come with the regfree behavior in cppwinrt:

1. Unintended path searching on failed activations (that were intended to fail because a class is being activated that is not installed) can lead to code injection. Disabling this feature entirely is additional defense in depth after the primary fix in Use safe DLL loading (avoid current directory) #1293.
2. Misconfigured apps that "happen to work" because they get lucky and regfree activations return the expected class, but then something changes and they stop being so lucky.
3. Failed activations for reasons other than "class not found", such as an out of process server returning an RPC error, can cause the DLL to be loaded inproc. This can create all sorts of difficult bugs because there are now multiple instances of what is supposed to be a singleton. Or the class fails to run properly because it is expecting to run in a specific local server and is instead running in the calling process.
4. Different behavior across language projections. An activation that succeeds using this behavior may fail with equivalent code in C++/CX, C#, or other languages.

Briefly summarize what changed

The easiest/smallest change is that CoIncrementMTAUsage now comes from direct linkage to COM instead of dynamically at runtime.

The bigger change is to regfree activation. This (often unexpected) behavior uses the class name of the thing being activated to LoadLibrary and try to activate based on that. For example, if you are activating Contoso.Subnamespace.Class then it will try to LoadLibrary("Contoso.Subnamespace.dll") and activate from that. If it fails it will then try LoadLibrary("Contoso.dll"). This repeats until there are no namespaces left. This behavior is now disabled by default.

The behavior disable is implemented in a non-ODR-violating way by using a function pointer. If WINRT_REG_FREE is not defined then this pointer is always null and the regfree behavior is disabled. If WINRT_REG_FREE is defined then a dynamic initializer is used to fill in the function pointer with a non-null value. The regfree activation code is then active. The activation code is unchanged aside from moving it to a new function (and converting a templated bool to a parameterized bool).

In practice this makes it so that regfree behavior is missing unless at least a single translation unit in a binary defines WINRT_REG_FREE before including <winrt/base.h>. Once a single TU defines it then this behavior lights up and becomes active.

The reverse can be enforced by defining WINRT_NEVER_REG_FREE before including <winrt/base.h>. If any translation unit in the binary has enabled regfree then the never-regfree define will break the build, identifying the problem.

How was this change tested?

The existing test suite. Four of the test binaries relied on regfree activation behavior to activate test_component.dll classes. Those have been updated to define WINRT_REG_FREE and they then pass. All other test binaries leave it undefined.

I also used the test_cpp20 binary to enforce never-regfree behavior. There is a new test case that asserts that regfree activation fails. A fusion manifest for one of the test classes ensures that explicitly registered activation still works.

[dmachaj]

This ifdef makes it so that there is definitely no LoadLibrary usage when regfree is inactive.

[sylveon]

A lot of users likely fell in the pitfall of relying on this behavior. For example, it's how Win2D and custom runtime components "just works" currently outside of packaged scenarios.

I heavily suggest keeping it on by default (for example, via a VS property which defaults to enabled), or at the very least clearly documenting this breaking change.

[dmachaj]

A lot of users likely fell in the pitfall of relying on this behavior. For example, it's how Win2D and custom runtime components "just works" currently outside of packaged scenarios.

I heavily suggest keeping it on by default (for example, via a VS property which defaults to enabled), or at the very least clearly documenting this breaking change.

That is fair feedback and I've been expecting some discussion regarding opt-in vs. opt-out. I think a strong argument could be made in both directions.

[sylveon]

A measure that could help alleviate this is automatically creating fusion manifest entries, like this

  <PropertyGroup>
    <BeforeLinkTargets>
      $(BeforeLinkTargets);
      _UnpackagedWin32GenerateAdditionalWinmdManifests;
    </BeforeLinkTargets>
  </PropertyGroup>
  <Target Name="_UnpackagedWin32MapWinmdsToManifestFiles" DependsOnTargets="ResolveAssemblyReferences">
    <ItemGroup>
      <!-- For each non-system .winmd file in References, generate a .manifest in IntDir for it. -->
      <_UnpackagedWin32WinmdManifest Include="@(ReferencePath->'$(IntDir)\%(FileName).manifest')" Condition="'%(ReferencePath.IsSystemReference)' != 'true' and '%(ReferencePath.WinMDFile)' == 'true' and '%(ReferencePath.ReferenceSourceTarget)' == 'ResolveAssemblyReference' and '%(ReferencePath.Implementation)' != ''">
        <WinMDPath>%(ReferencePath.FullPath)</WinMDPath>
        <Implementation>%(ReferencePath.Implementation)</Implementation>
      </_UnpackagedWin32WinmdManifest>
      <!--
          For each referenced project that _produces_ a winmd, generate a temporary item that maps to
          the winmd, and use that temporary item to generate a .manifest in IntDir for it.
          We don't set Implementation here because it's inherited from the _ResolvedNativeProjectReferencePaths.
      -->
      <_UnpackagedWin32WinmdProjectReference Condition="'%(_ResolvedNativeProjectReferencePaths.ProjectType)' != 'StaticLibrary' And '%(_ResolvedNativeProjectReferencePaths.DeploymentContent)' != 'false'" Include="@(_ResolvedNativeProjectReferencePaths-&gt;WithMetadataValue('FileType','winmd')-&gt;'%(RootDir)%(Directory)%(TargetPath)')" />
      <_UnpackagedWin32WinmdManifest Include="@(_UnpackagedWin32WinmdProjectReference->'$(IntDir)\%(FileName).manifest')">
        <WinMDPath>%(Identity)</WinMDPath>
      </_UnpackagedWin32WinmdManifest>
    </ItemGroup>
  </Target>
  <Target Name="_UnpackagedWin32GenerateAdditionalWinmdManifests" Inputs="@(_UnpackagedWin32WinmdManifest.WinMDPath)" Outputs="@(_UnpackagedWin32WinmdManifest)" DependsOnTargets="_UnpackagedWin32MapWinmdsToManifestFiles">
    <Message Text="Generating manifest for %(_UnpackagedWin32WinmdManifest.WinMDPath)" Importance="High" />
    <!-- This target is batched and a new Exec is spawned for each entry in _UnpackagedWin32WinmdManifest. -->
    <Exec Command="mt.exe -nologo -winmd:%(_UnpackagedWin32WinmdManifest.WinMDPath) -dll:%(_UnpackagedWin32WinmdManifest.Implementation) -out:%(_UnpackagedWin32WinmdManifest.Identity)" />
    <ItemGroup>
      <!--
          Emit the generated manifest into the Link inputs.
          Pass a metadata name that isn't used to wipe all metadata because otherwise VS tries copying the manifest to the output as %(FileName).winmd
      -->
      <Manifest Include="@(_UnpackagedWin32WinmdManifest)" KeepMetadata="DoesntExist" />
    </ItemGroup>
  </Target>

This should pretty much make this change work without manual intervention for most use cases on 18362+ (should probably still document it for those who don't fall in that though)

[dmachaj]

Another interesting note that was raised in discussions. The regfree activation doesn't look at the failure reason before attempting inproc loads. If the activation is supposed to go to a COM server then RPC errors will lead to the regfree activation path. That would attempt loading DLLs inproc and activating out of them, which may be a very wrong thing to do.

[dmachaj]

Another interesting note that was raised in discussions. The regfree activation doesn't look at the failure reason before attempting inproc loads. If the activation is supposed to go to a COM server then RPC errors will lead to the regfree activation path. That would attempt loading DLLs inproc and activating out of them, which may be a very wrong thing to do.

Fixed this by adding a new "only if error_class_not_registered" check before calling the regfree activation helper.

[jonwis]

I agree that this change is a behavioral break - someone relying on the prior version will get different results after picking up this cppwinrt version. Adding a note to this PR with a "you will know you relied on this when your app starts getting not-registered exceptions; you can fix it by adding {these four lines} to your DLL/exe main" is how I'd resolve that.

I don't love a compile-time configuration flag (like a flag to cppwinrt, or a #define that we key off of) to disable it.

We should be secure by default so IMO this is a righteous change that gets us closer to that.

[sylveon]

Adding a note to this PR with a "you will know you relied on this when your app starts getting not-registered exceptions; you can fix it by adding {these four lines} to your DLL/exe main" is how I'd resolve that.

I would argue this isn't really searchable. It should be added to https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/troubleshooting because it shows up as the second result when searching for "cppwinrt class not registered" on Google and Bing.

[jonwis]

Make sure to including that "breaking change notification" in the release notes.

[dmachaj]

I agree that this change is a behavioral break - someone relying on the prior version will get different results after picking up this cppwinrt version. Adding a note to this PR with a "you will know you relied on this when your app starts getting not-registered exceptions; you can fix it by adding {these four lines} to your DLL/exe main" is how I'd resolve that.

I don't love a compile-time configuration flag (like a flag to cppwinrt, or a #define that we key off of) to disable it.

We should be secure by default so IMO this is a righteous change that gets us closer to that.

I added a new section to the PR description that is very explicit this is a breaking change and two approaches to respond.

[dmachaj]

Adding a note to this PR with a "you will know you relied on this when your app starts getting not-registered exceptions; you can fix it by adding {these four lines} to your DLL/exe main" is how I'd resolve that.

I would argue this isn't really searchable. It should be added to https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/troubleshooting because it shows up as the second result when searching for "cppwinrt class not registered" on Google and Bing.

Good call. I don't think those docs can be updated until there is a cppwinrt release with this change in it (the most recent release is April 2024). Otherwise the explanation would be incomplete because it can't say which version is affected (because there is no released version). Once this makes it into a release I will get that page updated.

[dmachaj]

Just in case anyone is wondering - I haven't forgotten about this PR and I do intend to merge it. This carries a breaking change so I'd like to give the current HEAD a chance to go through some validation in the OS first. That way if we need to take any additional fixes we can do so without also having to consume the breaking change.

[github-actions]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[dmachaj]

[keepalive]

[sylveon]

The stale bot is way too aggressive

[github-actions]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.