From 71e2276663b98ae4634b2f9186bba50fdc1dca09 Mon Sep 17 00:00:00 2001 From: "Hyang-Ah (Hana) Kim" Date: Thu, 8 Jan 2015 13:07:46 -0500 Subject: [PATCH] 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 --- bind/java/SeqTest.java | 21 ++++++ bind/java/seq_android.c | 80 +++++++++++++++++++--- bind/java/seq_android.go | 5 ++ bind/java/testpkg/Testpkg.java | 38 ++++++---- bind/java/testpkg/go_testpkg/go_testpkg.go | 34 +++++---- bind/java/testpkg/testpkg.go | 6 ++ 6 files changed, 147 insertions(+), 37 deletions(-) diff --git a/bind/java/SeqTest.java b/bind/java/SeqTest.java index fedd00b..e0d8373 100644 --- a/bind/java/SeqTest.java +++ b/bind/java/SeqTest.java @@ -81,6 +81,27 @@ public class SeqTest extends TestCase { } } + // 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(); diff --git a/bind/java/seq_android.c b/bind/java/seq_android.c index f0c9bfd..bd32c82 100644 --- a/bind/java/seq_android.c +++ b/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 diff --git a/bind/java/seq_android.go b/bind/java/seq_android.go index 59102db..64fdec4 100644 --- a/bind/java/seq_android.go +++ b/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) } diff --git a/bind/java/testpkg/Testpkg.java b/bind/java/testpkg/Testpkg.java index aa21952..d4fd2b9 100644 --- a/bind/java/testpkg/Testpkg.java +++ b/bind/java/testpkg/Testpkg.java @@ -20,6 +20,17 @@ public abstract class Testpkg { 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 abstract class Testpkg { } 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"; } diff --git a/bind/java/testpkg/go_testpkg/go_testpkg.go b/bind/java/testpkg/go_testpkg/go_testpkg.go index c3229ac..9762a42 100644 --- a/bind/java/testpkg/go_testpkg/go_testpkg.go +++ b/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) } diff --git a/bind/java/testpkg/testpkg.go b/bind/java/testpkg/testpkg.go index 850fcf2..c4bde1b 100644 --- a/bind/java/testpkg/testpkg.go +++ b/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...) +}