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...) +}