rados: remove pointer arithmetic on C-buffers

This change uses slices on top of C allocated array memory in order
to have a simple (no pointer arithmetic) and safe (boundary-checked)
access to its elements.

Signed-off-by: Sven Anderson <sven@redhat.com>

internal/cutil/aliases.go

@@ -8,12 +8,13 @@ typedef void* voidptr;
 import "C"
 
 import (
+	"math"
 	"unsafe"
 )
 
 const (
 	// MaxIdx is the maximum index on 32 bit systems
-	MaxIdx = 1<<31 - 1 // 2GB, max int32 value, should be safe
+	MaxIdx = math.MaxInt32 // 2GB, max int32 value, should be safe
 
 	// PtrSize is the size of a pointer
 	PtrSize = C.sizeof_voidptr

internal/cutil/cslice.go

@@ -0,0 +1,76 @@
+package cutil
+
+// The following code needs some explanation:
+// This creates slices on top of the C memory buffers allocated before in
+// order to safely and comfortably use them as arrays. First the void pointer
+// is cast to a pointer to an array of the type that will be stored in the
+// array. Because the size of an array is a constant, but the real array size
+// is dynamic, we just use the biggest possible index value MaxIdx, to make
+// sure it's always big enough. (Nothing is allocated by casting, so the size
+// can be arbitrarily big.) So, if the array should store items of myType, the
+// cast would be (*[MaxIdx]myItem)(myCMemPtr).
+// From that array pointer a slice is created with the [start:end:capacity]
+// syntax. The capacity must be set explicitly here, because by default it
+// would be set to the size of the original array, which is MaxIdx, which
+// doesn't reflect reality in this case. This results in definitions like:
+// cSlice := (*[MaxIdx]myItem)(myCMemPtr)[:numOfItems:numOfItems]
+
+////////// CPtr //////////
+
+// CPtrCSlice is a C allocated slice of C pointers.
+type CPtrCSlice []CPtr
+
+// NewCPtrCSlice returns a CPtrSlice.
+// Similar to CString it must be freed with slice.Free()
+func NewCPtrCSlice(size int) CPtrCSlice {
+	if size == 0 {
+		return nil
+	}
+	cMem := Malloc(SizeT(size) * PtrSize)
+	cSlice := (*[MaxIdx]CPtr)(cMem)[:size:size]
+	return cSlice
+}
+
+// Ptr returns a pointer to CPtrSlice
+func (v *CPtrCSlice) Ptr() CPtr {
+	if len(*v) == 0 {
+		return nil
+	}
+	return CPtr(&(*v)[0])
+}
+
+// Free frees a CPtrSlice
+func (v *CPtrCSlice) Free() {
+	Free(v.Ptr())
+	*v = nil
+}
+
+////////// SizeT //////////
+
+// SizeTCSlice is a C allocated slice of C.size_t.
+type SizeTCSlice []SizeT
+
+// NewSizeTCSlice returns a SizeTCSlice.
+// Similar to CString it must be freed with slice.Free()
+func NewSizeTCSlice(size int) SizeTCSlice {
+	if size == 0 {
+		return nil
+	}
+	cMem := Malloc(SizeT(size) * SizeTSize)
+	cSlice := (*[MaxIdx]SizeT)(cMem)[:size:size]
+	return cSlice
+}
+
+// Ptr returns a pointer to SizeTCSlice
+func (v *SizeTCSlice) Ptr() CPtr {
+	if len(*v) == 0 {
+		return nil
+	}
+	return CPtr(&(*v)[0])
+}
+
+// Free frees a SizeTCSlice
+func (v *SizeTCSlice) Free() {
+	Free(v.Ptr())
+	*v = nil
+}

internal/cutil/cslice_test.go

@@ -0,0 +1,88 @@
+package cutil
+
+import (
+	"testing"
+	"unsafe"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCPtrCSlice(t *testing.T) {
+	t.Run("Size", func(t *testing.T) {
+		const size = 256
+		slice := NewCPtrCSlice(size)
+		defer slice.Free()
+		assert.Len(t, slice, size)
+	})
+	t.Run("Fill", func(t *testing.T) {
+		const size = 256
+		slice := NewCPtrCSlice(size)
+		defer slice.Free()
+		for i := range slice {
+			// In order to fill the array with valid C pointers we simply use
+			// the addresses of the array elements themselves.
+			slice[i] = CPtr(&slice[i])
+		}
+		for i := 0; i < size; i++ {
+			p := (*CPtr)(unsafe.Pointer(uintptr(slice.Ptr()) + uintptr(i)*uintptr(PtrSize)))
+			assert.Equal(t, slice[i], *p)
+		}
+	})
+	t.Run("OutOfBound", func(t *testing.T) {
+		const size = 1
+		slice := NewCPtrCSlice(size)
+		defer slice.Free()
+		assert.Panics(t, func() { slice[size] = nil })
+	})
+	t.Run("FreeSetsNil", func(t *testing.T) {
+		const size = 1
+		slice := NewCPtrCSlice(size)
+		slice.Free()
+		assert.Nil(t, slice)
+	})
+	t.Run("EmptySlice", func(t *testing.T) {
+		empty := NewCPtrCSlice(0)
+		assert.Len(t, empty, 0)
+		assert.Nil(t, empty)
+		assert.NotPanics(t, func() { empty.Free() })
+	})
+}
+
+func TestSizeTCSlice(t *testing.T) {
+	t.Run("Size", func(t *testing.T) {
+		const size = 256
+		slice := NewSizeTCSlice(size)
+		defer slice.Free()
+		assert.Len(t, slice, size)
+	})
+	t.Run("Fill", func(t *testing.T) {
+		const size = 256
+		slice := NewSizeTCSlice(size)
+		defer slice.Free()
+		for i := range slice {
+			slice[i] = SizeT(i)
+		}
+		for i := 0; i < size; i++ {
+			p := (*SizeT)(unsafe.Pointer(uintptr(slice.Ptr()) + uintptr(i)*uintptr(PtrSize)))
+			assert.Equal(t, slice[i], *p)
+		}
+	})
+	t.Run("OutOfBound", func(t *testing.T) {
+		const size = 1
+		slice := NewSizeTCSlice(size)
+		defer slice.Free()
+		assert.Panics(t, func() { slice[size] = 0 })
+	})
+	t.Run("FreeSetsNil", func(t *testing.T) {
+		const size = 1
+		slice := NewSizeTCSlice(size)
+		slice.Free()
+		assert.Nil(t, slice)
+	})
+	t.Run("EmptySlice", func(t *testing.T) {
+		empty := NewSizeTCSlice(0)
+		assert.Len(t, empty, 0)
+		assert.Nil(t, empty)
+		assert.NotPanics(t, func() { empty.Free() })
+	})
+}

rados/omap.go

@@ -4,20 +4,14 @@ package rados
 #cgo LDFLAGS: -lrados
 #include <stdlib.h>
 #include <rados/librados.h>
-
-typedef void* voidptr;
-
 */
 import "C"
 
 import (
 	"runtime"
 	"unsafe"
-)
 
-const (
-	ptrSize   = C.sizeof_voidptr
-	sizeTSize = C.sizeof_size_t
+	"github.com/ceph/go-ceph/internal/cutil"
 )
 
 // setOmapStep is a write op step. It holds C memory used in the operation.
@@ -26,50 +20,44 @@ type setOmapStep struct {
 	withoutUpdate
 
 	// C arguments
-	cKeys    **C.char
-	cValues  **C.char
-	cLengths *C.size_t
+	cKeys    cutil.CPtrCSlice
+	cValues  cutil.CPtrCSlice
+	cLengths cutil.SizeTCSlice
 	cNum     C.size_t
 }
 
 func newSetOmapStep(pairs map[string][]byte) *setOmapStep {
 
-	maplen := C.size_t(len(pairs))
-	cKeys := C.malloc(maplen * ptrSize)
-	cValues := C.malloc(maplen * ptrSize)
-	cLengths := C.malloc(maplen * sizeTSize)
+	maplen := len(pairs)
+	cKeys := cutil.NewCPtrCSlice(maplen)
+	cValues := cutil.NewCPtrCSlice(maplen)
+	cLengths := cutil.NewSizeTCSlice(maplen)
 
 	sos := &setOmapStep{
-		cKeys:    (**C.char)(cKeys),
-		cValues:  (**C.char)(cValues),
-		cLengths: (*C.size_t)(cLengths),
-		cNum:     C.size_t(len(pairs)),
+		cKeys:    cKeys,
+		cValues:  cValues,
+		cLengths: cLengths,
+		cNum:     C.size_t(maplen),
 	}
-	sos.add(cKeys)
-	sos.add(cValues)
-	sos.add(cLengths)
 
 	var i uintptr
 	for key, value := range pairs {
 		// key
 		ck := C.CString(key)
 		sos.add(unsafe.Pointer(ck))
-		ckp := (**C.char)(unsafe.Pointer(uintptr(cKeys) + i*ptrSize))
-		*ckp = ck
+		cKeys[i] = cutil.CPtr(ck)
 
 		// value and its length
-		cvp := (**C.char)(unsafe.Pointer(uintptr(cValues) + i*ptrSize))
-		vlen := C.size_t(len(value))
+		vlen := cutil.SizeT(len(value))
 		if vlen > 0 {
 			cv := C.CBytes(value)
 			sos.add(cv)
-			*cvp = (*C.char)(cv)
+			cValues[i] = cutil.CPtr(cv)
 		} else {
-			*cvp = nil
+			cValues[i] = nil
 		}
 
-		clp := (*C.size_t)(unsafe.Pointer(uintptr(cLengths) + i*ptrSize))
-		*clp = vlen
+		cLengths[i] = vlen
 
 		i++
 	}
@@ -79,9 +67,9 @@ func newSetOmapStep(pairs map[string][]byte) *setOmapStep {
 }
 
 func (sos *setOmapStep) free() {
-	sos.cKeys = nil
-	sos.cValues = nil
-	sos.cLengths = nil
+	sos.cKeys.Free()
+	sos.cValues.Free()
+	sos.cLengths.Free()
 	sos.withRefs.free()
 }
 
@@ -190,23 +178,21 @@ type removeOmapKeysStep struct {
 	withoutUpdate
 
 	// arguments:
-	cKeys **C.char
+	cKeys cutil.CPtrCSlice
 	cNum  C.size_t
 }
 
 func newRemoveOmapKeysStep(keys []string) *removeOmapKeysStep {
-	cKeys := C.malloc(C.size_t(len(keys)) * ptrSize)
+	cKeys := cutil.NewCPtrCSlice(len(keys))
 	roks := &removeOmapKeysStep{
-		cKeys: (**C.char)(cKeys),
+		cKeys: cKeys,
 		cNum:  C.size_t(len(keys)),
 	}
-	roks.add(cKeys)
 
 	i := 0
 	for _, key := range keys {
-		ckp := (**C.char)(unsafe.Pointer(uintptr(cKeys) + uintptr(i)*ptrSize))
-		*ckp = C.CString(key)
-		roks.add(unsafe.Pointer(*ckp))
+		cKeys[i] = cutil.CPtr(C.CString(key))
+		roks.add(unsafe.Pointer(cKeys[i]))
 		i++
 	}
 
@@ -215,7 +201,7 @@ func newRemoveOmapKeysStep(keys []string) *removeOmapKeysStep {
 }
 
 func (roks *removeOmapKeysStep) free() {
-	roks.cKeys = nil
+	roks.cKeys.Free()
 	roks.withRefs.free()
 }
 

rados/write_op.go

@@ -90,15 +90,15 @@ func (w *WriteOp) Create(exclusive CreateOption) {
 	C.rados_write_op_create(w.op, C.int(exclusive), nil)
 }
 
-//  SetOmap appends the map `pairs` to the omap `oid`.
+// SetOmap appends the map `pairs` to the omap `oid`.
 func (w *WriteOp) SetOmap(pairs map[string][]byte) {
 	sos := newSetOmapStep(pairs)
 	w.steps = append(w.steps, sos)
 	C.rados_write_op_omap_set(
 		w.op,
-		sos.cKeys,
-		sos.cValues,
-		sos.cLengths,
+		(**C.char)(sos.cKeys.Ptr()),
+		(**C.char)(sos.cValues.Ptr()),
+		(*C.size_t)(sos.cLengths.Ptr()),
 		sos.cNum)
 }
 
@@ -108,7 +108,7 @@ func (w *WriteOp) RmOmapKeys(keys []string) {
 	w.steps = append(w.steps, roks)
 	C.rados_write_op_omap_rm_keys(
 		w.op,
-		roks.cKeys,
+		(**C.char)(roks.cKeys.Ptr()),
 		roks.cNum)
 }
 

rbd/mirror.go

@@ -14,9 +14,9 @@ import "C"
 
 import (
 	"fmt"
-	"math"
 	"unsafe"
 
+	"github.com/ceph/go-ceph/internal/cutil"
 	"github.com/ceph/go-ceph/internal/retry"
 	"github.com/ceph/go-ceph/rados"
 )
@@ -364,7 +364,7 @@ func (gmis GlobalMirrorImageStatus) LocalStatus() (SiteMirrorImageStatus, error)
 	return ss, err
 }
 
-type siteArray [math.MaxInt32]C.rbd_mirror_image_site_status_t
+type siteArray [cutil.MaxIdx]C.rbd_mirror_image_site_status_t
 
 // GetGlobalMirrorStatus returns status information pertaining to the state
 // of the images's mirroring.