8258077: Using -Xcheck:jni can lead to a double-free after JDK-8193234

David Holmes · David Holmes · commit 712014c5956c · 2021-01-08T04:11:22.000Z
Reviewed-by: dcubed, hseigel
diff --git a/src/hotspot/share/prims/jniCheck.cpp b/src/hotspot/share/prims/jniCheck.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -43,6 +43,7 @@
 #include "runtime/jfieldIDWorkaround.hpp"
 #include "runtime/jniHandles.inline.hpp"
 #include "runtime/thread.inline.hpp"
+#include "utilities/formatBuffer.hpp"
 #include "utilities/utf8.hpp"
 
 // Complain every extra number of unplanned local refs
@@ -399,19 +400,16 @@ static void* check_wrapped_array(JavaThread* thr, const char* fn_name,
   GuardedMemory guarded(carray);
   void* orig_result = guarded.get_tag();
   if (!guarded.verify_guards()) {
-    tty->print_cr("ReleasePrimitiveArrayCritical: release array failed bounds "
-        "check, incorrect pointer returned ? array: " PTR_FORMAT " carray: "
-        PTR_FORMAT, p2i(obj), p2i(carray));
-    guarded.print_on(tty);
-    NativeReportJNIFatalError(thr, "ReleasePrimitiveArrayCritical: "
-        "failed bounds check");
+    tty->print_cr("%s: release array failed bounds check, incorrect pointer returned ? array: "
+                  PTR_FORMAT " carray: " PTR_FORMAT, fn_name, p2i(obj), p2i(carray));
+    DEBUG_ONLY(guarded.print_on(tty);) // This may crash.
+    NativeReportJNIFatalError(thr, err_msg("%s: failed bounds check", fn_name));
   }
   if (orig_result == NULL) {
-    tty->print_cr("ReleasePrimitiveArrayCritical: unrecognized elements. array: "
-        PTR_FORMAT " carray: " PTR_FORMAT, p2i(obj), p2i(carray));
-    guarded.print_on(tty);
-    NativeReportJNIFatalError(thr, "ReleasePrimitiveArrayCritical: "
-        "unrecognized elements");
+    tty->print_cr("%s: unrecognized elements. array: " PTR_FORMAT " carray: " PTR_FORMAT,
+                  fn_name, p2i(obj), p2i(carray));
+    DEBUG_ONLY(guarded.print_on(tty);) // This may crash.
+    NativeReportJNIFatalError(thr, err_msg("%s: unrecognized elements", fn_name));
   }
   if (rsz != NULL) {
     *rsz = guarded.get_user_size();
@@ -420,24 +418,30 @@ static void* check_wrapped_array(JavaThread* thr, const char* fn_name,
 }
 
 static void* check_wrapped_array_release(JavaThread* thr, const char* fn_name,
-    void* obj, void* carray, jint mode) {
+                                         void* obj, void* carray, jint mode, jboolean is_critical) {
   size_t sz;
   void* orig_result = check_wrapped_array(thr, fn_name, obj, carray, &sz);
   switch (mode) {
-  // As we never make copies, mode 0 and JNI_COMMIT are the same.
   case 0:
+    memcpy(orig_result, carray, sz);
+    GuardedMemory::free_copy(carray);
+    break;
   case JNI_COMMIT:
     memcpy(orig_result, carray, sz);
+    if (is_critical) {
+      // For ReleasePrimitiveArrayCritical we must free the internal buffer
+      // allocated through GuardedMemory.
+      GuardedMemory::free_copy(carray);
+    }
     break;
   case JNI_ABORT:
+    GuardedMemory::free_copy(carray);
     break;
   default:
     tty->print_cr("%s: Unrecognized mode %i releasing array "
-        PTR_FORMAT " elements " PTR_FORMAT, fn_name, mode, p2i(obj), p2i(carray));
+                  PTR_FORMAT " elements " PTR_FORMAT, fn_name, mode, p2i(obj), p2i(carray));
     NativeReportJNIFatalError(thr, "Unrecognized array release mode");
   }
-  // We always need to release the copy we made with GuardedMemory
-  GuardedMemory::free_copy(carray);
   return orig_result;
 }
 
@@ -1715,7 +1719,7 @@ JNI_ENTRY_CHECKED(void,  \
       typeArrayOop a = typeArrayOop(JNIHandles::resolve_non_null(array)); \
     ) \
     ElementType* orig_result = (ElementType *) check_wrapped_array_release( \
-        thr, "checked_jni_Release"#Result"ArrayElements", array, elems, mode); \
+        thr, "checked_jni_Release"#Result"ArrayElements", array, elems, mode, JNI_FALSE); \
     UNCHECKED()->Release##Result##ArrayElements(env, array, orig_result, mode); \
     functionExit(thr); \
 JNI_END
@@ -1884,7 +1888,8 @@ JNI_ENTRY_CHECKED(void,
       check_is_primitive_array(thr, array);
     )
     // Check the element array...
-    void* orig_result = check_wrapped_array_release(thr, "ReleasePrimitiveArrayCritical", array, carray, mode);
+    void* orig_result = check_wrapped_array_release(thr, "ReleasePrimitiveArrayCritical",
+                                                    array, carray, mode, JNI_TRUE);
     UNCHECKED()->ReleasePrimitiveArrayCritical(env, array, orig_result, mode);
     functionExit(thr);
 JNI_END
diff --git a/test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseArrayElements.java b/test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseArrayElements.java
@@ -0,0 +1,122 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8258077
+ * @summary verify multiple release calls on a copied array work when checked
+ * @library /test/lib
+ * @run main/native TestCheckedReleaseArrayElements launch
+ */
+
+import java.util.Arrays;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.Utils;
+import jtreg.SkippedException;
+
+public class TestCheckedReleaseArrayElements {
+
+    static {
+        System.loadLibrary("TestCheckedReleaseArrayElements");
+    }
+
+    public static void main(String[] args) throws Throwable {
+        if (args == null || args.length == 0) {
+            test();
+        } else {
+            OutputAnalyzer output =
+                ProcessTools.executeTestJvm("-Xcheck:jni",
+                                            "-Djava.library.path=" + Utils.TEST_NATIVE_PATH,
+                                            "TestCheckedReleaseArrayElements");
+            output.shouldHaveExitValue(0);
+            output.stderrShouldBeEmpty();
+            output.stdoutShouldNotBeEmpty();
+        }
+    }
+
+    /*
+     * If GetIntArrayElements returns a copy, we update the array in slices
+     * calling ReleaseIntArrayElements with JNI_COMMIT to write-back the
+     * updates, which are then checked. Finally we use JNI_ABORT to free
+     * the copy.
+     */
+    public static void test() {
+        final int slices = 3;
+        final int sliceLength = 3;
+        int[] arr = new int[slices * sliceLength];
+
+        if (!init(arr)) {
+            throw new SkippedException("Test skipped as GetIntArrayElements did not make a copy");
+        }
+
+        System.out.println("Array before: " + Arrays.toString(arr));
+
+        // We fill the array in slices so that arr[i] = i
+        for (int i = 0; i < slices; i++) {
+            int start = i * sliceLength;
+            fill(arr, start, sliceLength);
+            System.out.println("Array during: " + Arrays.toString(arr));
+            check(arr, (i + 1) * sliceLength);
+        }
+        System.out.println("Array after: " + Arrays.toString(arr));
+        cleanup(arr);
+    }
+
+    /*
+     * Calls GetIntArrayElements and stashes the native pointer for
+     * use by fill() if a copy was made.
+     * Returns true if a copy was made else false.
+    */
+    static native boolean init(int[] arr);
+
+    /*
+     * Fills in target[start] to target[start+count-1], so that
+     * target[i] == i. The update is done natively using the raw
+     * pointer into the array.
+     */
+    static native void fill(int[] target, int start, int count);
+
+    /*
+     * Properly release the copied array
+     */
+    static native void cleanup(int[] target);
+
+
+    static void check(int[] source, int count) {
+        for (int i = 0; i < count; i++) {
+            if (source[i] != i) {
+                System.out.println("Failing source array: " + Arrays.toString(source));
+                throw new RuntimeException("Expected source[" + i + "] == " +
+                                           i + " but got " + source[i]);
+            }
+        }
+        for (int i = count; i < source.length; i++) {
+            if (source[i] != 0) {
+                System.out.println("Failing source array: " + Arrays.toString(source));
+                throw new RuntimeException("Expected source[" + i +
+                                           "] == 0 but got " + source[i]);
+            }
+        }
+
+    }
+}
diff --git a/test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseCriticalArray.java b/test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseCriticalArray.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/* @test
+ * @bug 8193234 8258077
+ * @summary Check ReleasePrimitiveArrayCritical doesn't leak memory with
+ * Xcheck:jni and JNI_COMMIT mode
+ * @comment This is a manual test as you need to verify memory usage externally.
+ * @library /test/lib
+ * @run main/othervm/native/manual -Xms4m -Xmx4m -Xcheck:jni TestCheckedReleaseCriticalArray
+ */
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+import jtreg.SkippedException;
+
+public class TestCheckedReleaseCriticalArray {
+
+    static {
+        System.loadLibrary("TestCheckedReleaseCriticalArray");
+    }
+
+    /*
+     * We repeatedly modify an array via the JNI critical functions, using
+     * JNI_COMMIT mode. No memory leak should be observed on a VM that
+     * provides direct array access.
+     */
+    public static void main(String[] args) {
+        int[] array = new int[] { 1, 2, 3 };
+        if (!modifyArray(array)) {
+            // If the VM makes copies then we will leak them if we only ever use
+            // JNI_COMMIT mode.
+            throw new SkippedException("Test skipped as GetPrimitiveArrayCritical made a copy");
+        }
+        while (true) {
+            modifyArray(array);
+        }
+    }
+
+    private static native boolean modifyArray(int[] array);
+}
diff --git a/test/hotspot/jtreg/runtime/jni/checked/libTestCheckedReleaseArrayElements.c b/test/hotspot/jtreg/runtime/jni/checked/libTestCheckedReleaseArrayElements.c
@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <jni.h>
+
+static jint* arr;
+
+JNIEXPORT jboolean JNICALL
+Java_TestCheckedReleaseArrayElements_init(JNIEnv *env,
+                                          jclass* clazz,
+                                          jintArray target) {
+  jboolean isCopy;
+  arr = (*env)->GetIntArrayElements(env, target, &isCopy);
+  if (arr == NULL) {
+    (*env)->FatalError(env, "Unexpected NULL return from GetIntArrayElements");
+  }
+  if (isCopy == JNI_FALSE) {
+    (*env)->ReleaseIntArrayElements(env, target, arr, JNI_ABORT);
+  }
+  return isCopy;
+}
+
+JNIEXPORT void JNICALL
+Java_TestCheckedReleaseArrayElements_cleanup(JNIEnv *env,
+                                             jclass* clazz,
+                                             jintArray target) {
+  (*env)->ReleaseIntArrayElements(env, target, arr, JNI_ABORT);
+}
+
+
+JNIEXPORT void JNICALL
+Java_TestCheckedReleaseArrayElements_fill(JNIEnv *env,
+                                          jclass* clazz,
+                                          jintArray target,
+                                          jint start,
+                                          jint count) {
+  // Update a slice of the raw array
+  int i;
+  for (i = start; count > 0; i++, count--) {
+    arr[i] = i;
+  }
+  // Write the results back to target, leaving it usable for future updates
+  (*env)->ReleaseIntArrayElements(env, target, arr, JNI_COMMIT);
+}
diff --git a/test/hotspot/jtreg/runtime/jni/checked/libTestCheckedReleaseCriticalArray.c b/test/hotspot/jtreg/runtime/jni/checked/libTestCheckedReleaseCriticalArray.c
@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <jni.h>
+
+JNIEXPORT jboolean JNICALL
+Java_TestCheckedReleaseCriticalArray_modifyArray(JNIEnv *env,
+                                                 jclass clazz,
+                                                 jintArray iarr) {
+  jboolean isCopy;
+  jint* arr = (jint *)(*env)->GetPrimitiveArrayCritical(env, iarr, &isCopy);
+  if (arr == NULL) {
+    (*env)->FatalError(env, "Unexpected NULL return from GetPrimitiveArrayCritical");
+  }
+  if (isCopy == JNI_FALSE) {
+    jint len = (*env)->GetArrayLength(env, iarr);
+    // make arbitrary changes to the array
+    for (int i = 0; i < len; i++) {
+      arr[i] *= 2;
+    }
+    // write-back using JNI_COMMIT to test for memory leak
+    (*env)->ReleasePrimitiveArrayCritical(env, iarr, arr, JNI_COMMIT);
+  }
+  // we skip the test if the VM makes a copy - as it will definitely leak
+  return !isCopy;
+}
