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

[RTL][NTDLL_APITEST] Implement RtlRemovePrivileges #4614

Merged
merged 2 commits into from Oct 5, 2022

Conversation

RatinCN
Copy link
Contributor

@RatinCN RatinCN commented Aug 19, 2022

Implement RtlRemovePrivileges

sdk/include/ndk/rtlfuncs.h Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
@HBelusca HBelusca added the NT6+ For PRs that aim at implementing NT6+ functionality. label Aug 19, 2022
@binarymaster binarymaster added the enhancement For PRs with an enhancement/new feature. label Aug 21, 2022
@binarymaster binarymaster added this to New PRs in ReactOS PRs via automation Aug 21, 2022
@binarymaster binarymaster moved this from New PRs to WIP / Waiting on contributor in ReactOS PRs Aug 21, 2022
@HBelusca
Copy link
Contributor

Ah, and since this is also a new function, please write an apitest for it in order to document and test its functioning, following the existing examples of
https://git.reactos.org/?p=reactos.git;a=tree;f=modules/rostests/apitests/ntdll;h=1b71170096ae03425f27561db49de82d05391f4d;hb=HEAD

@HBelusca HBelusca added the needs tests The proposed changes should be supported by tests to ensure their correctness. label Aug 21, 2022
@GeoB99 GeoB99 self-requested a review August 21, 2022 15:41
sdk/include/ndk/rtlfuncs.h Outdated Show resolved Hide resolved
IN ULONG PrivilegeCount)
{
DPRINT("RtlRemovePrivileges(%p, %p, %u)\n", TokenHandle, PrivilegesToKeep, PrivilegeCount);

Copy link
Member

Choose a reason for hiding this comment

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

You wouldn't want your RtlRemovePrivileges to explode in your face if the current IRQL is inappropriate for the function to do its work. Use PAGED_CODE_RTL().

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 RTL function exported from ntdll.dll in user-mode, in this case, PAGED_CODE_RTL() is not necessary?

Copy link
Member

Choose a reason for hiding this comment

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

There can be NTDLL functions that can be used in kernel mode on-demand. Is this RtlRemovePrivileges strictly used for user mode system components or can it have its usage in kernel mode too? If the former then you might not need PAGED_CODE_RTL(), otherwise you have to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, RtlRemovePrivileges is exported in user-mode only (in Windows), not in kernel-mode, like RtlAdjustPrivilege but new for NT 6.0+

Copy link
Contributor

Choose a reason for hiding this comment

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

@GeoB99 : Normally the RTL should be compiled twice: once for the kernel (making it use possible kernel-specific defines), and once for user-mode (for ntdll). We don't do that yet, and as a result we may end up (in non-optimized builds) with user-mode-only functions (like this one) being added in the kernel but being never used.

sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/include/ndk/rtlfuncs.h Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the ROSTESTS Label for ROS testcases PRs. label Aug 30, 2022
@RatinCN RatinCN requested review from GeoB99 and HBelusca and removed request for learn-more, HeisSpiter, ThFabba and GeoB99 August 30, 2022 17:19
@RatinCN
Copy link
Contributor Author

RatinCN commented Sep 4, 2022

Ok to go? @HBelusca @GeoB99 @tkreuzer @ThFabba

@binarymaster binarymaster changed the title [RTL] Implement RtlRemovePrivileges [RTL][NTDLL_APITEST] Implement RtlRemovePrivileges Sep 13, 2022
@binarymaster binarymaster removed the needs tests The proposed changes should be supported by tests to ensure their correctness. label Sep 13, 2022
@RatinCN
Copy link
Contributor Author

RatinCN commented Sep 26, 2022

Screenshot
ntdll.dll and ntdll_apitest.exe complied without NTDDK_VERSION (temporarily removed to run this test) and run on ReactOS. In formal build (NTDDK_VERSION lower than Vista), this function will not involved and api test will skipped.

sdk/include/ndk/rtlfuncs.h Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
IN ULONG PrivilegeCount)
{
DPRINT("RtlRemovePrivileges(%p, %p, %u)\n", TokenHandle, PrivilegesToKeep, PrivilegeCount);

Copy link
Contributor

Choose a reason for hiding this comment

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

@GeoB99 : Normally the RTL should be compiled twice: once for the kernel (making it use possible kernel-specific defines), and once for user-mode (for ntdll). We don't do that yet, and as a result we may end up (in non-optimized builds) with user-mode-only functions (like this one) being added in the kernel but being never used.

sdk/lib/rtl/priv.c Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
sdk/lib/rtl/priv.c Show resolved Hide resolved
sdk/lib/rtl/priv.c Show resolved Hide resolved
sdk/lib/rtl/priv.c Show resolved Hide resolved
@HBelusca
Copy link
Contributor

ntdll.dll and ntdll_apitest.exe complied without NTDDK_VERSION (temporarily removed to run this test) and run on ReactOS. In formal build (NTDDK_VERSION lower than Vista), this function will not involved and api test will skipped.

Does this test pass OK on Windows 7+ ?

@RatinCN
Copy link
Contributor Author

RatinCN commented Sep 28, 2022

ntdll.dll and ntdll_apitest.exe complied without NTDDK_VERSION (temporarily removed to run this test) and run on ReactOS. In formal build (NTDDK_VERSION lower than Vista), this function will not involved and api test will skipped.

Does this test pass OK on Windows 7+ ?

Yes, this apitest also passed in my Win7 and Win11:
Screenshot2
Screenshot1

sdk/lib/rtl/priv.c Show resolved Hide resolved
sdk/lib/rtl/priv.c Outdated Show resolved Hide resolved
@HBelusca HBelusca requested a review from ThFabba October 1, 2022 13:20
@HBelusca
Copy link
Contributor

HBelusca commented Oct 4, 2022

Please ensure that
image
is enabled in the PRs so that if there are (for example) last-time minor formatting/typos suggestions to be applied to it, they can be applied by the person who will commit the PR into master, instead of waiting for PR author / making a separate branch and then manually closing the PR.

@RatinCN
Copy link
Contributor Author

RatinCN commented Oct 5, 2022

Please ensure that image is enabled in the PRs so that if there are (for example) last-time minor formatting/typos suggestions to be applied to it, they can be applied by the person who will commit the PR into master, instead of waiting for PR author / making a separate branch and then manually closing the PR.

Done
image

sdk/lib/rtl/priv.c Show resolved Hide resolved
@HBelusca HBelusca merged commit badd970 into reactos:master Oct 5, 2022
ReactOS PRs automation moved this from WIP / Waiting on contributor to Done Oct 5, 2022
@RatinCN RatinCN deleted the Impl_RtlRemovePrivileges branch October 7, 2022 07:53
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. NT6+ For PRs that aim at implementing NT6+ functionality. ROSTESTS Label for ROS testcases PRs.
Projects
ReactOS PRs
  
Done
6 participants