x/sys/windows/svc: session change notification triggers "checkptr: pointer arithmetic result points to invalid allocation" #59687
Comments
|
CC @golang/windows |
ianlancetaylor
added
OS-Windows
NeedsInvestigation
|
Thanks for reporting this issue @elmeyer. The $ go vet ./windows/svc
# golang.org/x/sys/windows/svc
.\service.go:204:18: possible misuse of unsafe.Pointer
I'm not sure why the code is doing that, but I'll prefer to keep doing it, doesn't hurt and when it was added (in CL 330010 it fixed a couple of TODOs. What we should fix is the invalid pointer conversion. It seems like a good case to use a cgo.Handle, like this: func ctlHandler(ctl, evtype, evdata, context uintptr) uintptr {
s := cgo.Handle(context).Value().(service) // <-- CHANGE
e := ctlEvent{cmd: Cmd(ctl), eventType: uint32(evtype), eventData: evdata, context: 123456} // Set context to 123456 to test issue #25660.
s.c <- e
return 0
}
var theService service // This is, unfortunately, a global, which means only one service per process.
// serviceMain is the entry point called by the service manager, registered earlier by
// the call to StartServiceCtrlDispatcher.
func serviceMain(argc uint32, argv **uint16) uintptr {
handle, err := windows.RegisterServiceCtrlHandlerEx(
windows.StringToUTF16Ptr(theService.name),
ctlHandlerCallback,
uintptr(cgo.NewHandle(theService)), // <-- CHANGE
)
...
}@elmeyer if you agree with my solution, are you up to sending a CL to the |
|
@qmuntal Thanks for your response. This does seem like the best way to go short of removing the |
qmuntal
added
NeedsFix
This fixes go vet complaints and checkptr crashes when the handler receives an event, e.g. a session change notification. Fixes golang/go#59687
|
Apologies for asking, but this is my first contribution on behalf of my company and I'd just like to check whether something went wrong here. I chose to make a pull request for the changes as per https://go.dev/doc/contribute#sending_a_change_github and signed the CLA on behalf of my company. However, as my title does not authorize me to do so according to https://opensource.google.com/docs/cla/policy/#titles-that-confer-authority, the CLA was revoked and I asked our CTO to sign it again. The CLA check in the PR succeeded when I first signed it, however, and I am now worried that something got stuck along the way - can someone check? Thanks in advance! |
Not a CLA expert, but it seems that the PR CLA check is happy. Gerrit bot hasn't ported the PR to Gerrit yet, it could be because the PR only contains a commit without changes. Could you make sure that you have pushed all your local changes? |
|
Oh dear, that’s embarrassing. Thanks for catching that! |
This fixes go vet complaints and checkptr crashes when the handler receives an event, e.g. a session change notification. Fixes golang/go#59687
|
Change https://go.dev/cl/490135 mentions this issue: |
I just tried to cross compile So we cannot import Alex |
|
Unrelated to this issue, but I just run @ianlancetaylor should Thank you. Alex |
Strange, I see no errors when running |
Let's compare our Go environments. Here is mine: Alex |
And here is mine: $ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\***\AppData\Local\go-build
set GOENV=C:\Users\***\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\***\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\***\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\***\code\sys\go.mod
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\***\AppData\Local\Temp\go-build1908209761=/tmp/go-build -gno-record-gcc-switchesAlso tried with gotip: $ gotip version
go version devel go1.21-4b66502dda Sun Apr 30 00:23:53 2023 +0000 windows/amd64On the other hand, I can reproduce the error when running with $ set CGO_ENABLED=0
$ gotip test -c -a -o /dev/null
# golang.org/x/sys/windows/svc.test
runtime.cgo_yield: relocation target _cgo_yield not defined
_cgo_init: relocation target x_cgo_init not defined
_cgo_thread_start: relocation target x_cgo_thread_start not defined
_cgo_notify_runtime_init_done: relocation target x_cgo_notify_runtime_init_done not defined
_cgo_pthread_key_created: relocation target x_cgo_pthread_key_created not defined
_cgo_bindm: relocation target x_cgo_bindm not defined
_cgo_getstackbound: relocation target x_cgo_getstackbound not defined
_crosscall2_ptr: relocation target x_crosscall2_ptr not defined |
|
@qmuntal I run my command on Linux (see #59687 (comment) for details). You run the command on Windows. Try running the command on any non-Windows OS. Alex |
|
@alexbrainman I edited my last comment with some new info. It fails when running without CGO. It is a pity that |
Why do we need sync.Map here? I don't see any race reported. The problem is with unsafe conversion of pointers. I also run this command: There are many other similar errors in Thank you. Alex |
Most of the In contrast, the run-time |
Thanks for explaining.
I don't have anything to add on #58625. I don't use Windows much nowadays.
I assume you refer to the Alex |
|
Hi, sorry for bumping, but what's the status on this issue? Are there any changes necessary here? Should I attempt to replace the use of |
|
I would follow @bcmills suggestion in https://go-review.googlesource.com/c/sys/+/490135/comment/ab3aee9e_f96b96aa/:
|
FWIW, this is #64663. |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
I am running the Windows service example.
I have forked the
sysrepository and addedsvc.AcceptSessionChangeto the accepted service commands. (I have also redirectedstderr/stdoutto a fileexample.lognext to the executable to be able to catch the crash output.)https://github.com/elmeyer/sys/tree/windows-svc-example-checkptr
What did you expect to see?
No errors when running the service example built with
-race.What did you see instead?
When logging out and logging back in, a session change notification is triggered. The resulting call to
svc.ctlHandlerreceives auintptrtosvc.theServicein thecontextparameter. Its conversion back to a*svc.serviceseems to be what triggers this crash.example.logI fundamentally don't understand why a pointer to
svc.theServiceis being passed towindows.RegisterServiceCtrlHandlerEx. The comment forsvc.theServiceexplicitly states:while the documentation for
RegisterServiceCtrlHandlerExWstates:(emphasis mine)
The crash naturally disappears when this parameter is replaced with 0 and
svc.ctlHandleris modified to assume that all calls to it refer tosvc.theService. However, the root cause of thischeckptrfailure remains unclear.cc @ericrange who helped reproduce this problem.
The text was updated successfully, but these errors were encountered: