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.

Sign up for GitHub

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

Jump to bottom

x/sys/windows/svc/mgr: Service.Control() should return valid svc.Status on certain errors #59015

Closed
craig65535 opened this issue Mar 13, 2023 · 6 comments
Closed

x/sys/windows/svc/mgr: Service.Control() should return valid svc.Status on certain errors #59015

craig65535 opened this issue Mar 13, 2023 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Unreleased

Comments

@craig65535
Copy link

What version of Go are you using (go version)?

$ go version
1.19.5

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

windows/amd64

What did you do?

On a given *mgr.Service (s), in a state like StopPending that does not accept controls, try:

status, err := s.Control(svc.Stop)

What did you expect to see?

err should be windows.ERROR_INVALID_SERVICE_CONTROL, and status should be filled in with the service's current state and set of accepted commands. This is the behaviour described for ControlService:

[out] lpServiceStatus

A pointer to a SERVICE_STATUS structure that receives the latest service status information. The information returned reflects the most recent status that the service reported to the service control manager.

The service control manager fills in the structure only when GetLastError returns one of the following error codes: NO_ERROR, ERROR_INVALID_SERVICE_CONTROL, ERROR_SERVICE_CANNOT_ACCEPT_CTRL, or ERROR_SERVICE_NOT_ACTIVE. Otherwise, the structure is not filled in.

This behaviour is useful because checking the service state independently of controlling it is a TOCTOU bug.

What did you see instead?

err is correctly set to windows.ERROR_INVALID_SERVICE_CONTROL, but status is unset.
@gopherbot gopherbot added this to the Unreleased milestone Mar 13, 2023
@craig65535 craig65535 changed the title x/sys/windows/svc: Control() should return valid svc.Status on certain errors x/sys/windows/svc: Service.Control() should return valid svc.Status on certain errors Mar 13, 2023
@craig65535
Copy link
Author

craig65535 commented Mar 13, 2023 •
edited

I am able to work around this by calling windows.ControlService() directly:

var t windows.SERVICE_STATUS
err := windows.ControlService(s.Handle, windows.SERVICE_CONTROL_STOP, &t)

This has the expected behaviour and fills in t when it returns windows.ERROR_INVALID_SERVICE_CONTROL. However, it seems like this should also be in the mgr package.

@craig65535 craig65535 changed the title x/sys/windows/svc: Service.Control() should return valid svc.Status on certain errors x/sys/windows/svc/mgr: Service.Control() should return valid svc.Status on certain errors Mar 13, 2023
@cherrymui cherrymui added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. OS-Windows labels Mar 17, 2023
@cherrymui
Copy link
Member

cc @golang/windows

@alexbrainman
Copy link
Member

Thank you @craig65535 for filling the issue.

I agree there is a bug in this code.

I suggest we update returned status parameter when error is one of the following error codes: NO_ERROR, ERROR_INVALID_SERVICE_CONTROL, ERROR_SERVICE_CANNOT_ACCEPT_CTRL, or ERROR_SERVICE_NOT_ACTIVE. As per Microsoft documentation.

Would you like to contribute the fix as per https://go.dev/doc/contribute ? Maybe even add new test for it.

Alex

@craig65535
Copy link
Author

@alexbrainman Sorry for the delayed response. I'll look into contributing.

@craig65535 craig65535 mentioned this issue Apr 15, 2023
Closed
@craig65535
Copy link
Author

golang/sys#156

@gopherbot
Copy link

Change https://go.dev/cl/484895 mentions this issue: windows/svc/mgr: Service.Control: populate Status when returning certain errors

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Apr 25, 2023
@golang golang locked and limited conversation to collaborators Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Unreleased
5 participants
@dmitshur @craig65535 @gopherbot @alexbrainman @cherrymui