From 259e27961b4319acccfbdb41a551ec41b7c7e24e Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 03:16:22 +0200 Subject: [PATCH 1/2] fix(jni): release handles and arrays on allocation failures --- CHANGELOG.md | 3 ++ jni/src/jni_core.cpp | 27 +++++++++-------- jni/src/jni_document.cpp | 12 ++++---- jni/src/jni_html.cpp | 29 ++++++++++++------- jni/src/jni_logger.cpp | 12 ++++++++ jni/src/jni_style.cpp | 6 ++-- jni/src/odr_jni.hpp | 7 ++++- jni/tests/app/opendocument/core/FileTest.java | 1 + jni/tests/app/opendocument/core/HtmlTest.java | 5 ++++ 9 files changed, 69 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 039e72241..a96ebf266 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/jni/src/jni_core.cpp b/jni/src/jni_core.cpp index 08a8a3801..279bfaf48 100644 --- a/jni/src/jni_core.cpp +++ b/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 &values) { - jintArray result = env->NewIntArray(static_cast(values.size())); + jintArray result = env->NewIntArray(checked_integer(values.size())); if (result == nullptr) { return nullptr; } - env->SetIntArrayRegion(result, 0, static_cast(values.size()), + env->SetIntArrayRegion(result, 0, checked_integer(values.size()), values.data()); return result; } @@ -38,14 +38,14 @@ jobjectArray to_jstring_array(JNIEnv *env, return nullptr; } jobjectArray result = - env->NewObjectArray(static_cast(values.size()), cls, nullptr); + env->NewObjectArray(checked_integer(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(i), value); + env->SetObjectArrayElement(result, checked_integer(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(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(codes[i])); - } - env->ReleaseIntArrayElements(file_type_priority, codes, JNI_ABORT); + const jsize length = env->GetArrayLength(file_type_priority); + std::vector codes(static_cast(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(code)); } // -1 is how java spells an unset field across the boundary if (csv_encoding >= 0) { diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 936839a71..e3ad0d0a6 100644 --- a/jni/src/jni_document.cpp +++ b/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 &handles = guard.handles(); - jlongArray result = env->NewLongArray(static_cast(handles.size())); + jlongArray result = env->NewLongArray(checked_integer(handles.size())); if (result == nullptr) { return nullptr; } - env->SetLongArrayRegion(result, 0, static_cast(handles.size()), + env->SetLongArrayRegion(result, 0, checked_integer(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(position.cell.row)); } } - jintArray array = env->NewIntArray(static_cast(stated.size())); + jintArray array = env->NewIntArray(checked_integer(stated.size())); if (array == nullptr) { return nullptr; } - env->SetIntArrayRegion(array, 0, static_cast(stated.size()), + env->SetIntArrayRegion(array, 0, checked_integer(stated.size()), stated.data()); return array; }); diff --git a/jni/src/jni_html.cpp b/jni/src/jni_html.cpp index 5e5b18a1e..35689f667 100644 --- a/jni/src/jni_html.cpp +++ b/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 &values) { - jlongArray result = env->NewLongArray(static_cast(values.size())); + jlongArray result = env->NewLongArray(checked_integer(values.size())); if (result == nullptr) { return nullptr; } - env->SetLongArrayRegion(result, 0, static_cast(values.size()), + env->SetLongArrayRegion(result, 0, checked_integer(values.size()), values.data()); return result; } @@ -58,12 +59,12 @@ jobject make_html(JNIEnv *env, odr::Html html) { return nullptr; } const std::vector &pages = html.pages(); - jobjectArray page_array = - env->NewObjectArray(static_cast(pages.size()), page_cls, nullptr); + jobjectArray page_array = env->NewObjectArray( + checked_integer(pages.size()), page_cls, nullptr); if (page_array == nullptr) { return nullptr; } - for (jsize i = 0; i < static_cast(pages.size()); ++i) { + for (jsize i = 0; i < checked_integer(pages.size()); ++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); @@ -114,11 +115,11 @@ jobject make_content(JNIEnv *env, const std::string &html, } jobjectArray located_array = env->NewObjectArray( - static_cast(resources.size()), located_cls, nullptr); + checked_integer(resources.size()), located_cls, nullptr); if (located_array == nullptr) { return nullptr; } - for (jsize i = 0; i < static_cast(resources.size()); ++i) { + for (jsize i = 0; i < checked_integer(resources.size()); ++i) { const auto &[resource, location] = resources[i]; HandleGuard guard(1); jobject resource_obj = @@ -293,11 +294,17 @@ Java_app_opendocument_core_HtmlService_bringOfflineViewsNative( return guarded(env, [&] { std::vector 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(handles[i])); + std::vector handles(static_cast(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(view_handle)); } - env->ReleaseLongArrayElements(view_handles, handles, JNI_ABORT); return make_html( env, service(handle).bring_offline(to_string(env, output_path), views)); }); diff --git a/jni/src/jni_logger.cpp b/jni/src/jni_logger.cpp index b35244c72..e60f1ccde 100644 --- a/jni/src/jni_logger.cpp +++ b/jni/src/jni_logger.cpp @@ -74,6 +74,9 @@ 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) { @@ -81,12 +84,21 @@ class JavaLogger final : public odr::ILogger { } m_bridge = static_cast(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"); } diff --git a/jni/src/jni_style.cpp b/jni/src/jni_style.cpp index 5ba42afb3..e72e2055d 100644 --- a/jni/src/jni_style.cpp +++ b/jni/src/jni_style.cpp @@ -728,10 +728,10 @@ jobject html_config_to_java(JNIEnv *env, const odr::HtmlConfig &config) { { jclass string_cls = env->FindClass("java/lang/String"); jobjectArray fonts = env->NewObjectArray( - static_cast(config.pdf_dual_layer_fallback_fonts.size()), + checked_integer(config.pdf_dual_layer_fallback_fonts.size()), string_cls, nullptr); - for (jsize i = 0; - i < static_cast(config.pdf_dual_layer_fallback_fonts.size()); + for (jsize i = 0; i < checked_integer( + config.pdf_dual_layer_fallback_fonts.size()); ++i) { jstring font = to_jstring(env, config.pdf_dual_layer_fallback_fonts[i]); env->SetObjectArrayElement(fonts, i, font); diff --git a/jni/src/odr_jni.hpp b/jni/src/odr_jni.hpp index 30ec77e8f..eeaa60884 100644 --- a/jni/src/odr_jni.hpp +++ b/jni/src/odr_jni.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -79,7 +80,11 @@ template class HandleGuard { } jlong add(T value) { - return m_handles.emplace_back(make_handle(std::move(value))); + auto owned = std::make_unique(std::move(value)); + const jlong handle = reinterpret_cast(owned.get()); + m_handles.push_back(handle); + owned.release(); + return handle; } [[nodiscard]] const std::vector &handles() const { return m_handles; } diff --git a/jni/tests/app/opendocument/core/FileTest.java b/jni/tests/app/opendocument/core/FileTest.java index 64a58fa74..2fd69ef80 100644 --- a/jni/tests/app/opendocument/core/FileTest.java +++ b/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 = '|'; diff --git a/jni/tests/app/opendocument/core/HtmlTest.java b/jni/tests/app/opendocument/core/HtmlTest.java index 83f1f27c0..c8c18e4e3 100644 --- a/jni/tests/app/opendocument/core/HtmlTest.java +++ b/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()); } } From 413414f3f42b65449d52eb44a5036dd60337df1d Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 22:34:20 +0200 Subject: [PATCH 2/2] refactor(jni): check each array length once before its loop Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- jni/src/jni_html.cpp | 13 +++++++------ jni/src/jni_style.cpp | 10 ++++------ 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/jni/src/jni_html.cpp b/jni/src/jni_html.cpp index 35689f667..0e42910b3 100644 --- a/jni/src/jni_html.cpp +++ b/jni/src/jni_html.cpp @@ -59,12 +59,12 @@ jobject make_html(JNIEnv *env, odr::Html html) { return nullptr; } const std::vector &pages = html.pages(); - jobjectArray page_array = env->NewObjectArray( - checked_integer(pages.size()), page_cls, nullptr); + const auto page_count = checked_integer(pages.size()); + jobjectArray page_array = env->NewObjectArray(page_count, page_cls, nullptr); if (page_array == nullptr) { return nullptr; } - for (jsize i = 0; i < checked_integer(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); @@ -114,12 +114,13 @@ jobject make_content(JNIEnv *env, const std::string &html, return nullptr; } - jobjectArray located_array = env->NewObjectArray( - checked_integer(resources.size()), located_cls, nullptr); + const auto resource_count = checked_integer(resources.size()); + jobjectArray located_array = + env->NewObjectArray(resource_count, located_cls, nullptr); if (located_array == nullptr) { return nullptr; } - for (jsize i = 0; i < checked_integer(resources.size()); ++i) { + for (jsize i = 0; i < resource_count; ++i) { const auto &[resource, location] = resources[i]; HandleGuard guard(1); jobject resource_obj = diff --git a/jni/src/jni_style.cpp b/jni/src/jni_style.cpp index e72e2055d..99d657896 100644 --- a/jni/src/jni_style.cpp +++ b/jni/src/jni_style.cpp @@ -727,12 +727,10 @@ jobject html_config_to_java(JNIEnv *env, const odr::HtmlConfig &config) { static_cast(config.pdf_text_mode))); { jclass string_cls = env->FindClass("java/lang/String"); - jobjectArray fonts = env->NewObjectArray( - checked_integer(config.pdf_dual_layer_fallback_fonts.size()), - string_cls, nullptr); - for (jsize i = 0; i < checked_integer( - config.pdf_dual_layer_fallback_fonts.size()); - ++i) { + const auto font_count = + checked_integer(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);