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

Avoid LoadLibraryExW #1414

Closed
wants to merge 2 commits into from
Closed

Avoid LoadLibraryExW #1414

wants to merge 2 commits into from

Conversation

kennykerr
Copy link
Collaborator

Even though C++/WinRT has provided safe DLL loading #1293 for over a year, there is still some risk that the fallback logic can trip up more interesting scenarios encountered by the OS. This just locks C++/WinRT down further by not using LoadLibraryExW for anything by default.

To enable the traditional "reg free" model you can define WINRT_REG_FREE.

@kennykerr kennykerr requested a review from oldnewthing April 16, 2024 19:56
hr = WINRT_IMPL_RoGetActivationFactory(*(void**)(&name), guid, result);
}

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

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

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

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

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

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

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

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

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants