microsoft / cppwinrt

C++/WinRT
MIT License
1.64k stars 238 forks source link

Bug: It is possible to wrongly assign through a non-const ref to Unknown or IInspectable #1298

Closed JaiganeshKumaran closed 1 year ago

JaiganeshKumaran commented 1 year ago

Version

All versions

Summary

Currently all projected types inherit from winrt::Windows::Foundation::IUnknown or winrt::Windows::Foundation::IInspectable, which implies that you are able to assign a non-const lvalue reference to it when you have any non-const projected type. And since IUnknown can be assigned to anything, it allows assigning to even types other than the original.

Reproducible example

#include <winrt/Windows.Foundation.h>

int main()
{
    winrt::Windows::Foundation::Uri uri{L""};
    winrt::Windows::Foundation::IUnknown /* or IInspectable */& uriUnknown = uri;
    uriUnknown = winrt::Windows::Foundation::MemoryBuffer{ 0U }; // assigning to wrong type.
    uri.Domain(); // Undefined behaviour.
}

Godbolt repro: https://godbolt.org/z/GaEcrYjGP.

Expected behaviour

winrt::Windows::Foundation::IUnknown / or IInspectable /& uriUnknown = uri; // error

Actual behaviour

winrt::Windows::Foundation::IUnknown / or IInspectable /& uriUnknown = uri; // works

Additional comments

Provide a separate base class for projected types instead of using IUnknown and IInspectable, and have a conversion operator instead, just like other interfaces but there would be no need for QueryInterface.

This is a breaking change but code that relied on assigning non-const ref to IUnknown or IInspectable was probably incorrect and non-const works for most cases.

kennykerr commented 1 year ago

This does not warrant a breaking change. My suggestion is not to do that.

JaiganeshKumaran commented 1 year ago

A similar issue exists for com_array. You can take a reference to array_view (due to inheritance) and assign it to any other array_view, leaking the original com_array data and, worse, calling CoTaskMemFree on the non-owning pointer set from array_view. An easy fix would be to inherit privately from array_view, disallowing binding non-const ref to array_view, and then 'using' every member function of array_view so you still get them on com_array.