os/user: fix race detector failure

The race detector doesn't like that windows.GetSid* functions return
pointers to the SID structure. This change makes these functions return
values instead and mark them with nocheckptr.

Fixes #70378

Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-race
Change-Id: Iba39d75bb31679d25a5ee43b51e4abb0c435dbac
Reviewed-on: https://go-review.googlesource.com/c/go/+/628995
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/internal/syscall/windows/security_windows.go b/src/internal/syscall/windows/security_windows.go
index 547c30031a12f39d5c6e51e398b41226ca98d134..017e25aaef5ec085f393e8442889e8d28d374de0 100644 (file)
--- a/src/internal/syscall/windows/security_windows.go
+++ b/src/internal/syscall/windows/security_windows.go
@@ -5,6 +5,7 @@
 package windows
 
 import (
+       "runtime"
        "syscall"
        "unsafe"
 )
@@ -231,6 +232,33 @@ var SECURITY_NT_AUTHORITY = SID_IDENTIFIER_AUTHORITY{
 }
 
 //sys  IsValidSid(sid *syscall.SID) (valid bool) = advapi32.IsValidSid
-//sys  GetSidIdentifierAuthority(sid *syscall.SID) (idauth *SID_IDENTIFIER_AUTHORITY) = advapi32.GetSidIdentifierAuthority
-//sys  GetSidSubAuthority(sid *syscall.SID, subAuthorityIdx uint32) (subAuth *uint32) = advapi32.GetSidSubAuthority
-//sys  GetSidSubAuthorityCount(sid *syscall.SID) (count *uint8) = advapi32.GetSidSubAuthorityCount
+//sys  getSidIdentifierAuthority(sid *syscall.SID) (idauth uintptr) = advapi32.GetSidIdentifierAuthority
+//sys  getSidSubAuthority(sid *syscall.SID, subAuthorityIdx uint32) (subAuth uintptr) = advapi32.GetSidSubAuthority
+//sys  getSidSubAuthorityCount(sid *syscall.SID) (count uintptr) = advapi32.GetSidSubAuthorityCount
+
+// The following GetSid* functions are marked as //go:nocheckptr because checkptr
+// instrumentation can't see that the pointer returned by the syscall is pointing
+// into the sid's memory, which is normally allocated on the Go heap. Therefore,
+// the checkptr instrumentation would incorrectly flag the pointer dereference
+// as pointing to an invalid allocation.
+// Also, use runtime.KeepAlive to ensure that the sid is not garbage collected
+// before the GetSid* functions return, as the Go GC is not aware that the
+// pointers returned by the syscall are pointing into the sid's memory.
+
+//go:nocheckptr
+func GetSidIdentifierAuthority(sid *syscall.SID) SID_IDENTIFIER_AUTHORITY {
+       defer runtime.KeepAlive(sid)
+       return *(*SID_IDENTIFIER_AUTHORITY)(unsafe.Pointer(getSidIdentifierAuthority(sid)))
+}
+
+//go:nocheckptr
+func GetSidSubAuthority(sid *syscall.SID, subAuthorityIdx uint32) uint32 {
+       defer runtime.KeepAlive(sid)
+       return *(*uint32)(unsafe.Pointer(getSidSubAuthority(sid, subAuthorityIdx)))
+}
+
+//go:nocheckptr
+func GetSidSubAuthorityCount(sid *syscall.SID) uint8 {
+       defer runtime.KeepAlive(sid)
+       return *(*uint8)(unsafe.Pointer(getSidSubAuthorityCount(sid)))
+}

diff --git a/src/internal/syscall/windows/zsyscall_windows.go b/src/internal/syscall/windows/zsyscall_windows.go
index f4048c440e32eaeef67a18fafe658eb2b11ae61d..c81bc399ff76705a005d52471fabf0a2f9580b2c 100644 (file)
--- a/src/internal/syscall/windows/zsyscall_windows.go
+++ b/src/internal/syscall/windows/zsyscall_windows.go
@@ -124,21 +124,21 @@ func DuplicateTokenEx(hExistingToken syscall.Token, dwDesiredAccess uint32, lpTo
        return
 }
 
-func GetSidIdentifierAuthority(sid *syscall.SID) (idauth *SID_IDENTIFIER_AUTHORITY) {
+func getSidIdentifierAuthority(sid *syscall.SID) (idauth uintptr) {
        r0, _, _ := syscall.Syscall(procGetSidIdentifierAuthority.Addr(), 1, uintptr(unsafe.Pointer(sid)), 0, 0)
-       idauth = (*SID_IDENTIFIER_AUTHORITY)(unsafe.Pointer(r0))
+       idauth = uintptr(r0)
        return
 }
 
-func GetSidSubAuthority(sid *syscall.SID, subAuthorityIdx uint32) (subAuth *uint32) {
+func getSidSubAuthority(sid *syscall.SID, subAuthorityIdx uint32) (subAuth uintptr) {
        r0, _, _ := syscall.Syscall(procGetSidSubAuthority.Addr(), 2, uintptr(unsafe.Pointer(sid)), uintptr(subAuthorityIdx), 0)
-       subAuth = (*uint32)(unsafe.Pointer(r0))
+       subAuth = uintptr(r0)
        return
 }
 
-func GetSidSubAuthorityCount(sid *syscall.SID) (count *uint8) {
+func getSidSubAuthorityCount(sid *syscall.SID) (count uintptr) {
        r0, _, _ := syscall.Syscall(procGetSidSubAuthorityCount.Addr(), 1, uintptr(unsafe.Pointer(sid)), 0, 0)
-       count = (*uint8)(unsafe.Pointer(r0))
+       count = uintptr(r0)
        return
 }
 

diff --git a/src/os/user/lookup_windows.go b/src/os/user/lookup_windows.go
index 11bb58e87bde54a65cbf76bf49c95dbc7191409a..e0e77f3ea75a55e7ec565bc60209ba9a8955e2f2 100644 (file)
--- a/src/os/user/lookup_windows.go
+++ b/src/os/user/lookup_windows.go
@@ -98,11 +98,11 @@ func isServiceAccount(sid *syscall.SID) bool {
        // - "S-1-5-18": LocalSystem
        // - "S-1-5-19": LocalService
        // - "S-1-5-20": NetworkService
-       if *windows.GetSidSubAuthorityCount(sid) != windows.SID_REVISION ||
-               *windows.GetSidIdentifierAuthority(sid) != windows.SECURITY_NT_AUTHORITY {
+       if windows.GetSidSubAuthorityCount(sid) != windows.SID_REVISION ||
+               windows.GetSidIdentifierAuthority(sid) != windows.SECURITY_NT_AUTHORITY {
                return false
        }
-       switch *windows.GetSidSubAuthority(sid, 0) {
+       switch windows.GetSidSubAuthority(sid, 0) {
        case windows.SECURITY_LOCAL_SYSTEM_RID,
                windows.SECURITY_LOCAL_SERVICE_RID,
                windows.SECURITY_NETWORK_SERVICE_RID: