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

Second take on DLL export versioning system #3139

Closed
wants to merge 38 commits into from

Conversation

wjk
Copy link
Contributor

@wjk wjk commented Sep 6, 2020

Purpose

This PR is a follow-up copy of #1872, since work on that PR seems to have stopped. I have fixed the merge conflicts in the original PR, addressed review comments from there, and wrote a unit test that exercises the functionality added.

JIRA issue: CORE-11288

Proposed changes

  • Add a feature to spec2def to generate a RosCompat descriptor (contained in a special section) pointing to a mask-array, that specifies for each named export on which Windows versions it is present.
  • Add a feature to the loader in NTDLL that checks for a RosCompat descriptor, and if present, uses it to patch the export table, splitting it into a public and private part.
  • Modify the loader to allow ReactOS DLLs (those that contain a RosCompat descriptor, even if it is empty) to import from private/hidden exports.
  • Add a CMake setting, ENABLE_DLL_VERSIONING, that defaults to ON. If enabled, the RosCompat descriptors will be emitted, and all APIs (up to and including Windows 10 APIs) will be compiled. If disabled, the RosCompat descriptors will not be emitted, and only NT5.2 APIs will be available. (The RosCompat parsing code will still be compiled in this case, but will do nothing.)

Open Questions

  • Should we remove the *_vista DLLs?
  • Do we have enough unit tests? The current test is a program that exercises kernel32!InitOnceExecuteOnce. This API is available in NT6.0+. Two copies of this version are compiled: versioning_win10.exe (whose manifest contains the Windows 10 compat GUID), which works; and versioning_xp.exe (whose manifest contains no compat GUIDs), which is expected to crash when run because InitOnceExecuteOnce is hidden by the loader.

tkreuzer and others added 30 commits September 1, 2020 19:30
Now spec2def uses the version annotations in spec files to generate a table, which will be used to dynamically patch the export table based on the current process' appcompat settings, when a dll is initialized, allowing to export the appropriate functions for each version.
TODO: create a static lib? Keep RosGetProcessCompatVersion() as inline?

[NTDLL] Retain the original ExportDirectory pointer

[NTDLL] Handle forwarders to private exports correctly
MSVC's LINK is not able to export both the undecorated and decorated symbol from the same dll. This results in an incoherence between the spec file entries and the actual exports, breaking the dll export versioning.
so that winhttp can use nt6+ apis
This allows to link our own applications to vista exports, which would usually not see the vista exports. The forwarder dll will be able to link to vista exports from advapi32 though, because it has a roscompat descriptor.
* Convert advapi32_vista into advapi32_vista_static library
* Link advapi32 to advapi32_vista_static and add exports from advapi32_vista
* Add new advapi32_vista.dll as a forwarder to advapi32.dll
* Add advapi_vista_addon dll, which can be renamed to advapi32_vista to support running on win2k3
This is required, because with dynamically patched export tables we will export all vista functions from the main dll, and you can link to them at compile time, but they will not be resolved at runtime, unless the importer has a roscompat-descriptor. Linking the vista version first will resolve the imports from the vista dll.
- Add SetProcessDEPPolicy stub
- Improve GetSystemTimePreciseAsFileTime stub to actually work
This also serves as a test for the InitOnceExecuteOnce() function.
This flag was disabled in an earlier commit,
and now I am removing the unused leftovers.
Also fixed a case typo that was causing CI to fail.
This was breaking the build on MSVC due to _WIN32_WINNT
value mismatches between different files.
These two functions do not exist in x64 builds
of msvcrt. (I checked with Wine to ensure that
we are not missing an implementation, and they
don't have those functions in x64 either.)

As such, the apiset stub DLLs are causing linker
errors because they lack the -i386 annotation in
their .spec files. This is now fixed.
This will be used later, to implement configure-time
disabling of NT6+ APIs.
Otherwise, there would be a mismatch in kernel32
due to symbols not implemented after NT5.2, and the
loader would assert.
This function is required to exist by the
loader subsystem, and is called directly
by NTDLL. If it is not omitted, then the
system will fail to boot properly.
This was different in api-ms-win-core-localization-l1-1-0.spec than
it was in kernel32.spec, which resulted in a linker error on the
MSVC CI.
@binarymaster binarymaster added the enhancement For PRs with an enhancement/new feature. label Sep 6, 2020
@binarymaster
Copy link
Member

since work on that PR seems to have stopped

I don't think so, but maybe it's better to ask in chat about such things...

@@ -5,7 +5,7 @@
@ stdcall -version=0x601+ FindNLSString() kernel32.FindNLSString
@ stdcall -version=0x601+ FindNLSStringEx() kernel32.FindNLSStringEx
@ stdcall -version=0x601+ GetACP() kernel32.GetACP
@ stdcall -version=0x601+ GetCPFileNameFromRegistry() kernel32.GetCPFileNameFromRegistry
@ stdcall -version=0x501-0x600 GetCPFileNameFromRegistry() kernel32.GetCPFileNameFromRegistry
Copy link
Contributor

Choose a reason for hiding this comment

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

Unwanted change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was intentional as well; see my commit comment on a4713a0 for detaials.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever the error was, that is a wrong fix, as apisets are all NT6+ at least.

@@ -32,7 +32,7 @@
@ stub -version=0x600+ BaseGenerateAppCompatData
@ stdcall BaseInitAppcompatCacheSupport()
@ stdcall BaseIsAppcompatInfrastructureDisabled() IsShimInfrastructureDisabled
@ stdcall -version=0x501-0x502 BaseProcessInitPostImport()
@ stdcall BaseProcessInitPostImport()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unwanted change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; see my commit comment for 84de3f2 for why this is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like a wrong fix as well:
either this function/export actually exists on NT6+ and this change should be a separate PR,
or it does not and there should be no change.

Copy link
Member

Choose a reason for hiding this comment

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

does not really matter, but reactos components should be able to bypass this mechanism so either way should work,
removing this version or leaving it in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should all be 0x600+, so that the loader can identify DLLs that have only Vista+ exports and fail when trying to load the DLL, emulating the non-existance of the DLL for apps that check this to see, whether it's running on Vista.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should all be 0x600+

Which 'they' are you referring to? Did you mean to comment about the apisets instead?
As changing BaseProcessInitPostImport to 0x600+ from 0x501-0x502 seems even more unexpected...

@tkreuzer
Copy link
Contributor

tkreuzer commented Sep 8, 2020

Instead of assuming it was abandoned, you could have asked. I am working on it.

@wjk
Copy link
Contributor Author

wjk commented Sep 27, 2020

Superseded by #3239. Please accept my apologies for my stepping on Timo's toes by opening this PR.

@wjk wjk closed this Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For PRs with an enhancement/new feature.
Projects
None yet
5 participants