Skip to content

fix: enable SeIncreaseBasePriorityPrivilege when setting High I/O priority - #65

Merged
PrimeBuild-pc merged 3 commits into
PrimeBuild-pc:mainfrom
SA-Mousavichashmi:fix/high-io-priority-privilege
Oct 12, 2026
Merged

PrimeBuild-pc merged 3 commits into
PrimeBuild-pc:mainfrom
SA-Mousavichashmi:fix/high-io-priority-privilege

Conversation

@SA-Mousavichashmi

Copy link
Copy Markdown
Contributor

Description

When configuring process I/O priority to High (ProcessIoPriority.High / level 3), Windows NT requires SeIncreaseBasePriorityPrivilege in the calling process's token.

Even though ThreadPilot runs elevated as administrator, Windows administrative user tokens have SeIncreaseBasePriorityPrivilege disabled by default until explicitly enabled via AdjustTokenPrivileges. Without enabling this privilege, NtSetInformationProcess(..., ProcessIoPriority, ...) fails with NTSTATUS 0xC0000061 (STATUS_PRIVILEGE_NOT_HELD). In ThreadPilot, this unmapped status previously produced a generic NativeApplyFailed error while leaving targeted processes stuck at default/throttled Low I/O priority.

Changes

  1. Privilege Adjustment P/Invoke:

    • Added standard Win32 token adjustment APIs (OpenProcessToken, LookupPrivilegeValueW, AdjustTokenPrivileges) to ProcessMemoryPriorityNativeMethods.
    • Added helper TryEnablePrivilege and exposed TryEnableProcessPrivilege on IProcessMemoryPriorityNativeApi.
  2. Acquire Privilege on High I/O Priority:

    • Updated ProcessMemoryPriorityService.SetIoPriorityAsync to invoke TryEnableProcessPrivilege(SeIncreaseBasePriorityPrivilege) before calling SetIoPriority when requesting ProcessIoPriority.High.
  3. Status Code Mapping:

    • Mapped 0xC0000061 (STATUS_PRIVILEGE_NOT_HELD) alongside 0xC0000022 (STATUS_ACCESS_DENIED) to AffinityApplyErrorCodes.AccessDenied and user-friendly access denied messaging.
  4. Unit Tests:

    • Added test coverage in ProcessMemoryPriorityServiceTests verifying that ProcessIoPriority.High attempts to acquire SeIncreaseBasePriorityPrivilege.
    • Added test verifying NTSTATUS 0xC0000061 maps safely to AccessDenied.

Verification

  • Verified P/Invoke privilege elevation and token adjustment behavior against Windows NT API specifications.
  • Verified unit test suite additions covering privilege request tracking and NTSTATUS 0xC0000061 error mapping.

@codecov-commenter

codecov-commenter commented Oct 8, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 80.53097% with 22 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...orms/Windows/ProcessMemoryPriorityNativeMethods.cs 78.64% 11 Missing and 11 partials ⚠️

📢 Thoughts on this report? Let us know!

@PrimeBuild-pc

Copy link
Copy Markdown
Owner

Thanks for the PR. Please scope SeIncreaseBasePriorityPrivilege to the NtSetInformationProcess operation. Capture PreviousState when calling AdjustTokenPrivileges and restore it in a finally block on both success and failure. Since this modifies the process-wide token, please also prevent overlapping callers from restoring the privilege while another call still needs it. Add a Windows integration test that verifies the original privilege state is restored. The existing fake-based tests do not exercise the native token path.

@SA-Mousavichashmi

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed and constructive review @PrimeBuild-pc! I've addressed all four points in commit 795db6f:

1. Scoped Privilege to NtSetInformationProcess

  • Replaced the open-ended privilege enablement with AcquirePrivilegeScope(string privilegeName) -> IDisposable? on IProcessMemoryPriorityNativeApi.
  • In ProcessMemoryPriorityService.SetIoPriorityAsync, High I/O priority acquires the scope right before calling SetIoPriority and safely disposes it inside a finally block on both success and error paths.

2. Capture and Restore PreviousState

  • Added an overloaded AdjustTokenPrivileges P/Invoke taking out TokenPrivileges previousState.
  • When the privilege scope is acquired, any non-empty previous state is captured.
  • When the scope is disposed, the original token state is restored via AdjustTokenPrivileges. If the privilege was already enabled prior to the call or unassigned, it safely handles those states without side effects.

