Skip to content

Avoid LoadLibraryExW #1414

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.

Sign up for GitHub

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

Closed
kennykerr wants to merge 2 commits into from
Closed

Avoid LoadLibraryExW #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
419b4be
Avoid LoadLibrary
kennykerr Apr 16, 2024
71e23aa
Merge branch 'master' into sans-load-library
kennykerr Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 40 additions & 46 deletions strings/base_activation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,69 +30,63 @@ namespace winrt::impl

if (hr == impl::error_not_initialized)
{
auto usage = reinterpret_cast<int32_t(__stdcall*)(void** cookie) noexcept>(WINRT_IMPL_GetProcAddress(load_library(L"combase.dll"), "CoIncrementMTAUsage"));

if (!usage)
{
return hr;
}

void* cookie;
usage(&cookie);
WINRT_IMPL_CoIncrementMTAUsage(&cookie);
hr = WINRT_IMPL_RoGetActivationFactory(*(void**)(&name), guid, result);
}

if (hr == 0)
#ifdef WINRT_REG_FREE
Copy link
Contributor

@dmachaj dmachaj Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WINRT_REG_FREE

Can you add ODR checks for this define? If it mismatches then it will be undefined behavior whether you get regfree or not.

Copy link
Collaborator Author

@kennykerr kennykerr Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to push a contribution. That's not something I've ever played with. 😊

Copy link
Contributor

@dmachaj dmachaj Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I'm catching up after more than a week OOF so I have very limited time today. That's the diff that detects the mismatch. The test projects have several build breaks when this is enforced because they are not consistent. I don't have time today to follow up and fix them all. I'm not going to push a build break so that will have to wait. Or you are welcome to fix it up first if you have time :).

Copy link
Contributor

@dmachaj dmachaj Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of the breaks:

Error LNK2038 mismatch detected for 'WINRT_REG_FREE': value 'false' doesn't match value 'true' in Composition.obj test_slow V:\T\cppwinrt\test\test_slow\main.obj 1

Copy link
Collaborator Author

@kennykerr kennykerr Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet thanks, I'll take a look.

Copy link
Contributor

@dmachaj dmachaj Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have the linker errors then you won't have confidence that this define is having an effect (or is missing). Every translation unit will have a potentially different version of this function. The linker will choose one unpredicatbly. Does it have regfree? Does it not? Who knows?! It might even change the next time you add a new cpp file to your project.

Copy link
Member

@jonwis jonwis Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OS should likely default to "not regfree" - I'm not aware of any outliers that want it.

Copy link
Contributor

@sylveon sylveon Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External users would most likely prefer reg-free, because consuming libraries like Win2D can be a painful endeavour for less experienced users otherwise (knowing why you get class not registered and what the solution is, is less than obvious to even knowledgeable users sometimes).

I would suggest making this opt out rather than opt in, especially since the existing published versions use reg-free, otherwise we should expect some amount of "class not registered after upgrading cppwinrt" tickets.

Copy link
Contributor

@sylveon sylveon Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of the breaks:

Error LNK2038 mismatch detected for 'WINRT_REG_FREE': value 'false' doesn't match value 'true' in Composition.obj test_slow V:\T\cppwinrt\test\test_slow\main.obj 1

Could these specific examples be because the main.cpp doesn't include the PCH? Defining this macro at the VS project level rather than in the PCH might be the ideal move here.

Copy link
Collaborator Author

@kennykerr kennykerr Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it's easy enough to fix this build. The bigger question is harder to answer. The options are:

  1. opt-in with WINRT_REG_FREE
  2. opt-out with WINRT_NO_REG_FREE
  3. do nothing

There are pros and cons to all of them. I think option 3 is good enough - and secure enough thanks to #1293 - and is certainly far less problematic from a compatibility point of view. I also don't want to add to the growing set of compatibility issues that someone will have to deal with when trying to update a large code base like Windows. This does serve as another good example of how hard it is to update a C++ library. Rust for examples does not have this problem since source code is not the unit of integration.

@dmachaj, @oldnewthing, and I have had a bit of an offline discussion with different perspectives. I'm not personally invested in the outcome as I'm not the customer here, so I'll leave this here as an example and let someone who is more invested in the outcome take it from here.

if (hr != 0)
{
return 0;
}
com_ptr<IErrorInfo> error_info;
WINRT_IMPL_GetErrorInfo(0, error_info.put_void());

