From fe76b779f299728f3bd63f77ea2c815504229c3b Mon Sep 17 00:00:00 2001 From: Yaroslav Vorobiov Date: Mon, 18 May 2020 12:52:01 +0000 Subject: [PATCH] windows: fix -d=checkptr slice failures This CL fixes unsafe casts to slices that are missing length or capacity. Running tests with -d=checkptr enabled may panic on casting unsafe.Pointer to a static array of large predefined length, that is most likely much bigger than the size of the actual array in memory. Checkptr check is not satisfied if slicing operator misses length and capacity arguments `(*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))[:]`, or when there is no slicing at all `(*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))`. To find all potential cases I used `grep -nr ")(unsafe.Pointer(" ./windows`, then filtered out safe casts when object size is always static and known at compile time. To reproduce the issue run tests with checkptr enabled `go test -a -gcflags=all=-d=checkptr ./windows/...`. Updates golang/go#34972 Fixes golang/go#38355 Change-Id: I9dd2084b4f9fb7618cdb140fb2f38b56b6d6cc04 GitHub-Last-Rev: 73288ad18a2692b1510f8ed564daad8d1cbd7e59 GitHub-Pull-Request: golang/sys#65 Reviewed-on: https://go-review.googlesource.com/c/sys/+/225418 Run-TryBot: Alex Brainman TryBot-Result: Gobot Gobot Reviewed-by: Alex Brainman --- windows/env_windows.go | 11 ++-------- windows/security_windows.go | 20 ++++++++++++++----- windows/svc/mgr/config.go | 40 +++++++++++++++---------------------- windows/svc/mgr/mgr.go | 13 +++++++++--- windows/svc/mgr/recovery.go | 12 ++++++++--- windows/svc/security.go | 7 ++----- windows/svc/service.go | 11 ++++++++-- windows/syscall_windows.go | 30 +++++++++++++++++++++++++++- 8 files changed, 92 insertions(+), 52 deletions(-) diff --git a/windows/env_windows.go b/windows/env_windows.go index f482a9fa..92ac05ff 100644 --- a/windows/env_windows.go +++ b/windows/env_windows.go @@ -8,7 +8,6 @@ package windows import ( "syscall" - "unicode/utf16" "unsafe" ) @@ -40,17 +39,11 @@ func (token Token) Environ(inheritExisting bool) (env []string, err error) { defer DestroyEnvironmentBlock(block) blockp := uintptr(unsafe.Pointer(block)) for { - entry := (*[(1 << 30) - 1]uint16)(unsafe.Pointer(blockp))[:] - for i, v := range entry { - if v == 0 { - entry = entry[:i] - break - } - } + entry := UTF16PtrToString((*uint16)(unsafe.Pointer(blockp))) if len(entry) == 0 { break } - env = append(env, string(utf16.Decode(entry))) + env = append(env, entry) blockp += 2 * (uintptr(len(entry)) + 1) } return env, nil diff --git a/windows/security_windows.go b/windows/security_windows.go index 4b6eff18..9e3c44a8 100644 --- a/windows/security_windows.go +++ b/windows/security_windows.go @@ -7,6 +7,8 @@ package windows import ( "syscall" "unsafe" + + "golang.org/x/sys/internal/unsafeheader" ) const ( @@ -1229,7 +1231,7 @@ func (sd *SECURITY_DESCRIPTOR) String() string { return "" } defer LocalFree(Handle(unsafe.Pointer(sddl))) - return UTF16ToString((*[(1 << 30) - 1]uint16)(unsafe.Pointer(sddl))[:]) + return UTF16PtrToString(sddl) } // ToAbsolute converts a self-relative security descriptor into an absolute one. @@ -1307,9 +1309,17 @@ func (absoluteSD *SECURITY_DESCRIPTOR) ToSelfRelative() (selfRelativeSD *SECURIT } func (selfRelativeSD *SECURITY_DESCRIPTOR) copySelfRelativeSecurityDescriptor() *SECURITY_DESCRIPTOR { - sdBytes := make([]byte, selfRelativeSD.Length()) - copy(sdBytes, (*[(1 << 31) - 1]byte)(unsafe.Pointer(selfRelativeSD))[:len(sdBytes)]) - return (*SECURITY_DESCRIPTOR)(unsafe.Pointer(&sdBytes[0])) + sdLen := (int)(selfRelativeSD.Length()) + + var src []byte + h := (*unsafeheader.Slice)(unsafe.Pointer(&src)) + h.Data = unsafe.Pointer(selfRelativeSD) + h.Len = sdLen + h.Cap = sdLen + + dst := make([]byte, sdLen) + copy(dst, src) + return (*SECURITY_DESCRIPTOR)(unsafe.Pointer(&dst[0])) } // SecurityDescriptorFromString converts an SDDL string describing a security descriptor into a @@ -1391,6 +1401,6 @@ func ACLFromEntries(explicitEntries []EXPLICIT_ACCESS, mergedACL *ACL) (acl *ACL } defer LocalFree(Handle(unsafe.Pointer(winHeapACL))) aclBytes := make([]byte, winHeapACL.aclSize) - copy(aclBytes, (*[(1 << 31) - 1]byte)(unsafe.Pointer(winHeapACL))[:len(aclBytes)]) + copy(aclBytes, (*[(1 << 31) - 1]byte)(unsafe.Pointer(winHeapACL))[:len(aclBytes):len(aclBytes)]) return (*ACL)(unsafe.Pointer(&aclBytes[0])), nil } diff --git a/windows/svc/mgr/config.go b/windows/svc/mgr/config.go index 8431edbe..30d39294 100644 --- a/windows/svc/mgr/config.go +++ b/windows/svc/mgr/config.go @@ -8,7 +8,6 @@ package mgr import ( "syscall" - "unicode/utf16" "unsafe" "golang.org/x/sys/windows" @@ -46,28 +45,21 @@ type Config struct { DelayedAutoStart bool // the service is started after other auto-start services are started plus a short delay } -func toString(p *uint16) string { - if p == nil { - return "" - } - return syscall.UTF16ToString((*[4096]uint16)(unsafe.Pointer(p))[:]) -} - func toStringSlice(ps *uint16) []string { - if ps == nil { - return nil - } r := make([]string, 0) - for from, i, p := 0, 0, (*[1 << 24]uint16)(unsafe.Pointer(ps)); true; i++ { - if p[i] == 0 { - // empty string marks the end - if i <= from { - break - } - r = append(r, string(utf16.Decode(p[from:i]))) - from = i + 1 + p := unsafe.Pointer(ps) + + for { + s := windows.UTF16PtrToString((*uint16)(p)) + if len(s) == 0 { + break } + + r = append(r, s) + offset := unsafe.Sizeof(uint16(0)) * (uintptr)(len(s)+1) + p = unsafe.Pointer(uintptr(p) + offset) } + return r } @@ -110,13 +102,13 @@ func (s *Service) Config() (Config, error) { ServiceType: p.ServiceType, StartType: p.StartType, ErrorControl: p.ErrorControl, - BinaryPathName: toString(p.BinaryPathName), - LoadOrderGroup: toString(p.LoadOrderGroup), + BinaryPathName: windows.UTF16PtrToString(p.BinaryPathName), + LoadOrderGroup: windows.UTF16PtrToString(p.LoadOrderGroup), TagId: p.TagId, Dependencies: toStringSlice(p.Dependencies), - ServiceStartName: toString(p.ServiceStartName), - DisplayName: toString(p.DisplayName), - Description: toString(p2.Description), + ServiceStartName: windows.UTF16PtrToString(p.ServiceStartName), + DisplayName: windows.UTF16PtrToString(p.DisplayName), + Description: windows.UTF16PtrToString(p2.Description), DelayedAutoStart: delayedStart, }, nil } diff --git a/windows/svc/mgr/mgr.go b/windows/svc/mgr/mgr.go index 33944d0f..9a454da6 100644 --- a/windows/svc/mgr/mgr.go +++ b/windows/svc/mgr/mgr.go @@ -17,6 +17,7 @@ import ( "unicode/utf16" "unsafe" + "golang.org/x/sys/internal/unsafeheader" "golang.org/x/sys/windows" ) @@ -73,7 +74,7 @@ func (m *Mgr) LockStatus() (*LockStatus, error) { status := &LockStatus{ IsLocked: lockStatus.IsLocked != 0, Age: time.Duration(lockStatus.LockDuration) * time.Second, - Owner: windows.UTF16ToString((*[(1 << 30) - 1]uint16)(unsafe.Pointer(lockStatus.LockOwner))[:]), + Owner: windows.UTF16PtrToString(lockStatus.LockOwner), } return status, nil } @@ -201,10 +202,16 @@ func (m *Mgr) ListServices() ([]string, error) { if servicesReturned == 0 { return nil, nil } - services := (*[1 << 20]windows.ENUM_SERVICE_STATUS_PROCESS)(unsafe.Pointer(&buf[0]))[:servicesReturned] + + var services []windows.ENUM_SERVICE_STATUS_PROCESS + hdr := (*unsafeheader.Slice)(unsafe.Pointer(&services)) + hdr.Data = unsafe.Pointer(&buf[0]) + hdr.Len = int(servicesReturned) + hdr.Cap = int(servicesReturned) + var names []string for _, s := range services { - name := syscall.UTF16ToString((*[1 << 20]uint16)(unsafe.Pointer(s.ServiceName))[:]) + name := windows.UTF16PtrToString(s.ServiceName) names = append(names, name) } return names, nil diff --git a/windows/svc/mgr/recovery.go b/windows/svc/mgr/recovery.go index 71ce2b81..e465cbbd 100644 --- a/windows/svc/mgr/recovery.go +++ b/windows/svc/mgr/recovery.go @@ -12,6 +12,7 @@ import ( "time" "unsafe" + "golang.org/x/sys/internal/unsafeheader" "golang.org/x/sys/windows" ) @@ -68,8 +69,13 @@ func (s *Service) RecoveryActions() ([]RecoveryAction, error) { return nil, err } + var actions []windows.SC_ACTION + hdr := (*unsafeheader.Slice)(unsafe.Pointer(&actions)) + hdr.Data = unsafe.Pointer(p.Actions) + hdr.Len = int(p.ActionsCount) + hdr.Cap = int(p.ActionsCount) + var recoveryActions []RecoveryAction - actions := (*[1024]windows.SC_ACTION)(unsafe.Pointer(p.Actions))[:p.ActionsCount] for _, action := range actions { recoveryActions = append(recoveryActions, RecoveryAction{Type: int(action.Type), Delay: time.Duration(action.Delay) * time.Millisecond}) } @@ -112,7 +118,7 @@ func (s *Service) RebootMessage() (string, error) { return "", err } p := (*windows.SERVICE_FAILURE_ACTIONS)(unsafe.Pointer(&b[0])) - return toString(p.RebootMsg), nil + return windows.UTF16PtrToString(p.RebootMsg), nil } // SetRecoveryCommand sets the command line of the process to execute in response to the RunCommand service controller action. @@ -131,5 +137,5 @@ func (s *Service) RecoveryCommand() (string, error) { return "", err } p := (*windows.SERVICE_FAILURE_ACTIONS)(unsafe.Pointer(&b[0])) - return toString(p.Command), nil + return windows.UTF16PtrToString(p.Command), nil } diff --git a/windows/svc/security.go b/windows/svc/security.go index 6fbc9236..65025998 100644 --- a/windows/svc/security.go +++ b/windows/svc/security.go @@ -7,8 +7,6 @@ package svc import ( - "unsafe" - "golang.org/x/sys/windows" ) @@ -48,9 +46,8 @@ func IsAnInteractiveSession() (bool, error) { if err != nil { return false, err } - p := unsafe.Pointer(&gs.Groups[0]) - groups := (*[2 << 20]windows.SIDAndAttributes)(p)[:gs.GroupCount] - for _, g := range groups { + + for _, g := range gs.AllGroups() { if windows.EqualSid(g.Sid, interSid) { return true, nil } diff --git a/windows/svc/service.go b/windows/svc/service.go index ee3d6965..bae818dd 100644 --- a/windows/svc/service.go +++ b/windows/svc/service.go @@ -14,6 +14,7 @@ import ( "syscall" "unsafe" + "golang.org/x/sys/internal/unsafeheader" "golang.org/x/sys/windows" ) @@ -224,10 +225,16 @@ const ( func (s *service) run() { s.goWaits.Wait() s.h = windows.Handle(ssHandle) - argv := (*[100]*int16)(unsafe.Pointer(sArgv))[:sArgc] + + var argv []*uint16 + hdr := (*unsafeheader.Slice)(unsafe.Pointer(&argv)) + hdr.Data = unsafe.Pointer(sArgv) + hdr.Len = int(sArgc) + hdr.Cap = int(sArgc) + args := make([]string, len(argv)) for i, a := range argv { - args[i] = syscall.UTF16ToString((*[1 << 20]uint16)(unsafe.Pointer(a))[:]) + args[i] = windows.UTF16PtrToString(a) } cmdsToHandler := make(chan ChangeRequest) diff --git a/windows/syscall_windows.go b/windows/syscall_windows.go index b3e55035..12c0544c 100644 --- a/windows/syscall_windows.go +++ b/windows/syscall_windows.go @@ -13,6 +13,8 @@ import ( "time" "unicode/utf16" "unsafe" + + "golang.org/x/sys/internal/unsafeheader" ) type Handle uintptr @@ -117,6 +119,32 @@ func UTF16PtrFromString(s string) (*uint16, error) { return &a[0], nil } +// UTF16PtrToString takes a pointer to a UTF-16 sequence and returns the corresponding UTF-8 encoded string. +// If the pointer is nil, this returns the empty string. This assumes that the UTF-16 sequence is terminated +// at a zero word; if the zero word is not present, the program may crash. +func UTF16PtrToString(p *uint16) string { + if p == nil { + return "" + } + if *p == 0 { + return "" + } + + // Find NUL terminator. + n := 0 + for ptr := unsafe.Pointer(p); *(*uint16)(ptr) != 0; n++ { + ptr = unsafe.Pointer(uintptr(ptr) + unsafe.Sizeof(*p)) + } + + var s []uint16 + h := (*unsafeheader.Slice)(unsafe.Pointer(&s)) + h.Data = unsafe.Pointer(p) + h.Len = n + h.Cap = n + + return string(utf16.Decode(s)) +} + func Getpagesize() int { return 4096 } // NewCallback converts a Go function to a function pointer conforming to the stdcall calling convention. @@ -1383,7 +1411,7 @@ func (t Token) KnownFolderPath(folderID *KNOWNFOLDERID, flags uint32) (string, e return "", err } defer CoTaskMemFree(unsafe.Pointer(p)) - return UTF16ToString((*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))[:]), nil + return UTF16PtrToString(p), nil } // RtlGetVersion returns the version of the underlying operating system, ignoring