-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Update ntldr.h #2547
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
base: master
Are you sure you want to change the base?
Update ntldr.h #2547
Conversation
I did not invent any of these names. Everything marked with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not invent any of these names. Everything marked with // private should match the official ones 🤷
I'm choosing to approve the PR since the changes seem functionally fine to me and I don't think the PR should be blocked just for naming disagreements.
That said, whether in this PR or as a follow up - I do think we should go back and clean up the parameter naming using reasonable judgement for consistently and clarity. IMO some of the naming is regressing a bit in this PR. I do not believe that Microsoft got the parameter naming right to begin with, parameter names (and to some degree types) are less set in stone compared to structure names/parameters - phnt is a good place (using our best judgement) to clean this up.
I think the official parameter names define the ground truth and we shouldn't deviate much from them. Yes, it's clear that Microsoft haven't established a consistent naming convention, and yes, we have a good position to fix big blunders. But doing renames like If I understand it correctly, the main concern is that some names might be confusing or misleading. The good place to resolve this confusion is in the docstring comments that @dmex introduces throughout phnt. If anything, that's the place where such comments can provide the most value. I can also join the effort and add docstring comments explaining parameter use for the functions changed in this PR, if it helps. |
phlib/mapldr.c
Outdated
|
||
__try | ||
{ | ||
status = LdrFindResource_U(DllBase, &resourceInfo, RESOURCE_DATA_LEVEL, &resourceData); | ||
status = LdrFindResource_U(DllBase, resourceIdPath, RTL_NUMBER_OF(resourceIdPath), &resourceData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these can be in an arbitrary order? There is an explicit leveling to locating a resource. So having the LDR_RESOURCE_INFO
be explicit seems most convenient to me. I'd also like information on where/why the name LDR_EXTERNAL_DLL_PATH
was found/chosen. The "external" seems like an odd choice, the resource could be internal to my own (and even current) module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll restore it back to when there was a helper union. As for LDR_EXTERNAL_DLL_PATH
, see #2547 (comment)
_Out_opt_ PVOID* Address, | ||
_Out_opt_ PULONG Size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that this leaked in NT5, but I find this to be a regression in clarity. 🤷
phnt/include/ntldr.h
Outdated
_In_ PLDR_RESOURCE_INFO ResourceInfo, | ||
_In_ ULONG Level, | ||
_Out_ PIMAGE_RESOURCE_DATA_ENTRY *ResourceDataEntry | ||
_In_reads_(ResourceIdPathLength) LDR_EXTERNAL_DLL_PATH ResourceIdPath[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really want to eliminate LDR_RESOURCE_INFO
. I'd change this to just be PULONG_PTR
and leave out LDR_EXTERNAL_DLL_PATH
.
This is an example of lack of clarity that I believe should remain as it was. As I understand it (correct me if I'm wrong) there is an explicit indexing to locating a resource. The ambiguity makes it easy to make a mistake ordering the items in the array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, each index has a separate meaning. I initially used PULONG_PTR
but kept a helper type (with some adjustments to make it a union), but I got the impression that dmex didn't like the new type. I'll remove LDR_EXTERNAL_DLL_PATH
in favor or PULONG_PTR
and add back the helper type
This reverts commit f9c52f7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates function prototypes to use official parameter names and types, while removing obsolete, unmatched functions from ntldr. Key changes include:
- Updating LdrInitializeThunk‘s parameter from "Parameter" to "NtdllBaseAddress" in ntrtl.h.
- Removing explicit casts for resource identifiers and updating function pointer types in phlib/mapldr.c.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
phnt/include/ntrtl.h | Updated prototype in LdrInitializeThunk to use official parameter name |
phlib/mapldr.c | Removed unnecessary casts for resourceInfo and updated function pointer types (from PLDR_INIT_ROUTINE to PDLL_INIT_ROUTINE) |
Comments suppressed due to low confidence (6)
phnt/include/ntrtl.h:9443
- Ensure that the new parameter name 'NtdllBaseAddress' accurately reflects the argument's intended usage and that all corresponding internal references are updated accordingly.
_In_ PVOID NtdllBaseAddress
phlib/mapldr.c:690
- Confirm that removing the cast to (ULONG_PTR) for resourceInfo.Type maintains the intended type safety and correct behavior.
resourceInfo.Type = Type;
phlib/mapldr.c:691
- Verify that the removal of the cast for resourceInfo.Name does not adversely affect the mapping logic or type correctness.
resourceInfo.Name = Name;
phlib/mapldr.c:1191
- Verify that using PDLL_INIT_ROUTINE instead of PLDR_INIT_ROUTINE for ImageEntryPoint aligns with the official definitions and intended function pointer behavior.
_Out_ PDLL_INIT_ROUTINE *ImageEntryPoint
phlib/mapldr.c:2645
- Confirm that replacing PLDR_INIT_ROUTINE with PDLL_INIT_ROUTINE in PhLoaderEntryUnloadDll aligns with the expected loader behavior.
PDLL_INIT_ROUTINE imageEntryRoutine;
phlib/mapldr.c:2721
- Ensure that the updated function pointer type PDLL_INIT_ROUTINE for imageEntryRoutine in PhLoadPluginImage is consistent with its usage elsewhere in the loader.
PDLL_INIT_ROUTINE imageEntryRoutine;
This pull request adds some missing types, updates function prototypes to use official parameter names, and adjusts minimal import versions in ntldr. It also removes several functions that don't have matching ntdll exports, namely
LdrRelocateImage
,LdrRelocateImageWithBias
,LdrVerifyMappedImageMatchesChecksum
, andLdrDllRedirectionCallback
.Additionally, I'm not sure what to do with
RtlpScpCfgNtdllExports
. It looks like we already have the official types for it (RTL_SCP_CFG_NTDLL_EXPORTS
and co.) inntmmapi.h
which can replace the reversed version of the same typeRTL_SCPCFG_NTDLL_EXPORTS
fromntldr.h
. I don't know if you want to move the function tontmmapi.h
(closer to the types, so it can use them) or move both intontrtl.h
, or something else.