From b82340f3fce1ee9bbcc13151f6f74c374d03f24d Mon Sep 17 00:00:00 2001 From: David Machaj <46852402+dmachaj@users.noreply.github.com> Date: Wed, 30 Oct 2024 09:54:35 -0700 Subject: [PATCH] Bug: Projected winrt::consume_ methods will nullptr crash if the underlying QueryInterface call fails (#1442) # Why is this change being made? The generated projection code for interfaces that the metadata declares as required for a runtimeclass assume that QueryInterface never fails. Assuming the metadata is correct in the first place, this is a valid assumption for inproc calls. However, for cross-process calls the QI can fail even if the metadata is correct and the class really does implement all of the required interfaces. It can fail with E_ACCESSDENIED and a variety of other RPC error codes. When this happens there is a nullptr crash in the generated consume method. This can be very painful to debug because the HRESULT is lost by the time it crashes. # Briefly summarize what changed This set of changes fixes the crash by detecting the QueryInterface error and throwing an exception when that occurs during one of these required casts. The try_as method was changed to capture COM error context when the QI fails. The code gen surrounding WINRT_IMPL_STUB was changed to save the result into a temporary variable and then pass it to a new check_cast_result method. check_cast_result is marked noinline so that the binary size impact of throwing exceptions is limited to a single function instead of inlining into high-volume generated code. If the cast succeeded then nothing happens. If the cast failed, returning null, then the stored COM exception is retrieved. Assuming it is available the HRESULT is pulled out of it and it is thrown. This then propagates like any other exception. Callers are free to try and catch it or let it go uncaught and crash. Now they have the choice. I also added a new file of test code that exercises this code path. The test_component IDL declares a runtimeclass that implements IStringable. And then the implementation fails to implement IStringable. When ToString is called on this object it hits the failure path. The cppwinrt code gen will not allow this to happen so I had to directly use winrt::implements. # How was this change tested? Besides the new test cases I also wrote a little console app that crashes this way. I built and ran it using the latest stable cppwinrt as well as my private new one. As expected the debugger blame is far more useful with these changes. --- cppwinrt.sln | 3 + cppwinrt/code_writers.h | 20 +- scratch/scratch.vcxproj | 255 ++-------------------- strings/base_error.h | 22 ++ strings/base_extern.h | 3 + strings/base_windows.h | 6 +- test/test/missing_required_interfaces.cpp | 25 +++ test/test/test.vcxproj | 1 + test/test_component/test_component.idl | 7 + 9 files changed, 106 insertions(+), 236 deletions(-) create mode 100644 test/test/missing_required_interfaces.cpp diff --git a/cppwinrt.sln b/cppwinrt.sln index 43e96ffab..e2ac95942 100644 --- a/cppwinrt.sln +++ b/cppwinrt.sln @@ -88,6 +88,9 @@ EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "fast_fwd", "fast_fwd\fast_fwd.vcxproj", "{A63B3AD1-AB7B-461E-9FFF-2447F5BCD459}" EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "scratch", "scratch\scratch.vcxproj", "{E893622C-47DE-4F83-B422-0A26711590A4}" + ProjectSection(ProjectDependencies) = postProject + {D613FB39-5035-4043-91E2-BAB323908AF4} = {D613FB39-5035-4043-91E2-BAB323908AF4} + EndProjectSection EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "test_module_lock_none", "test\test_module_lock_none\test_module_lock_none.vcxproj", "{D48A96C2-8512-4CC3-B6E4-7CFF07ED8ED3}" ProjectSection(ProjectDependencies) = postProject diff --git a/cppwinrt/code_writers.h b/cppwinrt/code_writers.h index 44b889b81..0e516b528 100644 --- a/cppwinrt/code_writers.h +++ b/cppwinrt/code_writers.h @@ -1129,9 +1129,16 @@ namespace cppwinrt if (is_remove_overload(method)) { // we intentionally ignore errors when unregistering event handlers to be consistent with event_revoker + // + // The `noexcept` versions will crash if check_cast_result throws but that is no different than previous + // behavior where it would not check the cast result and nullptr crash. At least the exception will terminate + // immediately while preserving the error code and local variables. format = R"( template auto consume_%::%(%) const noexcept {% - WINRT_IMPL_SHIM(%)->%(%);% + auto const& castedResult = static_cast<% const&>(static_cast(*this)); + auto const abiType = *(abi_t<%>**)&castedResult; + check_cast_result(abiType); + abiType->%(%);% } )"; } @@ -1139,7 +1146,10 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const noexcept {% - WINRT_VERIFY_(0, WINRT_IMPL_SHIM(%)->%(%));% + auto const& castedResult = static_cast<% const&>(static_cast(*this)); + auto const abiType = *(abi_t<%>**)&castedResult; + check_cast_result(abiType); + WINRT_VERIFY_(0, abiType->%(%));% } )"; } @@ -1148,7 +1158,10 @@ namespace cppwinrt { format = R"( template auto consume_%::%(%) const {% - check_hresult(WINRT_IMPL_SHIM(%)->%(%));% + auto const& castedResult = static_cast<% const&>(static_cast(*this)); + auto const abiType = *(abi_t<%>**)&castedResult; + check_cast_result(abiType); + check_hresult(abiType->%(%));% } )"; } @@ -1161,6 +1174,7 @@ namespace cppwinrt bind(signature), bind(signature, false), type, + type, get_abi_name(method), bind(signature), bind(signature)); diff --git a/scratch/scratch.vcxproj b/scratch/scratch.vcxproj index 4a26617b4..45122c13d 100644 --- a/scratch/scratch.vcxproj +++ b/scratch/scratch.vcxproj @@ -1,5 +1,6 @@ + Debug @@ -35,277 +36,67 @@ + true + true + true + true 16.0 {E893622C-47DE-4F83-B422-0A26711590A4} scratch scratch - - - - Application - true - - - Application - true - - Application - true + + ..\_build\$(Platform)\$(Configuration) + false - - Application - false - true - - - Application - false - true - - - Application - false - true - - - Application + + true - - Application + false true - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - MaxSpeed - true - true - $(OutputPath);Generated Files;..\..\..\library - NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreaded - - - Console - true - true - - - - - - - - - - - - - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug - - - Console - - - - - - - - - - - + - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug + $(OutputPath);Generated Files; Console - - - - - - - - - - - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug - - - Console - - - - - - - - - - - - - Disabled - $(OutputPath);Generated Files;..\..\..\library - _MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreadedDebug - - - Console - - - - - - - - - - - + MaxSpeed true true - $(OutputPath);Generated Files;..\..\..\library NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) MultiThreaded - Console true true - - - - - - - - - + - MaxSpeed - true - true - $(OutputPath);Generated Files;..\..\..\library - NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreaded - - - Console - true - true - - - - - - - - - - - - - MaxSpeed - true - true - $(OutputPath);Generated Files;..\..\..\library - NOMINMAX;_MBCS;%(PreprocessorDefinitions) - %(AdditionalOptions) - MultiThreaded + Disabled + _MBCS;%(PreprocessorDefinitions) + MultiThreadedDebug - - Console - true - true - - - - - - - - - - NotUsing - Use - Use - Use - Use - Use - Use - Use - Use + Use - Create - Create - Create - Create - Create - Create - Create - Create + Create + + + diff --git a/strings/base_error.h b/strings/base_error.h index 6e0aa103a..8a3a3a088 100644 --- a/strings/base_error.h +++ b/strings/base_error.h @@ -546,6 +546,28 @@ namespace winrt::impl } return result; } + + template + WINRT_IMPL_NOINLINE void check_cast_result(T* from WINRT_IMPL_SOURCE_LOCATION_ARGS) + { + if (!from) + { + com_ptr restrictedError; + if (WINRT_IMPL_GetRestrictedErrorInfo(restrictedError.put_void()) == 0) + { + WINRT_IMPL_SetRestrictedErrorInfo(restrictedError.get()); + + int32_t code; + impl::bstr_handle description; + impl::bstr_handle restrictedDescription; + impl::bstr_handle capabilitySid; + if (restrictedError->GetErrorDetails(description.put(), &code, restrictedDescription.put(), capabilitySid.put()) == 0) + { + throw hresult_error(code, take_ownership_from_abi WINRT_IMPL_SOURCE_LOCATION_FORWARD); + } + } + } + } } #undef WINRT_IMPL_RETURNADDRESS diff --git a/strings/base_extern.h b/strings/base_extern.h index 92872d416..c0e83e75a 100644 --- a/strings/base_extern.h +++ b/strings/base_extern.h @@ -29,8 +29,11 @@ extern "C" int32_t __stdcall WINRT_IMPL_SetThreadpoolTimerEx(winrt::impl::ptp_timer, void*, uint32_t, uint32_t) noexcept WINRT_IMPL_LINK(SetThreadpoolTimerEx, 16); int32_t __stdcall WINRT_IMPL_SetThreadpoolWaitEx(winrt::impl::ptp_wait, void*, void*, void*) noexcept WINRT_IMPL_LINK(SetThreadpoolWaitEx, 16); int32_t __stdcall WINRT_IMPL_RoOriginateLanguageException(int32_t error, void* message, void* exception) noexcept WINRT_IMPL_LINK(RoOriginateLanguageException, 12); + int32_t __stdcall WINRT_IMPL_RoCaptureErrorContext(int32_t error) noexcept WINRT_IMPL_LINK(RoCaptureErrorContext, 4); void __stdcall WINRT_IMPL_RoFailFastWithErrorContext(int32_t) noexcept WINRT_IMPL_LINK(RoFailFastWithErrorContext, 4); int32_t __stdcall WINRT_IMPL_RoTransformError(int32_t, int32_t, void*) noexcept WINRT_IMPL_LINK(RoTransformError, 12); + int32_t __stdcall WINRT_IMPL_GetRestrictedErrorInfo(void**) noexcept WINRT_IMPL_LINK(GetRestrictedErrorInfo, 4); + int32_t __stdcall WINRT_IMPL_SetRestrictedErrorInfo(void*) noexcept WINRT_IMPL_LINK(SetRestrictedErrorInfo, 4); void* __stdcall WINRT_IMPL_LoadLibraryExW(wchar_t const* name, void* unused, uint32_t flags) noexcept WINRT_IMPL_LINK(LoadLibraryExW, 12); int32_t __stdcall WINRT_IMPL_FreeLibrary(void* library) noexcept WINRT_IMPL_LINK(FreeLibrary, 4); diff --git a/strings/base_windows.h b/strings/base_windows.h index 41030d368..f5230e1c8 100644 --- a/strings/base_windows.h +++ b/strings/base_windows.h @@ -128,7 +128,11 @@ namespace winrt::impl } void* result{}; - ptr->QueryInterface(guid_of(), &result); + hresult code = ptr->QueryInterface(guid_of(), &result); + if (code < 0) + { + WINRT_IMPL_RoCaptureErrorContext(code); + } return wrap_as_result(result); } } diff --git a/test/test/missing_required_interfaces.cpp b/test/test/missing_required_interfaces.cpp new file mode 100644 index 000000000..237e4ff41 --- /dev/null +++ b/test/test/missing_required_interfaces.cpp @@ -0,0 +1,25 @@ +#include "pch.h" + +// Unset lean and mean so we can implement a type from the test_component namespace +#undef WINRT_LEAN_AND_MEAN +#include + +namespace +{ + struct LiesAboutInheritance : public winrt::implements + { + LiesAboutInheritance() = default; + void StubMethod() {} + }; +} + +TEST_CASE("missing_required_interfaces") +{ + auto lies = winrt::make_self().as(); + REQUIRE(lies); + REQUIRE_NOTHROW(lies.StubMethod()); + + // The IStringable::ToString method does not exist on this type. In previous versions of cppwinrt + // this line would crash with a nullptr deference. It now throws an exception. + REQUIRE_THROWS_AS(lies.ToString(), winrt::hresult_error); +} diff --git a/test/test/test.vcxproj b/test/test/test.vcxproj index b75f2e219..a82465149 100644 --- a/test/test/test.vcxproj +++ b/test/test/test.vcxproj @@ -387,6 +387,7 @@ NotUsing + NotUsing NotUsing diff --git a/test/test_component/test_component.idl b/test/test_component/test_component.idl index fb7444889..a027c394c 100644 --- a/test/test_component/test_component.idl +++ b/test/test_component/test_component.idl @@ -163,6 +163,13 @@ namespace test_component static void StaticMethodWithAsyncReturn(); } + // This class declares that it implements another interface but under the covers it actually does + // not. This allows us to test the behavior when QI's that should not fail, do fail. + runtimeclass LiesAboutInheritance : Windows.Foundation.IStringable + { + void StubMethod(); + } + namespace Structs { struct All