Do not use Go pointers containing Go pointers in a CGo call

https://github.com/golang/proposal/blob/master/design/12416-cgo-pointers.md

3 files changed, 65 insertions, 4 deletions

diff --git a/librsync/librsync.go b/librsync/librsync.go
index 234b1f3..641e480 100644
--- a/librsync/librsync.go
+++ b/librsync/librsync.go
@@ -275,7 +275,7 @@ func NewDeltaGen(sig Signature, newfile io.Reader) (job *Job, err error) {
 
 // Patcher is a job with additional hidden data for patching.
 //
-// IMPORTANT: You still need to Close() this!
+// This patcher must be closed after use to free memory.
 type Patcher struct {
 	*Job
 	basis io.ReaderAt
@@ -299,11 +299,21 @@ func NewPatcher(delta io.Reader, basis io.ReaderAt) (job *Patcher, err error) {
 		Job:   _job,
 		basis: basis}
 
-	job.job = C.rs_patch_begin((*C.rs_copy_cb)(patchCallback), unsafe.Pointer(job))
+	id := uintptr(unsafe.Pointer(_job.rsbufs)) // this is a unique, unchanging number (C doesn't change pointers under the hood)
+	storePatcher(job, id)
+	job.job = C.rs_patch_begin((*C.rs_copy_cb)(patchCallback), unsafe.Pointer(id))
 	if job.job == nil {
+		dropPatcher(id)
 		job.Close()
 		return nil, errors.New("rs_patch_begin failed")
 	}
 
 	return
 }
+
+// Close unreferences memory that the garbage collector would not otherwise be
+// able to free.
+func (patch *Patcher) Close() error {
+	dropPatcher(uintptr(unsafe.Pointer(patch.Job.rsbufs)))
+	return patch.Job.Close()
+}
diff --git a/librsync/librsync_callback.go b/librsync/librsync_callback.go
index c8b9e4f..ace9779 100644
--- a/librsync/librsync_callback.go
+++ b/librsync/librsync_callback.go
@@ -12,8 +12,8 @@ import (
 )
 
 //export patchCallbackGo
-func patchCallbackGo(_patcher unsafe.Pointer, pos C.rs_long_t, len *C.size_t, _buf *unsafe.Pointer) C.rs_result {
-	patcher := (*Patcher)(_patcher)
+func patchCallbackGo(_patcher uintptr, pos C.rs_long_t, len *C.size_t, _buf *unsafe.Pointer) C.rs_result {
+	patcher := getPatcher(_patcher)
 
 	patcher.buf = make([]byte, int(*len))
 	n, err := patcher.basis.ReadAt(patcher.buf, int64(pos))
diff --git a/librsync/pointers.go b/librsync/pointers.go
new file mode 100644
index 0000000..d14f41f
--- /dev/null
+++ b/librsync/pointers.go
@@ -0,0 +1,51 @@
+package librsync
+
+import (
+	"sync"
+)
+
+// pointerMap holds Go *Patcher objects to pass to C.
+// Don't touch this data structure, instead use the storePatcher, getPatcher,
+// and dropPatcher functions.
+var patcherStore = struct {
+	lock  sync.Mutex
+	store map[uintptr]*Patcher
+}{
+	store: make(map[uintptr]*Patcher),
+}
+
+// storePatcher stores the value and returns a reference to it, for use in a CGo
+// call. Use the same reference for dropPatcher. C callbacks can use getPatcher
+// to get the original value.
+func storePatcher(patcher *Patcher, id uintptr) {
+	patcherStore.lock.Lock()
+	defer patcherStore.lock.Unlock()
+
+	if _, ok := patcherStore.store[id]; ok {
+		// Just to be on the safe side.
+		panic("pointer already stored")
+	}
+	patcherStore.store[id] = patcher
+}
+
+// getPatcher returns the value for the reference id. It returns nil if there
+// is no such reference.
+func getPatcher(id uintptr) *Patcher {
+	patcherStore.lock.Lock()
+	defer patcherStore.lock.Unlock()
+
+	return patcherStore.store[id]
+}
+
+// dropPatcher unreferences the value so the garbage collector can free it's
+// memory.
+func dropPatcher(id uintptr) {
+	patcherStore.lock.Lock()
+	defer patcherStore.lock.Unlock()
+
+	if _, ok := patcherStore.store[id]; !ok {
+		panic("pointer not stored")
+	}
+
+	delete(patcherStore.store, id)
+}