From 0bb6d5c3c3a6f01a96e8bebe5a796ff3d47fa2d1 Mon Sep 17 00:00:00 2001 From: "Paul \"TBBle\" Hampson" Date: Wed, 13 Jan 2021 04:50:54 +1100 Subject: [PATCH] Create a correctly-sized slice to proxy *uint16 Fixes the below issue seen in the containerd test suite. ``` fatal error: checkptr: converted pointer straddles multiple allocations ``` Also adds `-gcflags=all=-d=checkptr` to all the test runs on CI, to avoid this regressing in future. This requires testing with Go 1.14 or newer, so CI now runs on Go 1.15, as Go 1.14 did not recommend using checkptr on Windows. And _that_ requires the Visual Studio 2019 build image on AppVeyor. Signed-off-by: Paul "TBBle" Hampson --- appveyor.yml | 18 +++++++++--------- internal/safefile/safeopen.go | 2 +- internal/winapi/utils.go | 17 +++++++++++++---- internal/winapi/winapi_test.go | 15 +++++---------- 4 files changed, 28 insertions(+), 24 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index 9391879b65..8d7065b156 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -1,6 +1,6 @@ version: 0.1.{build} -image: Visual Studio 2017 +image: Visual Studio 2019 clone_folder: c:\gopath\src\github.com\Microsoft\hcsshim @@ -8,7 +8,7 @@ environment: GOPATH: c:\gopath PATH: "%GOPATH%\\bin;C:\\gometalinter-2.0.12-windows-amd64;%PATH%" -stack: go 1.13.4 +stack: go 1.15 build_script: - appveyor DownloadFile https://github.com/alecthomas/gometalinter/releases/download/v2.0.12/gometalinter-2.0.12-windows-amd64.zip @@ -19,16 +19,16 @@ build_script: - go build ./cmd/tar2ext4 - go build ./cmd/wclayer - go build ./cmd/device-util - - go build ./internal/tools/grantvmgroupaccess + - go build ./internal/tools/grantvmgroupaccess - go build ./internal/tools/uvmboot - go build ./internal/tools/zapdir - - go test -v ./... -tags admin + - go test -gcflags=all=-d=checkptr -v ./... -tags admin - cd test - - go test -v ./internal -tags admin - - go test -c ./containerd-shim-runhcs-v1/ -tags functional - - go test -c ./cri-containerd/ -tags functional - - go test -c ./functional/ -tags functional - - go test -c ./runhcs/ -tags functional + - go test -gcflags=all=-d=checkptr -v ./internal -tags admin + - go test -gcflags=all=-d=checkptr -c ./containerd-shim-runhcs-v1/ -tags functional + - go test -gcflags=all=-d=checkptr -c ./cri-containerd/ -tags functional + - go test -gcflags=all=-d=checkptr -c ./functional/ -tags functional + - go test -gcflags=all=-d=checkptr -c ./runhcs/ -tags functional - go build -o sample-logging-driver.exe ./cri-containerd/helpers/log.go artifacts: diff --git a/internal/safefile/safeopen.go b/internal/safefile/safeopen.go index 2c243b948a..05f22f39dd 100644 --- a/internal/safefile/safeopen.go +++ b/internal/safefile/safeopen.go @@ -177,7 +177,7 @@ func LinkRelative(oldname string, oldroot *os.File, newname string, newroot *os. linkinfo := (*winapi.FileLinkInformation)(unsafe.Pointer(linkinfoBuffer)) linkinfo.RootDirectory = parent.Fd() linkinfo.FileNameLength = uint32(len(newbase16) * 2) - copy((*[32768]uint16)(unsafe.Pointer(&linkinfo.FileName[0]))[:], newbase16) + copy(winapi.Uint16BufferToSlice(&linkinfo.FileName[0], len(newbase16)), newbase16) var iosb winapi.IOStatusBlock status := winapi.NtSetInformationFile( diff --git a/internal/winapi/utils.go b/internal/winapi/utils.go index 5d3f5a7b7f..db59567d08 100644 --- a/internal/winapi/utils.go +++ b/internal/winapi/utils.go @@ -2,12 +2,24 @@ package winapi import ( "errors" + "reflect" "syscall" "unsafe" "golang.org/x/sys/windows" ) +// Uint16BufferToSlice wraps a uint16 pointer-and-length into a slice +// for easier interop with Go APIs +func Uint16BufferToSlice(buffer *uint16, bufferLength int) (result []uint16) { + hdr := (*reflect.SliceHeader)(unsafe.Pointer(&result)) + hdr.Data = uintptr(unsafe.Pointer(buffer)) + hdr.Cap = bufferLength + hdr.Len = bufferLength + + return +} + type UnicodeString struct { Length uint16 MaximumLength uint16 @@ -16,12 +28,9 @@ type UnicodeString struct { //String converts a UnicodeString to a golang string func (uni UnicodeString) String() string { - p := (*[0xffff]uint16)(unsafe.Pointer(uni.Buffer)) - // UnicodeString is not guaranteed to be null terminated, therefore // use the UnicodeString's Length field - lengthInChars := uni.Length / 2 - return syscall.UTF16ToString(p[:lengthInChars]) + return syscall.UTF16ToString(Uint16BufferToSlice(uni.Buffer, int(uni.Length/2))) } // NewUnicodeString allocates a new UnicodeString and copies `s` into diff --git a/internal/winapi/winapi_test.go b/internal/winapi/winapi_test.go index e55a740428..0de03309a7 100644 --- a/internal/winapi/winapi_test.go +++ b/internal/winapi/winapi_test.go @@ -3,12 +3,10 @@ package winapi import ( "testing" "unicode/utf16" - "unsafe" ) -func wideStringsEqual(target, actual []uint16, actualLengthInBytes int) bool { - actualLength := actualLengthInBytes / 2 - if len(target) != actualLength { +func wideStringsEqual(target, actual []uint16) bool { + if len(target) != len(actual) { return false } @@ -38,13 +36,10 @@ func TestNewUnicodeString(t *testing.T) { t.Fatalf("Expected new Unicode String maximum length to be %d for target string %s, got %d instead", targetLength, target, uni.MaximumLength) } - uniBufferStringAsSlice := (*[32768]uint16)(unsafe.Pointer(uni.Buffer))[:] + uniBufferStringAsSlice := Uint16BufferToSlice(uni.Buffer, len(target)) - // since we have to do casting to convert the unicode string's buffer into a uint16 slice - // the length of the actual slice will not be the true length of the contents in the unicode buffer - // therefore we need to use the unicode string's length field when comparing - if !wideStringsEqual(targetWideString, uniBufferStringAsSlice, int(uni.Length)) { - t.Fatalf("Expected wide string %v, got %v instead", targetWideString, uniBufferStringAsSlice[:uni.Length]) + if !wideStringsEqual(targetWideString, uniBufferStringAsSlice) { + t.Fatalf("Expected wide string %v, got %v instead", targetWideString, uniBufferStringAsSlice) } } }