bind/java: pin java byte array elements until Seq send is done.

When passing a byte array from Java to Go, Seq.writeByteArray JNI
call encodes only the array size and the pointer to the array.
Go-side receives the (size, ptr) pair info during the subsequent
Seq.send JNI call, and copies the elements into a Go byte slice.
We must pin the array elements until Go-side completes copying
so that they are not moved or collected by Java runtime.

This change keeps track of the pinned array info in a 'pinned' linked
list, and unpin them as the Seq memory is freed.  The jbyteArray
argument passed to Seq.writeByteArray is needed to release the pinned
byte array elements, but that is a "local reference". It is not
guaranteed that the reference is valid after the method returns. Thus,
we stash its global reference in the 'pinned' list and delete it later
as well.

A similar problem can occur on the byte slice returned from a Go function.
This change does not address the case yet.

Fixes golang/go#9486

Change-Id: I1255aefbc80b21ccbe9b2bf37699faaf0c5f0bae
Reviewed-on: https://go-review.googlesource.com/2586
Reviewed-by: David Crawshaw <[email protected]>

Author: hyangah

bind/java/SeqTest.java

@@ -81,6 +81,27 @@ public void testByteArray() {
     }
   }
 
+  // Test for golang.org/issue/9486.
+  public void testByteArrayAfterString() {
+    byte[] bytes = new byte[1024];
+    for (int i=0; i < bytes.length; i++) {
+           bytes[i] = 8;
+    }
+
+    String stuff = "stuff";
+    byte[] got = Testpkg.AppendToString(stuff, bytes);
+
+    try {
+      byte[] s = stuff.getBytes("UTF-8");
+      byte[] want = new byte[s.length + bytes.length];
+      System.arraycopy(s, 0, want, 0, s.length);
+      System.arraycopy(bytes, 0, want, s.length, bytes.length);
+      MoreAsserts.assertEquals("Bytes should match", want, got);
+    } catch (Exception e) {
+      fail("Cannot perform the test: " + e.toString());
+    }
+  }
+
   public void testGoRefGC() {
     Testpkg.S s = Testpkg.New();
     runGC();

bind/java/seq_android.c

@@ -19,6 +19,15 @@ static jfieldID receive_refnum_id;
 static jfieldID receive_code_id;
 static jfieldID receive_handle_id;
 
+static jclass jbytearray_clazz;
+
+// pinned represents a pinned array to be released at the end of Send call.
+typedef struct pinned {
+	jobject ref;
+	void* ptr;
+	struct pinned* next;
+} pinned;
+
 // mem is a simple C equivalent of seq.Buffer.
 //
 // Many of the allocations around mem could be avoided to improve
@@ -28,6 +37,9 @@ typedef struct mem {
 	uint32_t off;
 	uint32_t len;
 	uint32_t cap;
+
+	// TODO(hyangah): have it as a separate field outside mem?
+	pinned* pinned;
 } mem;
 
 // mem_ensure ensures that m has at least size bytes free.
@@ -42,6 +54,7 @@ static mem *mem_ensure(mem *m, uint32_t size) {
 		m->off = 0;
 		m->len = 0;
 		m->buf = NULL;
+		m->pinned = NULL;
 	}
 	if (m->cap > m->off+size) {
 		return m;
@@ -95,6 +108,47 @@ uint8_t *mem_write(JNIEnv *env, jobject obj, uint32_t size) {
 	return res;
 }
 
+static void *pin_array(JNIEnv *env, jobject obj, jobject arr) {
+	mem *m = mem_get(env, obj);
+	if (m == NULL) {
+		m = mem_ensure(m, 64);
+	}
+	pinned *p = (pinned*) malloc(sizeof(pinned));
+	if (p == NULL) {
+		LOG_FATAL("pin_array malloc failed");
+	}
+	p->ref = (*env)->NewGlobalRef(env, arr);
+
+	if ((*env)->IsInstanceOf(env, p->ref, jbytearray_clazz)) {
+		p->ptr = (*env)->GetByteArrayElements(env, p->ref, NULL);
+	} else {
+		LOG_FATAL("unsupported array type");
+	}
+
+	p->next = m->pinned;
+	m->pinned = p;
+	return p->ptr;
+}
+
+static void unpin_arrays(JNIEnv *env, mem *m) {
+	pinned* p = m->pinned;
+	while (p != NULL) {
+		if ((*env)->IsInstanceOf(env, p->ref, jbytearray_clazz)) {
+			(*env)->ReleaseByteArrayElements(env, p->ref, (jbyte*)p->ptr, JNI_ABORT);
+		} else {
+			LOG_FATAL("invalid array type");
+		}
+
+		(*env)->DeleteGlobalRef(env, p->ref);
+
+		pinned* o = p;
+		p = p->next;
+		free(o);
+	}
+	m->pinned = NULL;
+}
+
+
 static jfieldID find_field(JNIEnv *env, const char *class_name, const char *field_name, const char *field_type) {
 	jclass clazz = (*env)->FindClass(env, class_name);
 	if (clazz == NULL) {
@@ -109,6 +163,15 @@ static jfieldID find_field(JNIEnv *env, const char *class_name, const char *fiel
 	return id;
 }
 
+static jclass find_class(JNIEnv *env, const char *class_name) {
+	jclass clazz = (*env)->FindClass(env, class_name);
+	if (clazz == NULL) {
+		LOG_FATAL("cannot find %s", class_name);
+		return NULL;
+	}
+	return (*env)->NewGlobalRef(env, clazz);
+}
+
 void init_seq(void *javavm) {
 	JavaVM *vm = (JavaVM*)javavm;
 	JNIEnv *env;
@@ -128,6 +191,8 @@ void init_seq(void *javavm) {
 	receive_handle_id = find_field(env, "go/Seq$Receive", "handle", "I");
 	receive_code_id = find_field(env, "go/Seq$Receive", "code", "I");
 
+	jbytearray_clazz = find_class(env, "[B");
+
 	LOG_INFO("loaded go/Seq");
 
 	if (res == JNI_EDETACHED) {
@@ -148,6 +213,7 @@ JNIEXPORT void JNICALL
 Java_go_Seq_free(JNIEnv *env, jobject obj) {
 	mem *m = mem_get(env, obj);
 	if (m != NULL) {
+		unpin_arrays(env, m);
 		free((void*)m->buf);
 		free((void*)m);
 	}
@@ -276,17 +342,8 @@ Java_go_Seq_writeByteArray(JNIEnv *env, jobject obj, jbyteArray v) {
 		return;
 	}
 
-	jboolean isCopy;
-	jbyte* b = (*env)->GetByteArrayElements(env, v, &isCopy);
-	if (isCopy) {
-		// TODO: It's not clear how to handle if b is pointing to
-		// a copy that may become invalid with ReleaseByteArrayElements.
-		// Should we fall back to copy the byte array into the buffer?
-		LOG_FATAL("got a copied byte array (len=%d)", len);
-	}
-	// gross pointer-to-int64 conversion.
-	MEM_WRITE(int64_t) = (int64_t)((intptr_t)b);
-	(*env)->ReleaseByteArrayElements(env, v, (jbyte*)b, 0);
+	jbyte* b = pin_array(env, obj, v);
+	MEM_WRITE(int64_t) = (jlong)(uintptr_t)b;
 }
 
 JNIEXPORT void JNICALL
@@ -337,6 +394,7 @@ Java_go_Seq_send(JNIEnv *env, jclass clazz, jstring descriptor, jint code, jobje
 	desc.n = (*env)->GetStringUTFLength(env, descriptor);
 	Send(desc, (GoInt)code, src->buf, src->len, &dst->buf, &dst->len);
 	(*env)->ReleaseStringUTFChars(env, descriptor, desc.p);
+	unpin_arrays(env, src);  // assume 'src' is no longer needed.
 }
 
 JNIEXPORT void JNICALL

bind/java/seq_android.go

@@ -36,6 +36,11 @@ func Send(descriptor string, code int, req *C.uint8_t, reqlen C.size_t, res **C.
 	}
 	out := new(seq.Buffer)
 	fn(out, in)
+	// BUG(hyangah): the function returning a go byte slice (so fn writes a pointer into 'out') is unsafe.
+	// After fn is complete here, Go runtime is free to collect or move the pointed byte slice
+	// contents. (Explicitly calling runtime.GC here will surface the problem?)
+	// Without pinning support from Go side, it will be hard to fix it without extra copying.
+
 	seqToBuf(res, reslen, out)
 }
 

bind/java/testpkg/Testpkg.java

@@ -20,6 +20,17 @@ public static long Add(long x, long y) {
         return _result;
     }
 
+    public static byte[] AppendToString(String str, byte[] someBytes) {
+        go.Seq _in = new go.Seq();
+        go.Seq _out = new go.Seq();
+        byte[] _result;
+        _in.writeUTF16(str);
+        _in.writeByteArray(someBytes);
+        Seq.send(DESCRIPTOR, CALL_AppendToString, _in, _out);
+        _result = _out.readByteArray();
+        return _result;
+    }
+    
     public static byte[] BytesAppend(byte[] a, byte[] b) {
         go.Seq _in = new go.Seq();
         go.Seq _out = new go.Seq();
@@ -372,18 +383,19 @@ public static String StrDup(String s) {
     }
 
     private static final int CALL_Add = 1;
-    private static final int CALL_BytesAppend = 2;
-    private static final int CALL_CallE = 3;
-    private static final int CALL_CallF = 4;
-    private static final int CALL_CallI = 5;
-    private static final int CALL_CallS = 6;
-    private static final int CALL_CallV = 7;
-    private static final int CALL_CallVE = 8;
-    private static final int CALL_Err = 9;
-    private static final int CALL_GC = 10;
-    private static final int CALL_Keep = 11;
-    private static final int CALL_New = 12;
-    private static final int CALL_NumSCollected = 13;
-    private static final int CALL_StrDup = 14;
+    private static final int CALL_AppendToString = 2;
+    private static final int CALL_BytesAppend = 3;
+    private static final int CALL_CallE = 4;
+    private static final int CALL_CallF = 5;
+    private static final int CALL_CallI = 6;
+    private static final int CALL_CallS = 7;
+    private static final int CALL_CallV = 8;
+    private static final int CALL_CallVE = 9;
+    private static final int CALL_Err = 10;
+    private static final int CALL_GC = 11;
+    private static final int CALL_Keep = 12;
+    private static final int CALL_New = 13;
+    private static final int CALL_NumSCollected = 14;
+    private static final int CALL_StrDup = 15;
     private static final String DESCRIPTOR = "testpkg";
 }

bind/java/testpkg/go_testpkg/go_testpkg.go

@@ -16,6 +16,13 @@ func proxy_Add(out, in *seq.Buffer) {
 	out.WriteInt(res)
 }
 
+func proxy_AppendToString(out, in *seq.Buffer) {
+	param_str := in.ReadUTF16()
+	param_someBytes := in.ReadByteArray()
+	res := testpkg.AppendToString(param_str, param_someBytes)
+	out.WriteByteArray(res)
+}
+
 func proxy_BytesAppend(out, in *seq.Buffer) {
 	param_a := in.ReadByteArray()
 	param_b := in.ReadByteArray()
@@ -308,17 +315,18 @@ func proxy_StrDup(out, in *seq.Buffer) {
 
 func init() {
 	seq.Register("testpkg", 1, proxy_Add)
-	seq.Register("testpkg", 2, proxy_BytesAppend)
-	seq.Register("testpkg", 3, proxy_CallE)
-	seq.Register("testpkg", 4, proxy_CallF)
-	seq.Register("testpkg", 5, proxy_CallI)
-	seq.Register("testpkg", 6, proxy_CallS)
-	seq.Register("testpkg", 7, proxy_CallV)
-	seq.Register("testpkg", 8, proxy_CallVE)
-	seq.Register("testpkg", 9, proxy_Err)
-	seq.Register("testpkg", 10, proxy_GC)
-	seq.Register("testpkg", 11, proxy_Keep)
-	seq.Register("testpkg", 12, proxy_New)
-	seq.Register("testpkg", 13, proxy_NumSCollected)
-	seq.Register("testpkg", 14, proxy_StrDup)
+	seq.Register("testpkg", 2, proxy_AppendToString)
+	seq.Register("testpkg", 3, proxy_BytesAppend)
+	seq.Register("testpkg", 4, proxy_CallE)
+	seq.Register("testpkg", 5, proxy_CallF)
+	seq.Register("testpkg", 6, proxy_CallI)
+	seq.Register("testpkg", 7, proxy_CallS)
+	seq.Register("testpkg", 8, proxy_CallV)
+	seq.Register("testpkg", 9, proxy_CallVE)
+	seq.Register("testpkg", 10, proxy_Err)
+	seq.Register("testpkg", 11, proxy_GC)
+	seq.Register("testpkg", 12, proxy_Keep)
+	seq.Register("testpkg", 13, proxy_New)
+	seq.Register("testpkg", 14, proxy_NumSCollected)
+	seq.Register("testpkg", 15, proxy_StrDup)
 }

bind/java/testpkg/testpkg.go

@@ -108,3 +108,9 @@ func Err(s string) error {
 func BytesAppend(a []byte, b []byte) []byte {
 	return append(a, b...)
 }
+
+func AppendToString(str string, someBytes []byte) []byte {
+	a := []byte(str)
+	fmt.Printf("str=%q (len=%d), someBytes=%v (len=%d)\n", str, len(str), someBytes, len(someBytes))
+	return append(a, someBytes...)
+}