diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9b8c739..5fd26e7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -24,6 +24,9 @@ jobs: - '1.20' - '1.21' - '1.22' + - '1.23' + - 'oldstable' + - 'stable' steps: - name: Checkout diff --git a/conversion.go b/conversion.go index bc04f50..859aa04 100644 --- a/conversion.go +++ b/conversion.go @@ -31,13 +31,13 @@ func utf16FromString(str string) []uint16 { // goBytes copies the given C byte array to a Go byte array (see `C.GoBytes`). // This function avoids having cgo as dependency. -func goBytes(src uintptr, len uint32) []byte { - if src == uintptr(0) { +func goBytes(src *byte, len uint32) []byte { + if src == nil || len == 0 { return []byte{} } rv := make([]byte, len) copy(rv, *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{ - Data: src, + Data: uintptr(unsafe.Pointer(src)), Len: int(len), Cap: int(len), }))) @@ -59,7 +59,7 @@ func sysToCredential(cred *sysCREDENTIAL) (result *Credential) { result.CredentialBlob = goBytes(cred.CredentialBlob, cred.CredentialBlobSize) result.Attributes = make([]CredentialAttribute, cred.AttributeCount) attrSlice := *(*[]sysCREDENTIAL_ATTRIBUTE)(unsafe.Pointer(&reflect.SliceHeader{ - Data: cred.Attributes, + Data: uintptr(unsafe.Pointer(cred.Attributes)), Len: int(cred.AttributeCount), Cap: int(cred.AttributeCount), })) @@ -85,17 +85,13 @@ func sysFromCredential(cred *Credential) (result *sysCREDENTIAL) { result.LastWritten = syscall.NsecToFiletime(cred.LastWritten.UnixNano()) result.CredentialBlobSize = uint32(len(cred.CredentialBlob)) if len(cred.CredentialBlob) > 0 { - result.CredentialBlob = uintptr(unsafe.Pointer(&cred.CredentialBlob[0])) - } else { - result.CredentialBlob = 0 + result.CredentialBlob = &cred.CredentialBlob[0] } result.Persist = uint32(cred.Persist) result.AttributeCount = uint32(len(cred.Attributes)) attributes := make([]sysCREDENTIAL_ATTRIBUTE, len(cred.Attributes)) if len(attributes) > 0 { - result.Attributes = uintptr(unsafe.Pointer(&attributes[0])) - } else { - result.Attributes = 0 + result.Attributes = &attributes[0] } for i := range cred.Attributes { inAttr := &cred.Attributes[i] @@ -104,9 +100,7 @@ func sysFromCredential(cred *Credential) (result *sysCREDENTIAL) { outAttr.Flags = 0 outAttr.ValueSize = uint32(len(inAttr.Value)) if len(inAttr.Value) > 0 { - outAttr.Value = uintptr(unsafe.Pointer(&inAttr.Value[0])) - } else { - outAttr.Value = 0 + outAttr.Value = &inAttr.Value[0] } } result.TargetAlias, _ = syscall.UTF16PtrFromString(cred.TargetAlias) diff --git a/conversion_test.go b/conversion_test.go index b77b183..2d6ebec 100644 --- a/conversion_test.go +++ b/conversion_test.go @@ -1,3 +1,4 @@ +//go:build windows // +build windows package wincred @@ -5,7 +6,6 @@ package wincred import ( "testing" "time" - "unsafe" "github.com/stretchr/testify/assert" ) @@ -52,7 +52,7 @@ func BenchmarkUtf16ToByte(b *testing.B) { func TestGoBytes(t *testing.T) { input := []byte{1, 2, 3, 4, 5} - output := goBytes(uintptr(unsafe.Pointer(&input[0])), uint32(len(input))) + output := goBytes(&input[0], uint32(len(input))) assert.Equal(t, len(input), len(output)) assert.Equal(t, input[0], output[0]) assert.Equal(t, input[1], output[1]) @@ -65,7 +65,7 @@ func TestGoBytes(t *testing.T) { func TestGoBytes_Null(t *testing.T) { assert.NotPanics(t, func() { - output := goBytes(0, 123) + output := goBytes(nil, 123) assert.Equal(t, []byte{}, output) }) } @@ -73,7 +73,7 @@ func TestGoBytes_Null(t *testing.T) { func BenchmarkGoBytes(b *testing.B) { input := []byte{1, 2, 3, 4, 5} for i := 0; i < b.N; i++ { - goBytes(uintptr(unsafe.Pointer(&input[0])), uint32(len(input))) + _ = goBytes(&input[0], uint32(len(input))) } } @@ -108,7 +108,7 @@ func TestConversion_CredentialBlob(t *testing.T) { sys := sysFromCredential(cred) res := sysToCredential(sys) assert.Equal(t, uint32(3), sys.CredentialBlobSize) - assert.NotEqual(t, uintptr(0), sys.CredentialBlob) + assert.NotNil(t, res.CredentialBlob) assert.Equal(t, cred.CredentialBlob, res.CredentialBlob) } @@ -117,7 +117,7 @@ func TestConversion_CredentialBlob_Empty(t *testing.T) { cred.CredentialBlob = []byte{} // empty blob sys := sysFromCredential(cred) res := sysToCredential(sys) - assert.Equal(t, uintptr(0), sys.CredentialBlob) + assert.Nil(t, sys.CredentialBlob) assert.Equal(t, uint32(0), sys.CredentialBlobSize) assert.Equal(t, []byte{}, res.CredentialBlob) } @@ -127,7 +127,7 @@ func TestConversion_CredentialBlob_Nil(t *testing.T) { cred.CredentialBlob = nil // nil blob sys := sysFromCredential(cred) res := sysToCredential(sys) - assert.Equal(t, uintptr(0), sys.CredentialBlob) + assert.Nil(t, sys.CredentialBlob) assert.Equal(t, uint32(0), sys.CredentialBlobSize) assert.Equal(t, []byte{}, res.CredentialBlob) } @@ -140,7 +140,7 @@ func TestConversion_Attributes(t *testing.T) { } sys := sysFromCredential(cred) res := sysToCredential(sys) - assert.NotEqual(t, uintptr(0), sys.Attributes) + assert.NotNil(t, sys.Attributes) assert.Equal(t, uint32(2), sys.AttributeCount) assert.Equal(t, cred.Attributes, res.Attributes) } @@ -150,7 +150,7 @@ func TestConversion_Attributes_Empty(t *testing.T) { cred.Attributes = []CredentialAttribute{} sys := sysFromCredential(cred) res := sysToCredential(sys) - assert.Equal(t, uintptr(0), sys.Attributes) + assert.Nil(t, sys.Attributes) assert.Equal(t, uint32(0), sys.AttributeCount) assert.Equal(t, []CredentialAttribute{}, res.Attributes) } @@ -160,7 +160,7 @@ func TestConversion_Attributes_Nil(t *testing.T) { cred.Attributes = nil sys := sysFromCredential(cred) res := sysToCredential(sys) - assert.Equal(t, uintptr(0), sys.Attributes) + assert.Nil(t, sys.Attributes) assert.Equal(t, uint32(0), sys.AttributeCount) assert.Equal(t, []CredentialAttribute{}, res.Attributes) } diff --git a/go.mod b/go.mod index 673f244..8483b72 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/danieljoos/wincred go 1.18 require ( - github.com/stretchr/testify v1.9.0 + github.com/stretchr/testify v1.11.1 golang.org/x/sys v0.20.0 ) diff --git a/go.sum b/go.sum index 7d09d27..17455e7 100644 --- a/go.sum +++ b/go.sum @@ -4,8 +4,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= -github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= -github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/sys.go b/sys.go index fb8a6ac..a499970 100644 --- a/sys.go +++ b/sys.go @@ -5,6 +5,7 @@ package wincred import ( "reflect" + "runtime" "syscall" "unsafe" @@ -33,10 +34,10 @@ type sysCREDENTIAL struct { Comment *uint16 LastWritten windows.Filetime CredentialBlobSize uint32 - CredentialBlob uintptr + CredentialBlob *byte Persist uint32 AttributeCount uint32 - Attributes uintptr + Attributes *sysCREDENTIAL_ATTRIBUTE TargetAlias *uint16 UserName *uint16 } @@ -46,7 +47,7 @@ type sysCREDENTIAL_ATTRIBUTE struct { Keyword *uint16 Flags uint32 ValueSize uint32 - Value uintptr + Value *byte } // https://docs.microsoft.com/en-us/windows/desktop/api/wincred/ns-wincred-_credentialw @@ -93,6 +94,8 @@ func sysCredWrite(cred *Credential, typ sysCRED_TYPE) error { uintptr(unsafe.Pointer(ncred)), 0, ) + // Make sure everything reachable from ncred stays alive through the call. + runtime.KeepAlive(ncred) if ret == 0 { return err } diff --git a/sys_test.go b/sys_test.go index c803e2c..b36e509 100644 --- a/sys_test.go +++ b/sys_test.go @@ -5,7 +5,10 @@ package wincred import ( "errors" + "runtime" "testing" + "time" + "unsafe" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -89,3 +92,39 @@ func TestSysCredDelete_Mock(t *testing.T) { assert.Nil(t, err) mockCredDelete.AssertNumberOfCalls(t, "Call", 1) } + +func TestCredWrite_GCSafety_WithAttributes(t *testing.T) { + // Minimal repro for the Go 1.25 regression: we create a credential that has at least + // one Attribute with a non-empty Value (so sysFromCredential allocates the attributes + // slice internally). Then we force a GC after building the native struct and *before* + // calling CredWriteW. With the old uintptr-based fields, the GC can reclaim the slice, + // leaving dangling addresses and causing ERROR_INVALID_PARAMETER. With the fix, it’s fine. + cred := &Credential{ + TargetName: "Foo", + Comment: "Bar", + LastWritten: time.Now(), + TargetAlias: "MyAlias", + UserName: "Nobody", + Persist: PersistLocalMachine, + CredentialBlob: []byte("secret"), + Attributes: []CredentialAttribute{ + {Keyword: "label", Value: []byte("hello-world")}, + }, + } + + ncred := sysFromCredential(cred) + ncred.Type = uint32(sysCRED_TYPE_GENERIC) + + // run GC a few times to gc the attributes slice. + for i := 0; i < 5; i++ { + runtime.GC() + } + + // call CredWriteW - same as sysCredWrite + ret, _, err := procCredWrite.Call(uintptr(unsafe.Pointer(ncred)), 0) + if ret == 0 { + t.Fatalf("CredWriteW failed: %v", err) + } + + _ = sysCredDelete(cred, sysCRED_TYPE_GENERIC) +}