Repository: ceph/go-ceph
Pull request #426 (2021-01-04 20:08:17)

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.

Based on #428 

Signed-off-by: Sven Anderson <sven@redhat.com>


phlogistonjohn: Also, as I suspect you know, this would conflict with #408. It may be greedy of me to say so, but I would rather we merge that first as it has been open for >6 months in one form or another. This enhancements created by this PR would apply on top of 408 better than trying to adapt 408 after this IMO.
ansiwen: > I like how using a slice avoids having to do a bunch of obnoxious pointer math. That aspect is really great. My main issue is that by "hiding" the C memory behind a slice type we can accidentally lose track of that c memory if you do any operation on the slice that changes it's capacity.

Yes, I'm aware of that and I had the same though, but: You can't change the size or capacity of a slice, it's immutable (in contrary to its payload). What you are actually doing here is _overwriting_ a variable with another slice (via append). That's exactly the same as if you malloc some memory, and then you accidentally overwrite the pointer before calling free, which could happen even more easily than overwriting a slice I would claim. And that's why it's good practice to write a `defer` right in next line, because that will be evaluated with the _current_ value of the receiver, so the Free() will work even if you overwrite it later.


ansiwen: > Also, as I suspect you know, this would conflict with #408. It may be greedy of me to say so, but I would rather we merge that first as it has been open for >6 months in one form or another. This enhancements created by this PR would apply on top of 408 better than trying to adapt 408 after this IMO.

Yes, you are greedy, but I'm a giving person, so no problem. :laughing: No, seriously, all my stuff can easily be rebased and is partly redundant. I'll fix all up when #408 is merged.
mergify[bot]: This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏
ansiwen: @phlogistonjohn Oh, and I just realized: how would the pointer in a struct be protected from overwriting? You could do exactly the same and overwrite the "virtual slice" as you called it with another slice, and the pointer would be lost in the same way. You know, you just have to see a CSlice like you see a CString or a CBytes type, you have to take care to free it, and not overwriting it.
ansiwen: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

ansiwen: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

ansiwen: @Mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

phlogistonjohn: @mergifyio rebase
mergify[bot]: **Command `rebase`: success**
> **Branch has been successfully rebased**
> 

ansiwen: @phlogistonjohn for convenience, here the diff between your reviewed version (7750c01) and the current version (c7d3dd0) modulo rebases: 

```diff
diff --git a/internal/cutil/aliases.go b/internal/cutil/aliases.go
index a9af61a..309c19d 100644
--- a/internal/cutil/aliases.go
+++ b/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
diff --git a/internal/cutil/cslice.go b/internal/cutil/cslice.go
index 2d881e5..e134737 100644
--- a/internal/cutil/cslice.go
+++ b/internal/cutil/cslice.go
@@ -5,17 +5,15 @@ package cutil
 // 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
+// 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).
+// 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
+// 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]
-
-const maxIdx = 1<<31 - 1 // 2GB, max int32 value, should be safe
+// cSlice := (*[MaxIdx]myItem)(myCMemPtr)[:numOfItems:numOfItems]
 
 ////////// CPtr //////////
 
@@ -29,7 +27,7 @@ func NewCPtrCSlice(size int) CPtrCSlice {
 		return nil
 	}
 	cMem := Malloc(SizeT(size) * PtrSize)
-	cSlice := (*[maxIdx]CPtr)(cMem)[:size:size]
+	cSlice := (*[MaxIdx]CPtr)(cMem)[:size:size]
 	return cSlice
 }
 
@@ -59,7 +57,7 @@ func NewSizeTCSlice(size int) SizeTCSlice {
 		return nil
 	}
 	cMem := Malloc(SizeT(size) * SizeTSize)
-	cSlice := (*[maxIdx]SizeT)(cMem)[:size:size]
+	cSlice := (*[MaxIdx]SizeT)(cMem)[:size:size]
 	return cSlice
 }
 
diff --git a/internal/cutil/cslice_test.go b/internal/cutil/cslice_test.go
index 7913376..ba02db5 100644
--- a/internal/cutil/cslice_test.go
+++ b/internal/cutil/cslice_test.go
@@ -19,6 +19,8 @@ func TestCPtrCSlice(t *testing.T) {
 		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++ {
diff --git a/rados/omap.go b/rados/omap.go
index e5eb191..bebd6ce 100644
--- a/rados/omap.go
+++ b/rados/omap.go
@@ -17,11 +17,6 @@ import (
 	"github.com/ceph/go-ceph/internal/cutil"
 )
 
-const (
-	ptrSize   = C.sizeof_voidptr
-	sizeTSize = C.sizeof_size_t
-)
-
 // setOmapStep is a write op step. It holds C memory used in the operation.
 type setOmapStep struct {
 	withRefs
diff --git a/rbd/mirror.go b/rbd/mirror.go
index ddfe47e..6de3024 100644
--- a/rbd/mirror.go
+++ b/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.
```

Patch:
diff --git a/internal/cutil/aliases.go b/internal/cutil/aliases.go
index a9af61a..309c19d 100644
--- a/internal/cutil/aliases.go
+++ b/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
diff --git a/internal/cutil/cslice.go b/internal/cutil/cslice.go
new file mode 100644
index 0000000..e134737
--- /dev/null
+++ b/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
+}
diff --git a/rados/omap.go b/rados/omap.go
index 79b4c8e..e71f84b 100644
--- a/rados/omap.go
+++ b/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()
 }
 
diff --git a/rados/write_op.go b/rados/write_op.go
index 0fab210..cdb838d 100644
--- a/rados/write_op.go
+++ b/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)
 }
 
diff --git a/rbd/mirror.go b/rbd/mirror.go
index ddfe47e..6de3024 100644
--- a/rbd/mirror.go
+++ b/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.


Test patch:
diff --git a/internal/cutil/cslice_test.go b/internal/cutil/cslice_test.go
new file mode 100644
index 0000000..ba02db5
--- /dev/null
+++ b/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() })
+	})
+}