fix(jni): release handles and arrays on allocation failures

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Java bindings release native handles on failed allocations, avoid pinned-array
+  leaks, and check array sizes before passing them to the JVM.
+
 - Java bindings reject out-of-range coordinates, style channels, CSV delimiters
   and rendering limits instead of silently wrapping their integer values.
 

jni/src/jni_core.cpp

@@ -22,11 +22,11 @@ using odr_jni::to_jstring;
 using odr_jni::to_string;
 
 jintArray to_jint_array(JNIEnv *env, const std::vector<jint> &values) {
-  jintArray result = env->NewIntArray(static_cast<jsize>(values.size()));
+  jintArray result = env->NewIntArray(checked_integer<jsize>(values.size()));
   if (result == nullptr) {
     return nullptr;
   }
-  env->SetIntArrayRegion(result, 0, static_cast<jsize>(values.size()),
+  env->SetIntArrayRegion(result, 0, checked_integer<jsize>(values.size()),
                          values.data());
   return result;
 }
@@ -38,14 +38,14 @@ jobjectArray to_jstring_array(JNIEnv *env,
     return nullptr;
   }
   jobjectArray result =
-      env->NewObjectArray(static_cast<jsize>(values.size()), cls, nullptr);
+      env->NewObjectArray(checked_integer<jsize>(values.size()), cls, nullptr);
   env->DeleteLocalRef(cls);
   if (result == nullptr) {
     return nullptr;
   }
   for (std::size_t i = 0; i < values.size(); ++i) {
     jstring value = to_jstring(env, values[i]);
-    env->SetObjectArrayElement(result, static_cast<jsize>(i), value);
+    env->SetObjectArrayElement(result, checked_integer<jsize>(i), value);
     env->DeleteLocalRef(value);
   }
   return result;
@@ -264,14 +264,17 @@ Java_app_opendocument_core_Odr_openWithOptionsNative(
     if (as_file_type >= 0) {
       options.as_file_type = static_cast<odr::FileType>(as_file_type);
     }
-    if (jint *codes = env->GetIntArrayElements(file_type_priority, nullptr);
-        codes != nullptr) {
-      const jsize length = env->GetArrayLength(file_type_priority);
-      for (jsize i = 0; i < length; ++i) {
-        options.file_type_priority.push_back(
-            static_cast<odr::FileType>(codes[i]));
-      }
-      env->ReleaseIntArrayElements(file_type_priority, codes, JNI_ABORT);
+    const jsize length = env->GetArrayLength(file_type_priority);
+    std::vector<jint> codes(static_cast<std::size_t>(length));
+    if (length != 0) {
+      env->GetIntArrayRegion(file_type_priority, 0, length, codes.data());
+    }
+    if (env->ExceptionCheck()) {
+      throw std::runtime_error("could not read file type priority");
+    }
+    options.file_type_priority.reserve(codes.size());
+    for (const jint code : codes) {
+      options.file_type_priority.push_back(static_cast<odr::FileType>(code));
     }
     // -1 is how java spells an unset field across the boundary
     if (csv_encoding >= 0) {

jni/src/jni_document.cpp

@@ -27,11 +27,11 @@ odr::Element &element(jlong handle) {
 
 /// Elements cross JNI as plain `odr::Element` copies; typed Java views
 /// re-derive the typed C++ view per call via `as_*`.
-jlong wrap_element(odr::Element value) {
+jlong wrap_element(const odr::Element value) {
   if (!value) {
     return 0;
   }
-  return make_handle(std::move(value));
+  return make_handle(value);
 }
 
 jlongArray wrap_elements(JNIEnv *env, const odr::ElementRange &range) {
@@ -40,11 +40,11 @@ jlongArray wrap_elements(JNIEnv *env, const odr::ElementRange &range) {
     guard.add(value);
   }
   const std::vector<jlong> &handles = guard.handles();
-  jlongArray result = env->NewLongArray(static_cast<jsize>(handles.size()));
+  jlongArray result = env->NewLongArray(checked_integer<jsize>(handles.size()));
   if (result == nullptr) {
     return nullptr;
   }
-  env->SetLongArrayRegion(result, 0, static_cast<jsize>(handles.size()),
+  env->SetLongArrayRegion(result, 0, checked_integer<jsize>(handles.size()),
                           handles.data());
   guard.release();
   return result;
@@ -87,11 +87,11 @@ Java_app_opendocument_core_Document_recalculateNative(JNIEnv *env, jobject,
         stated.push_back(checked_integer<jint>(position.cell.row));
       }
     }
-    jintArray array = env->NewIntArray(static_cast<jsize>(stated.size()));
+    jintArray array = env->NewIntArray(checked_integer<jsize>(stated.size()));
     if (array == nullptr) {
       return nullptr;
     }
-    env->SetIntArrayRegion(array, 0, static_cast<jsize>(stated.size()),
+    env->SetIntArrayRegion(array, 0, checked_integer<jsize>(stated.size()),
                            stated.data());
     return array;
   });

jni/src/jni_html.cpp

@@ -12,6 +12,7 @@
 
 namespace {
 
+using odr_jni::checked_integer;
 using odr_jni::destroy_handle;
 using odr_jni::from_handle;
 using odr_jni::guarded;
@@ -22,11 +23,11 @@ using odr_jni::to_jstring;
 using odr_jni::to_string;
 
 jlongArray to_jlong_array(JNIEnv *env, const std::vector<jlong> &values) {
-  jlongArray result = env->NewLongArray(static_cast<jsize>(values.size()));
+  jlongArray result = env->NewLongArray(checked_integer<jsize>(values.size()));
   if (result == nullptr) {
     return nullptr;
   }
-  env->SetLongArrayRegion(result, 0, static_cast<jsize>(values.size()),
+  env->SetLongArrayRegion(result, 0, checked_integer<jsize>(values.size()),
                           values.data());
   return result;
 }
@@ -58,12 +59,12 @@ jobject make_html(JNIEnv *env, odr::Html html) {
     return nullptr;
   }
   const std::vector<odr::HtmlPage> &pages = html.pages();
-  jobjectArray page_array =
-      env->NewObjectArray(static_cast<jsize>(pages.size()), page_cls, nullptr);
+  const auto page_count = checked_integer<jsize>(pages.size());
+  jobjectArray page_array = env->NewObjectArray(page_count, page_cls, nullptr);
   if (page_array == nullptr) {
     return nullptr;
   }
-  for (jsize i = 0; i < static_cast<jsize>(pages.size()); ++i) {
+  for (jsize i = 0; i < page_count; ++i) {
     jstring name = to_jstring(env, pages[i].name);
     jstring path = to_jstring(env, pages[i].path);
     jobject page = env->NewObject(page_cls, page_ctor, name, path);
@@ -113,12 +114,13 @@ jobject make_content(JNIEnv *env, const std::string &html,
     return nullptr;
   }
 
-  jobjectArray located_array = env->NewObjectArray(
-      static_cast<jsize>(resources.size()), located_cls, nullptr);
+  const auto resource_count = checked_integer<jsize>(resources.size());
+  jobjectArray located_array =
+      env->NewObjectArray(resource_count, located_cls, nullptr);
   if (located_array == nullptr) {
     return nullptr;
   }
-  for (jsize i = 0; i < static_cast<jsize>(resources.size()); ++i) {
+  for (jsize i = 0; i < resource_count; ++i) {
     const auto &[resource, location] = resources[i];
     HandleGuard<odr::HtmlResource> guard(1);
     jobject resource_obj =
@@ -293,11 +295,17 @@ Java_app_opendocument_core_HtmlService_bringOfflineViewsNative(
   return guarded(env, [&] {
     std::vector<odr::HtmlView> views;
     const jsize length = env->GetArrayLength(view_handles);
-    jlong *handles = env->GetLongArrayElements(view_handles, nullptr);
-    for (jsize i = 0; i < length; ++i) {
-      views.push_back(*from_handle<odr::HtmlView>(handles[i]));
+    std::vector<jlong> handles(static_cast<std::size_t>(length));
+    if (length != 0) {
+      env->GetLongArrayRegion(view_handles, 0, length, handles.data());
+    }
+    if (env->ExceptionCheck()) {
+      throw std::runtime_error("could not read HTML view handles");
+    }
+    views.reserve(handles.size());
+    for (const jlong view_handle : handles) {
+      views.push_back(*from_handle<odr::HtmlView>(view_handle));
     }
-    env->ReleaseLongArrayElements(view_handles, handles, JNI_ABORT);
     return make_html(
         env, service(handle).bring_offline(to_string(env, output_path), views));
   });

jni/src/jni_logger.cpp

@@ -74,19 +74,31 @@ class JavaLogger final : public odr::ILogger {
   JavaLogger(JNIEnv *env, jobject sink) {
     env->GetJavaVM(&m_vm);
     m_sink = env->NewGlobalRef(sink);
+    if (m_sink == nullptr) {
+      return;
+    }
 
     jclass bridge = env->FindClass("app/opendocument/core/LoggerBridge");
     if (bridge == nullptr) {
       return;
     }
     m_bridge = static_cast<jclass>(env->NewGlobalRef(bridge));
     env->DeleteLocalRef(bridge);
+    if (m_bridge == nullptr) {
+      return;
+    }
     m_will_log = env->GetStaticMethodID(m_bridge, "willLog",
                                         "(Lapp/opendocument/core/ILogger;I)Z");
+    if (m_will_log == nullptr) {
+      return;
+    }
     m_log = env->GetStaticMethodID(m_bridge, "log",
                                    "(Lapp/opendocument/core/ILogger;JILjava/"
                                    "lang/String;Ljava/lang/String;Ljava/lang/"
                                    "String;I)V");
+    if (m_log == nullptr) {
+      return;
+    }
     m_flush = env->GetStaticMethodID(m_bridge, "flush",
                                      "(Lapp/opendocument/core/ILogger;)V");
   }

jni/src/jni_style.cpp

@@ -727,12 +727,10 @@ jobject html_config_to_java(JNIEnv *env, const odr::HtmlConfig &config) {
                             static_cast<jint>(config.pdf_text_mode)));
   {
     jclass string_cls = env->FindClass("java/lang/String");
-    jobjectArray fonts = env->NewObjectArray(
-        static_cast<jsize>(config.pdf_dual_layer_fallback_fonts.size()),
-        string_cls, nullptr);
-    for (jsize i = 0;
-         i < static_cast<jsize>(config.pdf_dual_layer_fallback_fonts.size());
-         ++i) {
+    const auto font_count =
+        checked_integer<jsize>(config.pdf_dual_layer_fallback_fonts.size());
+    jobjectArray fonts = env->NewObjectArray(font_count, string_cls, nullptr);
+    for (jsize i = 0; i < font_count; ++i) {
       jstring font = to_jstring(env, config.pdf_dual_layer_fallback_fonts[i]);
       env->SetObjectArrayElement(fonts, i, font);
       env->DeleteLocalRef(font);

jni/src/odr_jni.hpp

@@ -3,6 +3,7 @@
 #include <jni.h>
 
 #include <cstddef>
+#include <memory>
 #include <stdexcept>
 #include <string>
 #include <string_view>
@@ -79,7 +80,11 @@ template <typename T> class HandleGuard {
   }
 
   jlong add(T value) {
-    return m_handles.emplace_back(make_handle(std::move(value)));
+    auto owned = std::make_unique<T>(std::move(value));
+    const jlong handle = reinterpret_cast<jlong>(owned.get());
+    m_handles.push_back(handle);
+    owned.release();
+    return handle;
   }
 
   [[nodiscard]] const std::vector<jlong> &handles() const { return m_handles; }

jni/tests/app/opendocument/core/FileTest.java

@@ -165,6 +165,7 @@ void openCarriesCsvOptions() throws IOException {
     // detection would find the semicolon; a pipe it would not, so the caller says
     DecodeOptions options = new DecodeOptions();
     options.asFileType = FileType.COMMA_SEPARATED_VALUES;
+    options.fileTypePriority.add(FileType.COMMA_SEPARATED_VALUES);
     options.csv.separator = '\u017c';
     assertThrows(OdrException.class, () -> Odr.open(path.toString(), options));
     options.csv.separator = '|';

jni/tests/app/opendocument/core/HtmlTest.java

@@ -271,5 +271,10 @@ void htmlServiceViews() throws IOException {
 
     Html.Content content = views.get(0).writeHtml();
     assertTrue(content.html.contains(TestFiles.ODT_WORD));
+
+    Html offline = service.bringOffline(tempDir.resolve("selected").toString(), views);
+    assertEquals(1, offline.pages().size());
+    assertTrue(Files.readString(Path.of(offline.pages().get(0).path)).contains(TestFiles.ODT_WORD));
+    assertTrue(service.bringOffline(tempDir.resolve("empty").toString(), List.of()).pages().isEmpty());
   }
 }