Skip to content

Commit

Permalink
Restore OpenConsoleProxyStub's dependency on the CRT to fix QI (#13254)
Browse files Browse the repository at this point in the history
When #13160 introduced a new interface to the IConsoleHandoff idl, it
changed midl's RPC proxy stub lookup algorithm from a direct GUID
comparison to an unrolled binary search. Now, that would ordinarily not
be a problem...

However, in #11610, we took a shortcut and replaced `memcmp` -- used
only by RPC for GUID comparison -- with a direct GUID-only equality
comparator. This worked totally fine, and ordinarily would not be a
problem...

The unrolled binary search unfortunately _relies on memcmp's contract_:
it uses memcmp to match against a fully sorted set. Our memcmp only
returned 0 or 1 (equal or not), and it knew nothing about ordering.

When a package that contains a PackagedCOM proxy stub is installed, it
is selected as the primary proxy stub for any interfaces it can proxy.
After all, interfaces are immutable, so it doesn't matter whose proxy
you're using. Now, given that we installed a *broken* proxy... *all*
IIDs that got whacked by our memcmp issue broke for every consumer.

To fix it: instead of implementing memcmp ourselves, we're just going to
take a page out of WinAppSDK's book and link this binary using the
"Hybrid CRT" model. It will statically link any parts of the STL it uses
(none) and dynamically link the ucrt (which is guaranteed to be present
on Windows.)

Sure, the binary size goes up from 8k to 24k, but... the cost is never
having to worry about this again.

Closes #13251
  • Loading branch information
DHowett authored Jun 9, 2022
1 parent 4e20a86 commit cd35bc5
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 28 deletions.
2 changes: 0 additions & 2 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,6 @@ guardxfg
guc
gui
guidatom
guiddef
GValue
GWL
GWLP
Expand Down Expand Up @@ -1567,7 +1566,6 @@ NOCOLOR
NOCOMM
NOCONTEXTHELP
NOCOPYBITS
nodefaultlib
nodiscard
NODUP
noexcept
Expand Down
56 changes: 41 additions & 15 deletions src/host/proxy/Host.Proxy.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<Import Project="$(SolutionDir)src\common.build.pre.props" />
<ItemGroup>
<Midl Include="IConsoleHandoff.idl">
<!--
<!--
In Razzle, IDL files generate %FileName%.h
In Visual Studio, IDL files generate %FileName%_h.h
Visual Studio is easier to override than Razzle.
Expand All @@ -41,7 +41,6 @@
<ItemGroup>
<ClInclude Include="$(IntDir)\IConsoleHandoff.h" />
<ClInclude Include="$(IntDir)\ITerminalHandoff.h" />
<ClInclude Include="nodefaultlib_shim.h" />
</ItemGroup>
<ItemGroup>
<ClCompile Include="$(IntDir)\dlldata.c" />
Expand All @@ -67,27 +66,54 @@
</ItemDefinitionGroup>
<ItemDefinitionGroup>
<ClCompile>
<AdditionalIncludeDirectories>.;..;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<CallingConvention Condition="'$(Platform)'!='ARM64'">StdCall</CallingConvention>
<AdditionalIncludeDirectories>..;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<CallingConvention>StdCall</CallingConvention>
<!-- Must be Stdcall on all platforms to resolve _ObjectStublessClient3 -->
<PreprocessorDefinitions>REGISTER_PROXY_DLL;WIN32;%(PreprocessorDefinitions)</PreprocessorDefinitions>
<PrecompiledHeader>NotUsing</PrecompiledHeader>
<BasicRuntimeChecks>Default</BasicRuntimeChecks>
<BufferSecurityCheck>false</BufferSecurityCheck>
<SDLCheck>false</SDLCheck>
<ForcedIncludeFiles>nodefaultlib_shim.h;%(ForcedIncludeFiles)</ForcedIncludeFiles>
</ClCompile>
<Link>
<ModuleDefinitionFile>OpenConsoleProxy.def</ModuleDefinitionFile>
<!--
Not depending on the CRT cuts binary size by half and prevents issues if this DLL
is copied elsewhere and executed outside of our app package without our bundled CRT present.
-->
<AdditionalDependencies />
<IgnoreAllDefaultLibraries>true</IgnoreAllDefaultLibraries>
<EntryPointSymbol>DllMain</EntryPointSymbol>
</Link>
</ItemDefinitionGroup>

<!--
OpenConsoleProxy gets copied out of our app package and into a shared system store. As such, it can't take a
dependency on any libraries inside our package **or** inside any of our dependency packages. It has to stand
on its own.
Therefore, we're going to use the Hybrid CRT model from WinAppSDK for only OpenConsoleProxy. It statically
links the runtime and STL and dynamically links the UCRT instead of the VC++ CRT. The UCRT ships with Windows.
WinAppSDK asserts that this is "supported according to the CRT maintainer."
-->
<ItemDefinitionGroup Condition="'$(Configuration)'=='Debug'">
<ClCompile>
<!-- We use MultiThreadedDebug, rather than MultiThreadedDebugDLL, to avoid DLL dependencies on VCRUNTIME140d.dll and MSVCP140d.dll. -->
<RuntimeLibrary>MultiThreadedDebug</RuntimeLibrary>
</ClCompile>
<Link>
<!-- Link statically against the runtime and STL, but link dynamically against the CRT by ignoring the static CRT
lib and instead linking against the Universal CRT DLL import library. This "hybrid" linking mechanism is
supported according to the CRT maintainer. Dynamic linking against the CRT makes the binaries a bit smaller
than they would otherwise be if the CRT, runtime, and STL were all statically linked in. -->
<IgnoreSpecificDefaultLibraries>%(IgnoreSpecificDefaultLibraries);libucrtd.lib</IgnoreSpecificDefaultLibraries>
<AdditionalOptions>%(AdditionalOptions) /defaultlib:ucrtd.lib</AdditionalOptions>
</Link>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)'=='Release'">
<ClCompile>
<!-- We use MultiThreaded, rather than MultiThreadedDLL, to avoid DLL dependencies on VCRUNTIME140.dll and MSVCP140.dll. -->
<RuntimeLibrary>MultiThreaded</RuntimeLibrary>
</ClCompile>
<Link>
<!-- Link statically against the runtime and STL, but link dynamically against the CRT by ignoring the static CRT
lib and instead linking against the Universal CRT DLL import library. This "hybrid" linking mechanism is
supported according to the CRT maintainer. Dynamic linking against the CRT makes the binaries a bit smaller
than they would otherwise be if the CRT, runtime, and STL were all statically linked in. -->
<IgnoreSpecificDefaultLibraries>%(IgnoreSpecificDefaultLibraries);libucrt.lib</IgnoreSpecificDefaultLibraries>
<AdditionalOptions>%(AdditionalOptions) /defaultlib:ucrt.lib</AdditionalOptions>
</Link>
</ItemDefinitionGroup>

<!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->
<Import Project="$(SolutionDir)src\common.build.post.props" />
</Project>
3 changes: 0 additions & 3 deletions src/host/proxy/Host.Proxy.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@
<ClInclude Include="$(IntDir)\ITerminalHandoff.h">
<Filter>Header Files</Filter>
</ClInclude>
<ClInclude Include="nodefaultlib_shim.h">
<Filter>Header Files</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<Midl Include="IConsoleHandoff.idl">
Expand Down
8 changes: 0 additions & 8 deletions src/host/proxy/nodefaultlib_shim.h

This file was deleted.

0 comments on commit cd35bc5

Please sign in to comment.