Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Projected winrt::consume_ methods will nullptr crash if the underlying QueryInterface call fails #1442

Merged
merged 9 commits into from
Oct 30, 2024
1 change: 1 addition & 0 deletions strings/base_extern.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ 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);

Expand Down
6 changes: 6 additions & 0 deletions strings/base_implements.h
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,12 @@ namespace winrt::impl
return m_inner.try_as<Qi>();
}

template <typename Qi>
auto as_or_failfast() const noexcept
{
return m_inner.as_or_failfast<Qi>();
}

explicit operator bool() const noexcept
{
return m_inner.operator bool();
Expand Down
2 changes: 1 addition & 1 deletion strings/base_meta.h
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ namespace winrt::impl
{
operator I() const noexcept
{
return static_cast<D const*>(this)->template try_as<I>();
return static_cast<D const*>(this)->template as_or_failfast<I>();
jonwis marked this conversation as resolved.
Show resolved Hide resolved
}
};

Expand Down
31 changes: 31 additions & 0 deletions strings/base_windows.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,31 @@ namespace winrt::impl
ptr->QueryInterface(guid_of<To>(), &result);
return wrap_as_result<To>(result);
}

template <typename To, typename From, std::enable_if_t<is_com_interface_v<To>, int> = 0>
com_ref<To> as_or_failfast(From* ptr) noexcept
{
#ifdef WINRT_DIAGNOSTICS
get_diagnostics_info().add_query<To>();
#endif

if (!ptr)
{
return nullptr;
}

void* result{};
hresult code = ptr->QueryInterface(guid_of<To>(), &result);
if (code < 0)
{
// Capture an error context immediately before the failfast call. The failfast gives the best
// output when there is a "stowed" exception. Capturing the error context will stow it enough
// for the failfast to find it and use it when creating an exception context record.
WINRT_IMPL_RoCaptureErrorContext(code);
dmachaj marked this conversation as resolved.
Show resolved Hide resolved
WINRT_IMPL_RoFailFastWithErrorContext(code);
}
return wrap_as_result<To>(result);
}
}

WINRT_EXPORT namespace winrt::Windows::Foundation
Expand Down Expand Up @@ -205,6 +230,12 @@ WINRT_EXPORT namespace winrt::Windows::Foundation
return impl::try_as<To>(m_ptr);
}

template <typename To>
auto as_or_failfast() const noexcept
{
return impl::as_or_failfast<To>(m_ptr);
}

template <typename To>
void as(To& to) const
{
Expand Down
56 changes: 56 additions & 0 deletions test/test/missing_required_interfaces.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#include "pch.h"

// Unset lean and mean so we can implement a type from the test_component namespace
#undef WINRT_LEAN_AND_MEAN
#include <winrt/test_component.h>

namespace
{
struct LiesAboutInheritance : public winrt::implements<LiesAboutInheritance, winrt::test_component::ILiesAboutInheritance>
{
LiesAboutInheritance() = default;
void StubMethod() {}
};
}

bool g_failfastCalled = false;

void __stdcall WINRT_IMPL_RoFailFastWithErrorContext(int32_t) noexcept
{
g_failfastCalled = true;
}

void DoTheUncatcheable(winrt::test_component::LiesAboutInheritance& lies) noexcept
dmachaj marked this conversation as resolved.
Show resolved Hide resolved
{
lies.ToString();
}

bool CatchTheUncatcheable(winrt::test_component::LiesAboutInheritance& lies) noexcept
{
bool caughtCrash = false;
__try
{
// We verify that our replacement version of WINRT_IMPL_RoFailFastWithErrorContext is
// called. In a real program that would exit the process. But this is a test so it
// continues execution and hits a nullptr dereference instead. Eat that known/expected
// error and allow execution to continue onto other test cases.
DoTheUncatcheable(lies);
}
__except (EXCEPTION_EXECUTE_HANDLER)
{
caughtCrash = true;
}
return caughtCrash;
}

TEST_CASE("missing_required_interfaces")
{
auto lies = winrt::make_self<LiesAboutInheritance>().as<winrt::test_component::LiesAboutInheritance>();
REQUIRE(lies);
REQUIRE_NOTHROW(lies.StubMethod());

REQUIRE(!g_failfastCalled);
const bool caughtCrash = CatchTheUncatcheable(lies);
REQUIRE(caughtCrash);
REQUIRE(g_failfastCalled);
}
dmachaj marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions test/test/test.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@
<PrecompiledHeader>NotUsing</PrecompiledHeader>
</ClCompile>
<ClCompile Include="memory_buffer.cpp" />
<ClCompile Include="missing_required_interfaces.cpp" />
<ClCompile Include="module_lock_dll.cpp">
<PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">NotUsing</PrecompiledHeader>
<PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">NotUsing</PrecompiledHeader>
Expand Down
7 changes: 7 additions & 0 deletions test/test_component/test_component.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading