From 8604e1c20053c0c2b61d4f7e9fce02e1af0b4794 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Tue, 24 Jan 2023 16:40:17 -0800 Subject: [PATCH] Avoid use of gohacks.SliceHeader to access pointer to data Accessing a pointer to the data in a slice can be achieved with `unsafe.Pointer(&slice[0])`. f051ec64639b83faabcfe766ff078072def3c2aa motivated using gohacks.SliceHeader in this way with "we often use SliceHeader to extract pointers from slices in a way that avoids bounds checking and/or handles nil slices correctly", but this no longer seems to be the case. None of the remaining uses are obviously performance sensitive or necessarily include bounds checks, nor get used with nil slices. This brings us one step closer to removing gohacks.SliceHeader, which is one less internal detail to keep in sync with Go. For #8422 PiperOrigin-RevId: 504408578 --- pkg/fdchannel/BUILD | 1 - pkg/fdchannel/fdchannel_unsafe.go | 6 ++---- pkg/metric/metric_unsafe.go | 4 +--- pkg/sentry/fsimpl/iouringfs/BUILD | 1 - pkg/sentry/fsimpl/iouringfs/iouringfs_unsafe.go | 4 +--- pkg/sentry/vfs/mount_unsafe.go | 3 +-- pkg/sync/atomicptrmap/generic_atomicptrmap_unsafe.go | 3 +-- 7 files changed, 6 insertions(+), 16 deletions(-) diff --git a/pkg/fdchannel/BUILD b/pkg/fdchannel/BUILD index d240132e5..fe8f505bf 100644 --- a/pkg/fdchannel/BUILD +++ b/pkg/fdchannel/BUILD @@ -7,7 +7,6 @@ go_library( srcs = ["fdchannel_unsafe.go"], visibility = ["//visibility:public"], deps = [ - "//pkg/gohacks", "@org_golang_x_sys//unix:go_default_library", ], ) diff --git a/pkg/fdchannel/fdchannel_unsafe.go b/pkg/fdchannel/fdchannel_unsafe.go index f9a201eeb..1eb849658 100644 --- a/pkg/fdchannel/fdchannel_unsafe.go +++ b/pkg/fdchannel/fdchannel_unsafe.go @@ -24,7 +24,6 @@ import ( "unsafe" "golang.org/x/sys/unix" - "gvisor.dev/gvisor/pkg/gohacks" ) // int32 is the real type of a file descriptor. @@ -55,10 +54,9 @@ func (ep *Endpoint) Init(sockfd int) { // sendmsg+recvmsg for a zero-length datagram is slightly faster than // sendmsg+recvmsg for a single byte over a stream socket. cmsgSlice := make([]byte, unix.CmsgSpace(sizeofInt32)) - cmsgSliceHdr := (*gohacks.SliceHeader)(unsafe.Pointer(&cmsgSlice)) ep.sockfd = int32(sockfd) - ep.msghdr.Control = (*byte)(cmsgSliceHdr.Data) - ep.cmsg = (*unix.Cmsghdr)(cmsgSliceHdr.Data) + ep.msghdr.Control = (*byte)(unsafe.Pointer(&cmsgSlice[0])) + ep.cmsg = (*unix.Cmsghdr)(unsafe.Pointer(&cmsgSlice[0])) // ep.msghdr.Controllen and ep.cmsg.* are mutated by recvmsg(2), so they're // set before calling sendmsg/recvmsg. } diff --git a/pkg/metric/metric_unsafe.go b/pkg/metric/metric_unsafe.go index a9efce085..06c043e2a 100644 --- a/pkg/metric/metric_unsafe.go +++ b/pkg/metric/metric_unsafe.go @@ -33,13 +33,11 @@ func snapshotDistribution(samples []atomicbitops.Uint64) []uint64 { // no race condition from getting the number of buckets upfront. numBuckets := len(samples) snapshot := make([]uint64, numBuckets) - samplesHeader := (*gohacks.SliceHeader)(unsafe.Pointer(&samples)) - snapshotHeader := (*gohacks.SliceHeader)(unsafe.Pointer(&snapshot)) if sync.RaceEnabled { // runtime.RaceDisable() doesn't actually stop the race detector, so it // can't help us here. Instead, call runtime.memmove directly, which is // not instrumented by the race detector. - gohacks.Memmove(snapshotHeader.Data, samplesHeader.Data, unsafe.Sizeof(uint64(0))*uintptr(numBuckets)) + gohacks.Memmove(unsafe.Pointer(&snapshot[0]), unsafe.Pointer(&samples[0]), unsafe.Sizeof(uint64(0))*uintptr(numBuckets)) } else { for i := range samples { snapshot[i] = samples[i].RacyLoad() diff --git a/pkg/sentry/fsimpl/iouringfs/BUILD b/pkg/sentry/fsimpl/iouringfs/BUILD index eaad52d36..8e5eec31d 100644 --- a/pkg/sentry/fsimpl/iouringfs/BUILD +++ b/pkg/sentry/fsimpl/iouringfs/BUILD @@ -16,7 +16,6 @@ go_library( "//pkg/atomicbitops", "//pkg/context", "//pkg/errors/linuxerr", - "//pkg/gohacks", "//pkg/hostarch", "//pkg/safemem", "//pkg/sentry/kernel", diff --git a/pkg/sentry/fsimpl/iouringfs/iouringfs_unsafe.go b/pkg/sentry/fsimpl/iouringfs/iouringfs_unsafe.go index 715ace5d6..1533a7d9e 100644 --- a/pkg/sentry/fsimpl/iouringfs/iouringfs_unsafe.go +++ b/pkg/sentry/fsimpl/iouringfs/iouringfs_unsafe.go @@ -19,7 +19,6 @@ import ( "unsafe" "gvisor.dev/gvisor/pkg/atomicbitops" - "gvisor.dev/gvisor/pkg/gohacks" ) func atomicUint32AtOffset(buf []byte, offset int) *atomicbitops.Uint32 { @@ -30,6 +29,5 @@ func atomicUint32AtOffset(buf []byte, offset int) *atomicbitops.Uint32 { if offset%sizeOfUint32 != 0 { panic(fmt.Sprintf("cast at offset %d would produce unaligned pointer", offset)) } - hdr := (*gohacks.SliceHeader)(unsafe.Pointer(&buf)) - return (*atomicbitops.Uint32)(unsafe.Add(hdr.Data, offset)) + return (*atomicbitops.Uint32)(unsafe.Pointer(&buf[offset])) } diff --git a/pkg/sentry/vfs/mount_unsafe.go b/pkg/sentry/vfs/mount_unsafe.go index 6ef24b3d8..2769ab5a9 100644 --- a/pkg/sentry/vfs/mount_unsafe.go +++ b/pkg/sentry/vfs/mount_unsafe.go @@ -153,8 +153,7 @@ func (mt *mountTable) Init() { func newMountTableSlots(cap uintptr) unsafe.Pointer { slice := make([]mountSlot, cap, cap) - hdr := (*gohacks.SliceHeader)(unsafe.Pointer(&slice)) - return hdr.Data + return unsafe.Pointer(&slice[0]) } // Lookup returns the Mount with the given parent, mounted at the given point. diff --git a/pkg/sync/atomicptrmap/generic_atomicptrmap_unsafe.go b/pkg/sync/atomicptrmap/generic_atomicptrmap_unsafe.go index 1b7212c86..8324d4c0f 100644 --- a/pkg/sync/atomicptrmap/generic_atomicptrmap_unsafe.go +++ b/pkg/sync/atomicptrmap/generic_atomicptrmap_unsafe.go @@ -371,8 +371,7 @@ func (shard *apmShard) rehash(oldSlots unsafe.Pointer) { // Allocate the new table. newSlotsSlice := make([]apmSlot, newSize) - newSlotsHeader := (*gohacks.SliceHeader)(unsafe.Pointer(&newSlotsSlice)) - newSlots := newSlotsHeader.Data + newSlots := unsafe.Pointer(&newSlotsSlice[0]) newMask := newSize - 1 // Start a writer critical section now so that racing users of the old