8361447: [REDO] Checked version of JNI Release<type>ArrayElements needs to filter out known wrapped arrays

Co-authored-by: Thomas Stuefe <[email protected]>
Reviewed-by: mdoerr, stuefe, coleenp

Author: David Holmes

src/hotspot/share/memory/guardedMemory.cpp

@@ -25,11 +25,12 @@
 #include "nmt/memTag.hpp"
 #include "runtime/os.hpp"
 
-void* GuardedMemory::wrap_copy(const void* ptr, const size_t len, const void* tag) {
+void* GuardedMemory::wrap_copy(const void* ptr, const size_t len,
+                               const void* tag, const void* tag2) {
   size_t total_sz = GuardedMemory::get_total_size(len);
   void* outerp = os::malloc(total_sz, mtInternal);
   if (outerp != nullptr) {
-    GuardedMemory guarded(outerp, len, tag);
+    GuardedMemory guarded(outerp, len, tag, tag2);
     void* innerp = guarded.get_user_ptr();
     if (ptr != nullptr) {
       memcpy(innerp, ptr, len);
@@ -58,8 +59,8 @@ void GuardedMemory::print_on(outputStream* st) const {
     return;
   }
   st->print_cr("GuardedMemory(" PTR_FORMAT ") base_addr=" PTR_FORMAT
-      " tag=" PTR_FORMAT " user_size=%zu user_data=" PTR_FORMAT,
-      p2i(this), p2i(_base_addr), p2i(get_tag()), get_user_size(), p2i(get_user_ptr()));
+      " tag=" PTR_FORMAT " tag2=" PTR_FORMAT " user_size=%zu user_data=" PTR_FORMAT,
+      p2i(this), p2i(_base_addr), p2i(get_tag()), p2i(get_tag2()), get_user_size(), p2i(get_user_ptr()));
 
   Guard* guard = get_head_guard();
   st->print_cr("  Header guard @" PTR_FORMAT " is %s", p2i(guard), (guard->verify() ? "OK" : "BROKEN"));

src/hotspot/share/memory/guardedMemory.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2025, 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
@@ -26,6 +26,7 @@
 #define SHARE_MEMORY_GUARDEDMEMORY_HPP
 
 #include "memory/allocation.hpp"
+#include "runtime/os.hpp"
 #include "utilities/globalDefinitions.hpp"
 
 /**
@@ -43,13 +44,14 @@
  * |base_addr          | 0xABABABABABABABAB   | Head guard     |
  * |+16                | <size_t:user_size>   | User data size |
  * |+sizeof(uintptr_t) | <tag>                | Tag word       |
+ * |+sizeof(uintptr_t) | <tag2>               | Tag word       |
  * |+sizeof(void*)     | 0xF1 <user_data> (   | User data      |
  * |+user_size         | 0xABABABABABABABAB   | Tail guard     |
  * -------------------------------------------------------------
  *
  * Where:
  *  - guard padding uses "badResourceValue" (0xAB)
- *  - tag word is general purpose
+ *  - tag word and tag2 word are general purpose
  *  - user data
  *    -- initially padded with "uninitBlockPad" (0xF1),
  *    -- to "freeBlockPad" (0xBA), when freed
@@ -111,6 +113,10 @@ class GuardedMemory : StackObj { // Wrapper on stack
     }
 
     bool verify() const {
+      // We may not be able to dereference directly.
+      if (!os::is_readable_range((const void*) _guard, (const void*) (_guard + GUARD_SIZE))) {
+        return false;
+      }
       u_char* c = (u_char*) _guard;
       u_char* end = c + GUARD_SIZE;
       while (c < end) {
@@ -137,13 +143,16 @@ class GuardedMemory : StackObj { // Wrapper on stack
       size_t _user_size;
     };
     void* _tag;
+    void* _tag2;
    public:
     void set_user_size(const size_t usz) { _user_size = usz; }
     size_t get_user_size() const { return _user_size; }
 
     void set_tag(const void* tag) { _tag = (void*) tag; }
     void* get_tag() const { return _tag; }
 
+    void set_tag2(const void* tag2) { _tag2 = (void*) tag2; }
+    void* get_tag2() const { return _tag2; }
   }; // GuardedMemory::GuardHeader
 
   // Guarded Memory...
@@ -162,9 +171,11 @@ class GuardedMemory : StackObj { // Wrapper on stack
    * @param base_ptr  allocation wishing to be wrapped, must be at least "GuardedMemory::get_total_size()" bytes.
    * @param user_size the size of the user data to be wrapped.
    * @param tag       optional general purpose tag.
+   * @param tag2      optional second general purpose tag.
    */
-  GuardedMemory(void* base_ptr, const size_t user_size, const void* tag = nullptr) {
-    wrap_with_guards(base_ptr, user_size, tag);
+  GuardedMemory(void* base_ptr, const size_t user_size,
+                const void* tag = nullptr, const void* tag2 = nullptr) {
+    wrap_with_guards(base_ptr, user_size, tag, tag2);
   }
 
   /**
@@ -189,16 +200,19 @@ class GuardedMemory : StackObj { // Wrapper on stack
    * @param base_ptr  allocation wishing to be wrapped, must be at least "GuardedMemory::get_total_size()" bytes.
    * @param user_size the size of the user data to be wrapped.
    * @param tag       optional general purpose tag.
+   * @param tag2      optional second general purpose tag.
    *
    * @return user data pointer (inner pointer to supplied "base_ptr").
    */
-  void* wrap_with_guards(void* base_ptr, size_t user_size, const void* tag = nullptr) {
+  void* wrap_with_guards(void* base_ptr, size_t user_size,
+                         const void* tag = nullptr, const void* tag2 = nullptr) {
     assert(base_ptr != nullptr, "Attempt to wrap null with memory guard");
     _base_addr = (u_char*)base_ptr;
     get_head_guard()->build();
     get_head_guard()->set_user_size(user_size);
     get_tail_guard()->build();
     set_tag(tag);
+    set_tag2(tag2);
     set_user_bytes(uninitBlockPad);
     assert(verify_guards(), "Expected valid memory guards");
     return get_user_ptr();
@@ -230,6 +244,20 @@ class GuardedMemory : StackObj { // Wrapper on stack
    */
   void* get_tag() const { return get_head_guard()->get_tag(); }
 
+  /**
+   * Set the second general purpose tag.
+   *
+   * @param tag general purpose tag.
+   */
+  void set_tag2(const void* tag) { get_head_guard()->set_tag2(tag); }
+
+  /**
+   * Return the second general purpose tag.
+   *
+   * @return the second general purpose tag, defaults to null.
+   */
+  void* get_tag2() const { return get_head_guard()->get_tag2(); }
+
   /**
    * Return the size of the user data.
    *
@@ -302,10 +330,12 @@ class GuardedMemory : StackObj { // Wrapper on stack
    * @param ptr the memory to be copied
    * @param len the length of the copy
    * @param tag optional general purpose tag (see GuardedMemory::get_tag())
+   * @param tag2 optional general purpose tag (see GuardedMemory::get_tag2())
    *
    * @return guarded wrapped memory pointer to the user area, or null if OOM.
    */
-  static void* wrap_copy(const void* p, const size_t len, const void* tag = nullptr);
+  static void* wrap_copy(const void* p, const size_t len,
+                         const void* tag = nullptr, const void* tag2 = nullptr);
 
   /**
    * Free wrapped copy.

src/hotspot/share/prims/jniCheck.cpp

@@ -350,24 +350,33 @@ check_is_obj_array(JavaThread* thr, jarray jArray) {
   }
 }
 
+// Arbitrary (but well-known) tag for GetStringChars
+const void* STRING_TAG = (void*)0x47114711;
+
+// Arbitrary (but well-known) tag for GetStringUTFChars
+const void* STRING_UTF_TAG = (void*) 0x48124812;
+
+// Arbitrary (but well-known) tag for GetPrimitiveArrayCritical
+const void* CRITICAL_TAG = (void*)0x49134913;
+
 /*
  * Copy and wrap array elements for bounds checking.
  * Remember the original elements (GuardedMemory::get_tag())
  */
 static void* check_jni_wrap_copy_array(JavaThread* thr, jarray array,
-    void* orig_elements) {
+                                       void* orig_elements, jboolean is_critical = JNI_FALSE) {
   void* result;
   IN_VM(
     oop a = JNIHandles::resolve_non_null(array);
     size_t len = arrayOop(a)->length() <<
         TypeArrayKlass::cast(a->klass())->log2_element_size();
-    result = GuardedMemory::wrap_copy(orig_elements, len, orig_elements);
+    result = GuardedMemory::wrap_copy(orig_elements, len, orig_elements, is_critical ? CRITICAL_TAG : nullptr);
   )
   return result;
 }
 
 static void* check_wrapped_array(JavaThread* thr, const char* fn_name,
-    void* obj, void* carray, size_t* rsz) {
+                                 void* obj, void* carray, size_t* rsz, jboolean is_critical) {
   if (carray == nullptr) {
     tty->print_cr("%s: elements vector null" PTR_FORMAT, fn_name, p2i(obj));
     NativeReportJNIFatalError(thr, "Elements vector null");
@@ -386,6 +395,29 @@ static void* check_wrapped_array(JavaThread* thr, const char* fn_name,
     DEBUG_ONLY(guarded.print_on(tty);) // This may crash.
     NativeReportJNIFatalError(thr, err_msg("%s: unrecognized elements", fn_name));
   }
+  if (orig_result == STRING_TAG || orig_result == STRING_UTF_TAG) {
+    bool was_utf = orig_result == STRING_UTF_TAG;
+    tty->print_cr("%s: called on something allocated by %s",
+                  fn_name, was_utf ? "GetStringUTFChars" : "GetStringChars");
+    DEBUG_ONLY(guarded.print_on(tty);) // This may crash.
+    NativeReportJNIFatalError(thr, err_msg("%s called on something allocated by %s",
+                                           fn_name, was_utf ? "GetStringUTFChars" : "GetStringChars"));
+  }
+
+  if (is_critical && (guarded.get_tag2() != CRITICAL_TAG)) {
+    tty->print_cr("%s: called on something not allocated by GetPrimitiveArrayCritical", fn_name);
+    DEBUG_ONLY(guarded.print_on(tty);) // This may crash.
+    NativeReportJNIFatalError(thr, err_msg("%s called on something not allocated by GetPrimitiveArrayCritical",
+                                           fn_name));
+  }
+
+  if (!is_critical && (guarded.get_tag2() == CRITICAL_TAG)) {
+    tty->print_cr("%s: called on something allocated by GetPrimitiveArrayCritical", fn_name);
+    DEBUG_ONLY(guarded.print_on(tty);) // This may crash.
+    NativeReportJNIFatalError(thr, err_msg("%s called on something allocated by GetPrimitiveArrayCritical",
+                                           fn_name));
+  }
+
   if (rsz != nullptr) {
     *rsz = guarded.get_user_size();
   }
@@ -395,7 +427,7 @@ 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, jboolean is_critical) {
   size_t sz;
-  void* orig_result = check_wrapped_array(thr, fn_name, obj, carray, &sz);
+  void* orig_result = check_wrapped_array(thr, fn_name, obj, carray, &sz, is_critical);
   switch (mode) {
   case 0:
     memcpy(orig_result, carray, sz);
@@ -1430,9 +1462,6 @@ JNI_ENTRY_CHECKED(jsize,
     return result;
 JNI_END
 
-// Arbitrary (but well-known) tag
-const void* STRING_TAG = (void*)0x47114711;
-
 JNI_ENTRY_CHECKED(const jchar *,
   checked_jni_GetStringChars(JNIEnv *env,
                              jstring str,
@@ -1535,9 +1564,6 @@ JNI_ENTRY_CHECKED(jlong,
     return result;
 JNI_END
 
-// Arbitrary (but well-known) tag - different than GetStringChars
-const void* STRING_UTF_TAG = (void*) 0x48124812;
-
 JNI_ENTRY_CHECKED(const char *,
   checked_jni_GetStringUTFChars(JNIEnv *env,
                                 jstring str,
@@ -1859,7 +1885,7 @@ JNI_ENTRY_CHECKED(void *,
     )
     void *result = UNCHECKED()->GetPrimitiveArrayCritical(env, array, isCopy);
     if (result != nullptr) {
-      result = check_jni_wrap_copy_array(thr, array, result);
+      result = check_jni_wrap_copy_array(thr, array, result, JNI_TRUE);
     }
     functionExit(thr);
     return result;

test/hotspot/gtest/memory/test_guardedMemory.cpp

@@ -57,7 +57,7 @@ TEST(GuardedMemory, size) {
 }
 
 // Test the basic characteristics
-TEST(GuardedMemory, basic) {
+TEST_VM(GuardedMemory, basic) {
   u_char* basep =
           (u_char*) os::malloc(GuardedMemory::get_total_size(1), mtInternal);
   GuardedMemory guarded(basep, 1, GEN_PURPOSE_TAG);
@@ -78,7 +78,7 @@ TEST(GuardedMemory, basic) {
 }
 
 // Test a number of odd sizes
-TEST(GuardedMemory, odd_sizes) {
+TEST_VM(GuardedMemory, odd_sizes) {
   u_char* basep =
           (u_char*) os::malloc(GuardedMemory::get_total_size(1), mtInternal);
   GuardedMemory guarded(basep, 1, GEN_PURPOSE_TAG);
@@ -99,7 +99,7 @@ TEST(GuardedMemory, odd_sizes) {
 }
 
 // Test buffer overrun into head...
-TEST(GuardedMemory, buffer_overrun_head) {
+TEST_VM(GuardedMemory, buffer_overrun_head) {
   u_char* basep =
           (u_char*) os::malloc(GuardedMemory::get_total_size(1), mtInternal);
   GuardedMemory guarded(basep, 1, GEN_PURPOSE_TAG);
@@ -111,7 +111,7 @@ TEST(GuardedMemory, buffer_overrun_head) {
 }
 
 // Test buffer overrun into tail with a number of odd sizes
-TEST(GuardedMemory, buffer_overrun_tail) {
+TEST_VM(GuardedMemory, buffer_overrun_tail) {
   u_char* basep =
           (u_char*) os::malloc(GuardedMemory::get_total_size(1), mtInternal);
   GuardedMemory guarded(basep, 1, GEN_PURPOSE_TAG);
@@ -128,7 +128,7 @@ TEST(GuardedMemory, buffer_overrun_tail) {
 }
 
 // Test wrap_copy/wrap_free
-TEST(GuardedMemory, wrap) {
+TEST_VM(GuardedMemory, wrap) {
   EXPECT_TRUE(GuardedMemory::free_copy(nullptr)) << "Expected free nullptr to be OK";
 
   const char* str = "Check my bounds out";
@@ -146,3 +146,10 @@ TEST(GuardedMemory, wrap) {
   EXPECT_TRUE(GuardedMemory::free_copy(no_data_copy))
           << "Expected valid guards even for no data copy";
 }
+
+// Test passing back a bogus GuardedMemory region
+TEST_VM(GuardedMemory, unmapped) {
+  char* unmapped_base = (char*) (GuardedMemoryTest::get_guard_header_size() + 0x1000 + 1); // Avoids assert in constructor
+  GuardedMemory guarded(unmapped_base);
+  EXPECT_FALSE(guarded.verify_guards()) << "Guard was not broken as expected";
+}