3. Reference Counting for Overlapping Callers

  • Because token privileges affect the entire process, overlapping callers could prematurely revoke the privilege for other concurrent threads.
  • Introduced thread-safe reference counting (ActivePrivileges registry guarded by PrivilegeLock):
    • Overlapping calls requesting the same privilege increment the reference count and share the active scope.
    • The captured PreviousState is only restored when the reference count drops to zero (the last active caller disposes its scope).

4. Real Windows Integration Tests & Coverage

  • Added Win32 GetTokenInformation with TokenPrivileges (TOKEN_INFORMATION_CLASS = 3) to inspect actual process token privilege states.
  • Created ProcessMemoryPriorityNativeIntegrationTests.cs exercising real native token interactions:
    • Verifies that AcquirePrivilegeScope captures and restores previous token state on disposal.
    • Verifies nested / overlapping scopes maintain the privilege until the final scope is released.
    • Verifies invalid / null privilege names gracefully return null without throwing.
    • Exercises the fallback non-Windows / default interface paths.
  • Updated FakeProcessMemoryPriorityNativeApi and unit tests in ProcessMemoryPriorityServiceTests.cs to test scope acquisition and disposal across both success and failure (0xC0000061) execution paths.

Verification & Test Results

  • 768 / 768 tests passing (dotnet test -c Release).
  • Line coverage on ProcessMemoryPriorityNativeMethods.cs increased to 88.1% (nested classes PrivilegeState and ScopedPrivilege at 100% and 91.7%), resolving the Codecov gap on native token paths.

@PrimeBuild-pc

Copy link
Copy Markdown
Owner

The new native integration coverage does not yet verify concurrent callers. Tests/ThreadPilot.Core.Tests/ProcessMemoryPriorityNativeIntegrationTests.cs:55-87 only nests two scopes on one thread and disposes the inner scope before the outer scope; it cannot catch a race between independent threads acquiring and disposing the same privilege. Please add a Windows test that starts two concurrent scopes (with a barrier so both are active), disposes one while the other remains active, and asserts the privilege stays enabled until the final disposal, then confirms the original state. The implementation's reference counting may be correct, but this required concurrency behavior is currently untested.

@SA-Mousavichashmi

Copy link
Copy Markdown
Contributor Author

Good catch @PrimeBuild-pc! You're completely right that nested scopes on a single thread don't exercise inter-thread concurrency and potential race conditions between independent callers.

I've added the requested concurrent multi-threaded integration test in commit df5cf5d (AcquirePrivilegeScope_WithConcurrentThreads_MaintainsPrivilegeUntilFinalDisposal in ProcessMemoryPriorityNativeIntegrationTests.cs):

  1. Synchronized Overlap via Barrier: Starts two independent worker threads (thread1 and thread2) that each acquire an AcquirePrivilegeScope(privilegeName). A Barrier(2) synchronizes them so both scopes are confirmed active concurrently.
  2. Early Disposal on Thread 1: thread1 disposes its scope and signals a ManualResetEventSlim.
  3. Assertion of Continued Privilege on Thread 2: While thread2 still holds its scope, it awaits thread 1's disposal signal and asserts that the process privilege remains enabled despite thread 1 having fully disposed its scope.
  4. Final Scope Disposal & Restoration Verification: thread2 disposes its scope. After both threads join, the test confirms that the token privilege has been cleanly restored to its exact pre-test state (originalEnabled == finalEnabled).

All 769 tests are passing locally on Windows (dotnet test -c Release).

@PrimeBuild-pc PrimeBuild-pc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Verified df5cf5d against the requested changes. The new Windows integration test uses two independent threads and a barrier, releases the first scope while the second is still active, checks that the privilege remains enabled, and verifies the original state after the final disposal. The native implementation captures PreviousState and protects shared scope reference counts with a lock; the service disposes the scope in finally on success and failure. Ran the full Release test suite on Windows: 769 passed, 0 failed. SeShutdownPrivilege is present and initially disabled on this machine, so the concurrency test exercises actual token changes. CI build/test/security checks and CodeQL are also green. The local token does not contain SeIncreaseBasePriorityPrivilege, so a live elevated High-I/O operation was not exercised locally. Thank you for addressing the review.

@PrimeBuild-pc
PrimeBuild-pc merged commit 70b7f6d into PrimeBuild-pc:main Oct 12, 2026
3 checks passed
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.

3 participants