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

Ignore access denied on HcsTerminateProcess #1252

Merged

Conversation

gabriel-samfira
Copy link
Contributor

When calling HcsTerminateProcess on a process that has exited, but we still have open handles to, an ERROR_ACCESS_DENIED may be returned.

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminateprocess#remarks

Fixes: #1251

@gabriel-samfira gabriel-samfira requested a review from a team as a code owner December 22, 2021 12:00
@gabriel-samfira gabriel-samfira force-pushed the ignore-access-denied-on-kill branch 2 times, most recently from 0bd84c5 to 51ed656 Compare December 22, 2021 13:24
@helsaawy
Copy link
Contributor

This looks good to me; though I am not sure why TestPseudoConsolePowershell is timing out.
Can you try revendoring tests and push again?
@dcantah, thoughts?

@helsaawy helsaawy self-assigned this Dec 22, 2021
@gabriel-samfira
Copy link
Contributor Author

This looks good to me; though I am not sure why TestPseudoConsolePowershell is timing out. Can you try revendoring tests and push again? @dcantah, thoughts?

That one is addressed by: #1253

@dcantah
Copy link
Contributor

dcantah commented Dec 22, 2021

@helsaawy My assumption for how long powershell should take to launch and execute one command was off I guess hahah #1253 Here's a "fix"

Edit: Nevermind, test probably failing for a different reason

@dcantah
Copy link
Contributor

dcantah commented Dec 22, 2021

Oop @gabriel-samfira beat me to it

@anmaxvl
Copy link
Contributor

anmaxvl commented Dec 22, 2021

make sure to cd test && go mod vendor and repush.

When calling HcsTerminateProcess on a process that has exited, but we
still have open handles to, an ERROR_ACCESS_DENIED may be returned.

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminateprocess#remarks

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Copy link
Contributor Author

make sure to cd test && go mod vendor and repush.

done! Thanks!

@dcantah dcantah merged commit 58caeed into microsoft:master Dec 29, 2021
dcantah pushed a commit to dcantah/hcsshim that referenced this pull request Jan 4, 2022
When calling HcsTerminateProcess on a process that has exited, but we
still have open handles to, an ERROR_ACCESS_DENIED may be returned.

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminateprocess#remarks

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
(cherry picked from commit 58caeed)
Signed-off-by: Daniel Canter <dcanter@microsoft.com>
dcantah added a commit that referenced this pull request Jan 4, 2022
When calling HcsTerminateProcess on a process that has exited, but we
still have open handles to, an ERROR_ACCESS_DENIED may be returned.

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminateprocess#remarks

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
(cherry picked from commit 58caeed)
Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@AbelHu
Copy link

AbelHu commented Mar 1, 2022

@dcantah @jsturtevant do you know whether we can pick this fix in docker?

@gabriel-samfira
Copy link
Contributor Author

@dcantah @jsturtevant do you know whether we can pick this fix in docker?

Rather than this PR, I suggest looking at: #1275

Actually, I would suggest to have a look at all the fixes merged in 0.9.2: https://github.com/microsoft/hcsshim/releases/tag/v0.9.2

if those fixes are not currently present in docker.

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.

Sending a second Kill() returns "Access is denied."
5 participants