cgo: add centralized pointer handle manager

This also fixes a potential misuse of passing Go pointers to C.

Author: changkun

clipboard_linux.c

@@ -10,6 +10,9 @@
 #include <X11/Xlib.h>
 #include <X11/Xatom.h>
 
+// syncStatus is a function from the Go side.
+extern void syncStatus(unsigned long long handle, int status);
+
 int clipboard_test() {
     Display *d = XOpenDisplay(0);
     if (d == NULL) {
@@ -22,10 +25,10 @@ int clipboard_test() {
 // clipboard_write writes the given buf of size n as type typ.
 // if start is provided, the value of start will be changed to 1 to indicate
 // if the write is availiable for reading.
-int clipboard_write(char *typ, unsigned char *buf, size_t n, int *start) {
+int clipboard_write(char *typ, unsigned char *buf, size_t n, unsigned long long handle) {
     Display* d = XOpenDisplay(0);
     if (d == NULL) {
-        if (start != NULL && *start == 0) *start = -1;
+        if (handle != 0) syncStatus(handle, -1);
         return -1;
     }
 
@@ -41,24 +44,25 @@ int clipboard_write(char *typ, unsigned char *buf, size_t n, int *start) {
     Atom target = XInternAtom(d, typ, True);
     if (target == None) {
         XCloseDisplay(d);
-        if (start != NULL && *start == 0) *start = -2;
+        if (handle != 0) syncStatus(handle, -2);
         return -2;
     }
 
     XSetSelectionOwner(d, sel, w, CurrentTime);
     if (XGetSelectionOwner(d, sel) != w) {
         XCloseDisplay(d);
-        if (start != NULL && *start == 0) *start = -3;
+        if (handle != 0) syncStatus(handle, -3);
         return -3;
     }
 
     XEvent event;
     XSelectionRequestEvent* xsr;
+    int notified = 0;
     for (;;) {
-        // FIXME: this should race with the code on the Go side, start
-        // should use an atomic version, and use atomic_store.
-        if (start != NULL && *start == 0) 
-            *start = 1; // notify Go side
+        if (handle != 0 && notified == 0) { 
+            syncStatus(handle, 1); // notify Go side
+            notified = 1;
+        }
 
         XNextEvent(d, &event);
         switch (event.type) {

clipboard_linux.go

@@ -23,7 +23,7 @@ int clipboard_write(
 	char*          typ,
 	unsigned char* buf,
 	size_t         n,
-	int*           start // FIXME: should use atomic
+	unsigned long long handle
 );
 unsigned long clipboard_read(char* typ, char **out);
 */
@@ -36,6 +36,8 @@ import (
 	"runtime"
 	"time"
 	"unsafe"
+
+	"golang.design/x/clipboard/internal/cgo"
 )
 
 func init() {
@@ -90,7 +92,7 @@ func write(t Format, buf []byte) (<-chan struct{}, error) {
 		s = "image/png"
 	}
 
-	var start C.int
+	start := make(chan int)
 	done := make(chan struct{}, 1)
 
 	go func() { // surve as a daemon until the ownership is terminated.
@@ -100,11 +102,12 @@ func write(t Format, buf []byte) (<-chan struct{}, error) {
 		cs := C.CString(s)
 		defer C.free(unsafe.Pointer(cs))
 
+		h := cgo.NewHandle(start)
 		var ok C.int
 		if len(buf) == 0 {
-			ok = C.clipboard_write(cs, nil, 0, &start)
+			ok = C.clipboard_write(cs, nil, 0, C.ulonglong(h))
 		} else {
-			ok = C.clipboard_write(cs, (*C.uchar)(unsafe.Pointer(&(buf[0]))), C.size_t(len(buf)), &start)
+			ok = C.clipboard_write(cs, (*C.uchar)(unsafe.Pointer(&(buf[0]))), C.size_t(len(buf)), C.ulonglong(h))
 		}
 		if ok != C.int(0) {
 			fmt.Fprintf(os.Stderr, "write failed with status: %d\n", int(ok))
@@ -113,12 +116,8 @@ func write(t Format, buf []byte) (<-chan struct{}, error) {
 		close(done)
 	}()
 
-	// FIXME: this should race with the code on the C side, start
-	// should use an atomic version, and use atomic_load.
-	for start == 0 {
-	}
-
-	if start < 0 {
+	status := <-start
+	if status < 0 {
 		return nil, errInvalidOperation
 	}
 	// wait until enter event loop
@@ -149,3 +148,14 @@ func watch(ctx context.Context, t Format) <-chan []byte {
 	}()
 	return recv
 }
+
+type syncChan struct {
+	c chan int
+}
+
+//export syncStatus
+func syncStatus(h uintptr, val int) {
+	v := cgo.Handle(h).Value().(chan int)
+	v <- val
+	cgo.Handle(h).Delete()
+}

internal/cgo/handle.go

@@ -0,0 +1,143 @@
+// Copyright 2021 The golang.design Initiative Authors.
+// All rights reserved. Use of this source code is governed
+// by a MIT license that can be found in the LICENSE file.
+//
+// Written by Changkun Ou <changkun.de>
+
+// Package cgo is an implementation of golang.org/issue/37033.
+//
+// See golang.org/cl/294670 for code review discussion.
+package cgo
+
+import (
+	"reflect"
+	"sync"
+)
+
+// Handle provides a safe representation to communicate Go values between
+// C and Go. The zero value of a handle is not a valid handle, and thus
+// safe to use as a sentinel in C APIs.
+//
+// The underlying type of Handle may change, but the value is guaranteed
+// to fit in an integer type that is large enough to hold the bit pattern
+// of any pointer. For instance, on the Go side:
+//
+// 	package main
+//
+// 	/*
+// 	extern void GoPrint(unsigned long long handle);
+// 	void printFromGo(unsigned long long handle);
+// 	*/
+// 	import "C"
+// 	import "runtime/cgo"
+//
+// 	//export GoPrint
+// 	func GoPrint(handle C.ulonglong) {
+// 		h := cgo.Handle(handle)
+// 		val := h.Value().(int)
+// 		println(val)
+// 		h.Delete()
+// 	}
+//
+// 	func main() {
+// 		val := 42
+//
+// 		C.printFromGo(C.ulonglong(cgo.NewHandle(val))) // prints 42
+// 	}
+//
+// and on the C side:
+//
+// 	// This function is from Go side.
+// 	extern void GoPrint(unsigned long long handle);
+//
+// 	// A C function
+// 	void printFromGo(unsigned long long handle) {
+// 	    GoPrint(handle);
+// 	}
+type Handle uintptr
+
+// NewHandle returns a handle for a given value. If a given value is a
+// pointer, slice, map, channel, or function that refers to the same
+// object, the returned handle will also be the same. Besides, nil value
+// must not be used.
+//
+// The handle is valid until the program calls Delete on it. The handle
+// uses resources, and this package assumes that C code may hold on to
+// the handle, so a program must explicitly call Delete when the handle
+// is no longer needed.
+//
+// The intended use is to pass the returned handle to C code, which
+// passes it back to Go, which calls Value. See an example in the
+// comments of the Handle definition.
+func NewHandle(v interface{}) Handle {
+	var k uintptr
+
+	rv := reflect.ValueOf(v)
+	switch rv.Kind() {
+	case reflect.Ptr, reflect.UnsafePointer, reflect.Slice,
+		reflect.Map, reflect.Chan, reflect.Func:
+		if rv.IsNil() {
+			panic("cannot use handle for nil value")
+		}
+
+		k = rv.Pointer()
+	default:
+		k = reflect.ValueOf(&v).Pointer()
+	}
+
+	// v escapes to the heap, always. As Go do not have a moving GC (and
+	// possibly lasts true for a long future), it is safe to use its
+	// pointer address as the key of the global map at this moment.
+	// The implementation must be reconsidered if moving GC is
+	// introduced internally.
+	actual, loaded := m.LoadOrStore(k, v)
+	if !loaded {
+		return Handle(k)
+	}
+	arv := reflect.ValueOf(actual)
+	switch arv.Kind() {
+	case reflect.Ptr, reflect.UnsafePointer, reflect.Slice,
+		reflect.Map, reflect.Chan, reflect.Func:
+		// The underlying object of the given Go value already have
+		// its existing handle.
+		if arv.Pointer() == k {
+			return Handle(k)
+		}
+
+		// If the loaded actual value is inconsistent with the new
+		// value, it means the address has been used for different
+		// objects, and we should fallthrough, see comments below.
+		fallthrough
+	default:
+		// If a Go value is garbage collected and its address is reused
+		// for a new Go value, meaning that the Handle does not call
+		// Delete explicitly when the old Go value is not needed.
+		// Consider this as a misuse of a handle, do panic.
+		panic("misuse of a handle")
+	}
+}
+
+// Delete invalidates a handle. This method must be called when C code no
+// longer has a copy of the handle, and the program no longer needs the
+// Go value that associated with the handle.
+//
+// The method panics if the handle is invalid already.
+func (h Handle) Delete() {
+	_, ok := m.LoadAndDelete(uintptr(h))
+	if !ok {
+		panic("misuse of a handle")
+	}
+}
+
+// Value returns the associated Go value for a valid handle.
+//
+// The method panics if the handle is invalid already.
+func (h Handle) Value() interface{} {
+	v, ok := m.Load(uintptr(h))
+	if !ok {
+		panic("misuse of a handle")
+	}
+	return v
+}
+
+var m = &sync.Map{} // map[uintptr]interface{}

internal/cgo/handle_test.go

@@ -0,0 +1,124 @@
+package cgo
+
+import (
+	"testing"
+)
+
+func TestValueHandle(t *testing.T) {
+	v := 42
+
+	h1 := NewHandle(v)
+	h2 := NewHandle(v)
+
+	if uintptr(h1) == uintptr(h2) {
+		t.Fatalf("duplicated Go values should have different handles")
+	}
+
+	h1v := h1.Value().(int)
+	h2v := h2.Value().(int)
+	if h1v != h2v {
+		t.Fatalf("the Value of duplicated Go values are different: want %d, got %d", h1v, h2v)
+	}
+	if h1v != v {
+		t.Fatalf("the Value of a handle does not match origin: want %v, got %v", v, h1v)
+	}
+
+	h1.Delete()
+	h2.Delete()
+
+	siz := 0
+	m.Range(func(k, v interface{}) bool {
+		siz++
+		return true
+	})
+	if siz != 0 {
+		t.Fatalf("handles are not deleted, want: %d, got %d", 0, siz)
+	}
+}
+
+func TestPointerHandle(t *testing.T) {
+	v := 42
+
+	p1 := &v
+	p2 := &v
+
+	h1 := NewHandle(p1)
+	h2 := NewHandle(p2)
+
+	if uintptr(h1) != uintptr(h2) {
+		t.Fatalf("pointers to the same value should have same handle")
+	}
+
+	h1v := h1.Value().(*int)
+	h2v := h2.Value().(*int)
+	if h1v != h2v {
+		t.Fatalf("the Value of a handle does not match origin: want %v, got %v", v, h1v)
+	}
+
+	h1.Delete()
+
+	siz := 0
+	m.Range(func(k, v interface{}) bool {
+		siz++
+		return true
+	})
+	if siz != 0 {
+		t.Fatalf("handles are not deleted: want %d, got %d", 0, siz)
+	}
+
+	defer func() {
+		if r := recover(); r != nil {
+			return
+		}
+		t.Fatalf("double Delete on a same handle did not trigger a panic")
+	}()
+
+	h2.Delete()
+}
+
+func TestNilHandle(t *testing.T) {
+	var v *int
+
+	defer func() {
+		if r := recover(); r != nil {
+			return
+		}
+		t.Fatalf("nil should not be created as a handle successfully")
+	}()
+
+	_ = NewHandle(v)
+}
+
+func f1() {}
+func f2() {}
+
+type foo struct{}
+
+func (f *foo) bar() {}
+func (f *foo) wow() {}
+
+func TestFuncHandle(t *testing.T) {
+	h1 := NewHandle(f1)
+	h2 := NewHandle(f2)
+	h3 := NewHandle(f2)
+
+	if h1 == h2 {
+		t.Fatalf("different functions should have different handles")
+	}
+	if h2 != h3 {
+		t.Fatalf("same functions should have same handles")
+	}
+
+	f := foo{}
+	h4 := NewHandle(f.bar)
+	h5 := NewHandle(f.bar)
+	h6 := NewHandle(f.wow)
+
+	if h4 != h5 {
+		t.Fatalf("same methods should have same handles")
+	}
+
+	if h5 == h6 {
+		t.Fatalf("different methods should have different handles")
+	}
+}