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

Speed up clink injection by not suspending threads #507

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jalfd
Copy link

@jalfd jalfd commented Jan 14, 2019

Iterating over every thread on the system takes a lot of time, and is unnecessary as cmd.exe is already blocked (waiting in WaitForSingleObject()) whenever clink is being injected. So just don't do it.

Also add a few static_cast's to silence warnings in VS2017 about narrowing conversions.

Jesper Alf Dam added 2 commits January 11, 2019 13:36
cmd.exe is a singlethreaded application. When it is waiting for another
process (such as clink) to finish, it is blocked in
WaitForSingleObject().

That means there's no need to suspend its threads before injecting.

This is useful as enumerating all the threads on the machine frequently
takes around a third of a second, and this cost is incurred even if cmd
is started with /c, meaning it slows down script execution in
non-interactive shells as well.
@chrisant996
Copy link

is unnecessary as cmd.exe is already blocked (waiting in WaitForSingleObject()) whenever clink is being injected

That's only true when clink is being injected into the same cmd.exe that spawned the clink inject command. The --pid flag can inject into any cmd.exe. Also it can inject into a grandparent cmd.exe -- for example cmd.exe spawns devenv.exe and then devenv.exe spawns clink_x64.exe inject and then you run dir /s c:\ in the grandparent cmd.exe process: now hit F5 in the debugger to start the injection. Cmd.exe is not already blocked waiting.

This change sounds like it may accidentally remove support for less common ways that clink can be injected. Perhaps there could be a flag so that it's not the default way, but can be forced if the user is willing to take the risk.

@jalfd
Copy link
Author

jalfd commented Nov 24, 2020

is unnecessary as cmd.exe is already blocked (waiting in WaitForSingleObject()) whenever clink is being injected

That's only true when clink is being injected into the same cmd.exe that spawned the clink inject command. The --pid flag can inject into any cmd.exe. Also it can inject into a grandparent cmd.exe -- for example cmd.exe spawns devenv.exe and then devenv.exe spawns clink_x64.exe inject and then you run dir /s c:\ in the grandparent cmd.exe process: now hit F5 in the debugger to start the injection. Cmd.exe is not already blocked waiting.

This change sounds like it may accidentally remove support for less common ways that clink can be injected. Perhaps there could be a flag so that it's not the default way, but can be forced if the user is willing to take the risk.

That's true. I hadn't considered that use case (because I don't use it). Thanks for spotting that!

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