com_ptr<IErrorInfo> error_info;
WINRT_IMPL_GetErrorInfo(0, error_info.put_void());
std::wstring path{ static_cast<hstring const&>(name) };
std::size_t count{};

std::wstring path{ static_cast<hstring const&>(name) };
std::size_t count{};
while (std::wstring::npos != (count = path.rfind('.')))
{
path.resize(count);
path += L".dll";
library_handle library(load_library(path.c_str()));
path.resize(path.size() - 4);

while (std::wstring::npos != (count = path.rfind('.')))
{
path.resize(count);
path += L".dll";
library_handle library(load_library(path.c_str()));
path.resize(path.size() - 4);
if (!library)
{
continue;
}

if (!library)
{
continue;
}
auto library_call = reinterpret_cast<int32_t(__stdcall*)(void* classId, void** factory)>(WINRT_IMPL_GetProcAddress(library.get(), "DllGetActivationFactory"));

auto library_call = reinterpret_cast<int32_t(__stdcall*)(void* classId, void** factory)>(WINRT_IMPL_GetProcAddress(library.get(), "DllGetActivationFactory"));
if (!library_call)
{
continue;
}

if (!library_call)
{
continue;
}
com_ptr<abi_t<Windows::Foundation::IActivationFactory>> library_factory;

com_ptr<abi_t<Windows::Foundation::IActivationFactory>> library_factory;
if (0 != library_call(*(void**)(&name), library_factory.put_void()))
{
continue;
}

if (0 != library_call(*(void**)(&name), library_factory.put_void()))
{
continue;
if constexpr (isSameInterfaceAsIActivationFactory)
{
*result = library_factory.detach();
library.detach();
return 0;
}
else if (0 == library_factory.as(guid, result))
{
library.detach();
return 0;
}
}

if constexpr (isSameInterfaceAsIActivationFactory)
{
*result = library_factory.detach();
library.detach();
return 0;
}
else if (0 == library_factory.as(guid, result))
{
library.detach();
return 0;
}
WINRT_IMPL_SetErrorInfo(0, error_info.get());
}
#endif

WINRT_IMPL_SetErrorInfo(0, error_info.get());
return hr;
}

Expand Down
1 change: 1 addition & 0 deletions strings/base_extern.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ extern "C"
int32_t __stdcall WINRT_IMPL_GetErrorInfo(uint32_t reserved, void** info) noexcept WINRT_IMPL_LINK(GetErrorInfo, 8);
int32_t __stdcall WINRT_IMPL_CoInitializeEx(void*, uint32_t type) noexcept WINRT_IMPL_LINK(CoInitializeEx, 8);
void __stdcall WINRT_IMPL_CoUninitialize() noexcept WINRT_IMPL_LINK(CoUninitialize, 0);
int32_t __stdcall WINRT_IMPL_CoIncrementMTAUsage(void** cookie) noexcept WINRT_IMPL_LINK(CoIncrementMTAUsage, 4);

int32_t __stdcall WINRT_IMPL_CoCreateFreeThreadedMarshaler(void* outer, void** marshaler) noexcept WINRT_IMPL_LINK(CoCreateFreeThreadedMarshaler, 8);
int32_t __stdcall WINRT_IMPL_CoCreateInstance(winrt::guid const& clsid, void* outer, uint32_t context, winrt::guid const& iid, void** object) noexcept WINRT_IMPL_LINK(CoCreateInstance, 20);
Expand Down
3 changes: 2 additions & 1 deletion test/old_tests/UnitTests/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#define WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#define WINRT_REG_FREE
#define WINRT_NATVIS
#define _SILENCE_CXX17_UNCAUGHT_EXCEPTION_DEPRECATION_WARNING

Expand Down
1 change: 1 addition & 0 deletions test/test/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "mingw_com_support.h"

#define WINRT_LEAN_AND_MEAN
#define WINRT_REG_FREE
#include <unknwn.h>
#include "winrt/Windows.Foundation.Collections.h"
#include "winrt/Windows.Foundation.Numerics.h"
Expand Down
1 change: 1 addition & 0 deletions test/test_fast/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#define WINRT_REG_FREE
#include "catch.hpp"
#include "winrt/Windows.Foundation.Collections.h"

Expand Down
1 change: 1 addition & 0 deletions test/test_slow/pch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#define WINRT_REG_FREE
#include "catch.hpp"
#include "winrt/Windows.Foundation.Collections.h"

Expand Down