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
syscall: ntdll.dll errors in rtlGetNtVersionNumbers via os.StartProcess #49731
Comments
Given the failure mode here I suspect some kind of race. No idea beyond that. 🤷♂️ |
I have no idea, the dll is loaded but the basic function is not found. The only reasonable cause for this would be that some other dll was found under this name, but it's a system dll, so that also seems improbable. I don't know of any reasonable event in the OS that would do this to a program, so I feel it has to be internal. I could imagine something getting garbage collected that messes up the call to fetch the function pointer, but I can't see anything obvious in the code - or something messing with the syscall structure of the m, but I can't see anything that doesn't first lockOsThread that touches that. |
Another one, same symbol but different error:
|
Broadening the search, looks like it's still just those two, and both very recently. (This looks like a 1.18 regression to me.)
2021-11-22T21:52:20-100d7ea/windows-amd64-longtest |
I did run a test over night to try to provoke this on 32bit+64bit amd and arm64, it would not happen (surprise). I'll try to see if I can provoke a gomote, so far it has only happened on amd64-longtest, so maybe I need that specific setup to get the timing right... |
|
Is it possible that this is some GC jujitsu? func (d *DLL) FindProc(name string) (proc *Proc, err error) {
namep, err := BytePtrFromString(name)
if err != nil {
return nil, err
}
a, e := getprocaddress(uintptr(d.Handle), namep) And then: //go:linkname syscall_getprocaddress syscall.getprocaddress
//go:nosplit
//go:cgo_unsafe_args
func syscall_getprocaddress(handle uintptr, procname *byte) (outhandle, err uintptr) {
lockOSThread()
defer unlockOSThread()
c := &getg().m.syscall
c.fn = getGetProcAddress()
c.n = 2
c.args = uintptr(noescape(unsafe.Pointer(&handle)))
cgocall(asmstdcallAddr, unsafe.Pointer(c))
outhandle = c.r1
if outhandle == 0 {
err = c.err
}
return
} Could something weird be happening where |
Change https://golang.org/cl/367654 mentions this issue: |
If you're right (I don't know whether you are or not) that may indicate a bug in the implementation of |
Sort of I think. The majority of uses cases for it pass around integers of one form or another, which means for the uintptr case, the caller is responsible for keeping alive its casted arguments. The Windows example above violates that. But most others don't. Except for one OpenBSD case I found, maybe: diff --git a/src/runtime/sys_openbsd1.go b/src/runtime/sys_openbsd1.go
index 4b80f60226..d852e3c58a 100644
--- a/src/runtime/sys_openbsd1.go
+++ b/src/runtime/sys_openbsd1.go
@@ -14,7 +14,10 @@ import (
//go:nosplit
//go:cgo_unsafe_args
func thrsleep(ident uintptr, clock_id int32, tsp *timespec, lock uintptr, abort *uint32) int32 {
- return libcCall(unsafe.Pointer(abi.FuncPCABI0(thrsleep_trampoline)), unsafe.Pointer(&ident))
+ ret := libcCall(unsafe.Pointer(abi.FuncPCABI0(thrsleep_trampoline)), unsafe.Pointer(&ident))
+ KeepAlive(tsp)
+ KeepAlive(abort)
+ return ret
}
func thrsleep_trampoline()
My CL handles the three Windows ones, and depending on how certain the OpenBSD one smells, I'll roll that into the same patch. |
At least in the cases addressed by https://go-review.googlesource.com/c/go/+/367654/3/src/runtime/syscall_windows.go, even the first argument is already dead by time But I'm also not opposed to making |
As far as I can tell, the only effect of |
This specific bug is sort of interesting from a security persecptive, as I wonder if it could be leveraged for some clever code execution arising from the UaF. Code was going to call CC @FiloSottile |
In Still, I think that we should make the compiler do something sensible here, rather than depending on adding |
The code in question is:
This code is storing Maybe we can change I agree the latter seems in principle more appealing. But as |
My analysis and CL was looking at pointers, such as |
We also use So, OK, let's try to remember to fix the uses in the standard library. Thanks. |
@zx2c4 Thanks. I'd recommend we look at all E.g., looking briefly, this function in runtime/sys_darwin.go would be suspect:
because |
Right, yea. I've done a big pass through and caught a lot of cases on a bunch of platforms. The CL is updated now. @ianlancetaylor / @randall77 - I'll wait for your 👍 on that CL before I submit it. |
By the way, the patch's diffstat is now +124/-27, which is quite a bit more than the original +1/-0 when this all began. That makes me wonder whether we'd be better off changing the meaning of |
I was wondering that too. I think it would be pretty easy for liveness analysis to just mark all parameters of I'm inclined though to say lets land your CL since it's already prepared, and we can always roll it back if we implement the cmd/compile change. |
That works. Hopefully we can get all the changes, then, into one CL now, so that the eventual revert is more straight forward. |
@gopherbot please backport this because it seems kind of security sensitive. |
Backport issue(s) opened: #49867 (for 1.16), #49868 (for 1.17). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
Change https://golang.org/cl/368355 mentions this issue: |
Change https://golang.org/cl/368356 mentions this issue: |
…alive to prevent GC When syscall's DLL.FindProc calls into syscall_getprocaddress with a byte slice pointer, we need to keep those bytes alive. Otherwise the GC will collect the allocation, and we wind up calling `GetProcAddress` on garbage, which showed up as various flakes in the builders. It turns out that this problem extends to many uses of //go:cgo_unsafe_args throughout, on all platforms. So this patch fixes the issue by keeping non-integer pointer arguments alive through their invocation in //go:cgo_unsafe_args functions. Fixes #49868. Updates #49731. Change-Id: I93e4fbc2e8e210cb3fc53149708758bb33f2f9c7 Reviewed-on: https://go-review.googlesource.com/c/go/+/368355 Trust: Jason A. Donenfeld <Jason@zx2c4.com> Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com>
…alive to prevent GC When syscall's DLL.FindProc calls into syscall_getprocaddress with a byte slice pointer, we need to keep those bytes alive. Otherwise the GC will collect the allocation, and we wind up calling `GetProcAddress` on garbage, which showed up as various flakes in the builders. It turns out that this problem extends to many uses of //go:cgo_unsafe_args throughout, on all platforms. So this patch fixes the issue by keeping non-integer pointer arguments alive through their invocation in //go:cgo_unsafe_args functions. Fixes #49867. Updates #49731. Change-Id: I93e4fbc2e8e210cb3fc53149708758bb33f2f9c7 Reviewed-on: https://go-review.googlesource.com/c/go/+/368356 Trust: Jason A. Donenfeld <Jason@zx2c4.com> Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com>
greplogs --dashboard -md -l -e 'Failed to find RtlGetNtVersionNumbers'
2021-11-22T18:57:33-5a3d871/windows-amd64-longtest
CC @bufflig
The text was updated successfully, but these errors were encountered: