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

timer-resolution-control: Force limits immediately on mod load and setting change, restore original resolution on mod unload #18

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

levitation
Copy link

timer-resolution-control: During mod start, force timer resolution limit if the resolution was already changed by the program. During settings change, enforce the new settings immediately by enforcing new limits or restoring the resolution desired earlier by the program. On mod unload restore the resolution desired by the program.

…mit if the resolution was already changed by the program. During settings change, enforce the new settings immediately by enforcing new limits or restoring the resolution desired earlier by the program. On mod unload restore the resolution desired by the program.
…e program by the time the mod loads. This is interesting to know in case the timer resolution is still higher than the usual 15.6ms. I have observed this happening in one of my machines. Minor formatting consistency adjustments.
Copy link
Owner

@m417z m417z left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Looks good overall, well done.

sure the changes apply, you might want to restart the target program(s) or restart the computer.
The changes will apply immediately when the mod loads or when settings are changed.

Authors: m417z, levitation
Copy link
Owner

Choose a reason for hiding this comment

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

No need to list me again, and IMO "contributors" fits better here.

Suggested change
Authors: m417z, levitation
Contributors: [levitation](https://github.com/levitation).

if (status == STATUS_TIMER_RESOLUTION_NOT_SET) {
Wh_Log(L"NtSetTimerResolution has not been called by the program");
status = STATUS_SUCCESS;
CurrentResolution = g_lastDesiredResolution; //Take the value obtained from NtQueryTimerResolution, which does not fail with STATUS_TIMER_RESOLUTION_NOT_SET. Even if the program has not set the resolution, I have observed that the system itself may have set the resolution on program start to 1ms for some reason.
Copy link
Owner

Choose a reason for hiding this comment

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

If timer resolution is not set, then I assume that it's controlled by an external factor and can change. In this case, if we get CurrentResolution > limitResolution here, we won't impose a limit, but the resolution might change in the future in a way we won't detect.

I don't think there's a good solution for this, only polling, but it's a bit of an overkill.

Alternatively, we can add a new configuration, "default resolution", and set max(default, limit) explicitly in case the resolution isn't set. It will be fairly intrusive, though, as it will affect all processes.

All in all, I think it's fine to leave it as is, just bringing up some points to think about.

(double)CurrentResolution / 10000.0);
g_minimumResolution = MinimumResolution;
g_maximumResolution = MaximumResolution;
g_lastDesiredResolution = CurrentResolution; //TODO: During mod load there is a small race condition where the hook is not yet set, the g_lastDesiredResolution is already set, and then the program tries to set a new desired resolution which will not be saved into g_lastDesiredResolution.
Copy link
Owner

Choose a reason for hiding this comment

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

I think that calling it g_resolutionToRestore instead of g_lastDesiredResolution makes more sense and makes it easier to understand its purpose.

Also, perhaps it's better to have it represent a more precise state to restore: a resolution, nothing (if no limit was imposed), or unset (see my other comment). WDYT?

{
ULONG CurrentResolution;
NTSTATUS status = pOriginalNtSetTimerResolution(0, FALSE, &CurrentResolution);
if (status == STATUS_TIMER_RESOLUTION_NOT_SET) {
Copy link
Owner

Choose a reason for hiding this comment

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

If STATUS_TIMER_RESOLUTION_NOT_SET is returned, how about restoring the unset state on unload? According to the ExSetTimerResolution documentation and to ReactOS, it can be done by setting a zero resolution, but I didn't test it.

LoadSettings();

Wh_SetFunctionHook((void*)pNtSetTimerResolution, (void*)NtSetTimerResolutionHook, (void**)&pOriginalNtSetTimerResolution);

return TRUE;
}

void Wh_ModSettingsChanged(void)
void EnforceLimits()
Copy link
Owner

Choose a reason for hiding this comment

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

This function is incompatible with the block config. Which also has a small bug I just discovered: by setting g_limitResolution = g_minimumResolution;, setting the resolution isn't completely blocked, g_minimumResolution can still be set.

ULONG lastDesiredResolution = g_lastDesiredResolution;
Wh_Log(L"Restoring desired resolution: %f milliseconds", (double)lastDesiredResolution / 10000.0);
ULONG CurrentResolution;
pOriginalNtSetTimerResolution(g_lastDesiredResolution, TRUE, &CurrentResolution);
Copy link
Owner

Choose a reason for hiding this comment

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

Using pOriginalNtSetTimerResolution here is unsupported since the hooks are already removed. It's a tricky one, it coincidentally works when the mod is disabled, but it will likely crash when you exit Windhawk. Just use NtSetTimerResolution instead.

@m417z m417z force-pushed the main branch 2 times, most recently from 4fdb2f8 to 84a1de6 Compare September 9, 2024 19:21
@m417z m417z force-pushed the main branch 2 times, most recently from a75c03f to 37e50ae Compare October 4, 2024 18:33
@m417z m417z force-pushed the main branch 3 times, most recently from 56b4996 to 3b51119 Compare October 17, 2024 07:50
@m417z m417z force-pushed the main branch 3 times, most recently from 759c9df to c3d60eb Compare October 31, 2024 00:11
@m417z m417z force-pushed the main branch 3 times, most recently from a22aae5 to 3e45764 Compare November 22, 2024 00:42
@m417z m417z force-pushed the main branch 2 times, most recently from 42a6318 to ff6a20e Compare December 12, 2024 15:37
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.

2 participants