fix: thread safety, lifetime and bounds-checking follow-ups

apple/include/OdrCoreObjC/ODRFile.h

@@ -97,7 +97,8 @@ typedef NS_ENUM(NSInteger, ODRFileCategory) {
 } NS_SWIFT_NAME(FileCategory);
 
 typedef NS_ENUM(NSInteger, ODRFileLocation) {
-  ODRFileLocationMemory = 0,
+  ODRFileLocationUnknown = 0,
+  ODRFileLocationMemory,
   ODRFileLocationDisk,
 } NS_SWIFT_NAME(FileLocation);
 

apple/src/ODRFile.mm

@@ -100,6 +100,7 @@
 ODR_SAME_ENUM(ODRFileCategoryAudio, odr::FileCategory::audio);
 ODR_SAME_ENUM(ODRFileCategoryVideo, odr::FileCategory::video);
 
+ODR_SAME_ENUM(ODRFileLocationUnknown, odr::FileLocation::unknown);
 ODR_SAME_ENUM(ODRFileLocationMemory, odr::FileLocation::memory);
 ODR_SAME_ENUM(ODRFileLocationDisk, odr::FileLocation::disk);
 

jni/java/app/opendocument/core/FileLocation.java

@@ -2,7 +2,7 @@
 
 /** Mirrors {@code odr::FileLocation}; constant order must match the C++ declaration. */
 public enum FileLocation {
-  MEMORY, DISK;
+  UNKNOWN, MEMORY, DISK;
 
   static FileLocation fromNative(int code) {
     return code < 0 ? null : values()[code];

jni/src/jni_document.cpp

@@ -14,6 +14,7 @@ namespace {
 using odr_jni::destroy_handle;
 using odr_jni::from_handle;
 using odr_jni::guarded;
+using odr_jni::HandleGuard;
 using odr_jni::make_handle;
 using odr_jni::to_jstring;
 using odr_jni::to_string;
@@ -32,16 +33,18 @@ jlong wrap_element(odr::Element value) {
 }
 
 jlongArray wrap_elements(JNIEnv *env, const odr::ElementRange &range) {
-  std::vector<jlong> handles;
+  HandleGuard<odr::Element> guard;
   for (const odr::Element &value : range) {
-    handles.push_back(make_handle(odr::Element(value)));
+    guard.add(value);
   }
+  const std::vector<jlong> &handles = guard.handles();
   jlongArray result = env->NewLongArray(static_cast<jsize>(handles.size()));
   if (result == nullptr) {
     return nullptr;
   }
   env->SetLongArrayRegion(result, 0, static_cast<jsize>(handles.size()),
                           handles.data());
+  guard.release();
   return result;
 }
 

jni/src/jni_html.cpp

@@ -15,6 +15,7 @@ namespace {
 using odr_jni::destroy_handle;
 using odr_jni::from_handle;
 using odr_jni::guarded;
+using odr_jni::HandleGuard;
 using odr_jni::make_handle;
 using odr_jni::to_jbytes;
 using odr_jni::to_jstring;
@@ -31,12 +32,16 @@ jlongArray to_jlong_array(JNIEnv *env, const std::vector<jlong> &values) {
 }
 
 jlongArray wrap_views(JNIEnv *env, const odr::HtmlViews &views) {
-  std::vector<jlong> handles;
-  handles.reserve(views.size());
+  HandleGuard<odr::HtmlView> guard(views.size());
   for (const odr::HtmlView &view : views) {
-    handles.push_back(make_handle(odr::HtmlView(view)));
+    guard.add(view);
   }
-  return to_jlong_array(env, handles);
+  jlongArray result = to_jlong_array(env, guard.handles());
+  if (result == nullptr) {
+    return nullptr;
+  }
+  guard.release();
+  return result;
 }
 
 /// Builds an `app.opendocument.core.Html` from an offline result.
@@ -49,9 +54,15 @@ jobject make_html(JNIEnv *env, odr::Html html) {
   }
   jmethodID page_ctor = env->GetMethodID(
       page_cls, "<init>", "(Ljava/lang/String;Ljava/lang/String;)V");
+  if (page_ctor == nullptr) {
+    return nullptr;
+  }
   const std::vector<odr::HtmlPage> &pages = html.pages();
   jobjectArray page_array =
       env->NewObjectArray(static_cast<jsize>(pages.size()), page_cls, nullptr);
+  if (page_array == nullptr) {
+    return nullptr;
+  }
   for (jsize i = 0; i < static_cast<jsize>(pages.size()); ++i) {
     jstring name = to_jstring(env, pages[i].name);
     jstring path = to_jstring(env, pages[i].path);
@@ -70,6 +81,9 @@ jobject make_html(JNIEnv *env, odr::Html html) {
   jmethodID html_ctor = env->GetMethodID(
       html_cls, "<init>",
       "(Lapp/opendocument/core/HtmlConfig;[Lapp/opendocument/core/HtmlPage;)V");
+  if (html_ctor == nullptr) {
+    return nullptr;
+  }
   jobject result = env->NewObject(html_cls, html_ctor, config, page_array);
   env->DeleteLocalRef(html_cls);
   return result;
@@ -87,15 +101,33 @@ jobject make_content(JNIEnv *env, const std::string &html,
   jmethodID located_ctor = env->GetMethodID(
       located_cls, "<init>",
       "(Lapp/opendocument/core/HtmlResource;Ljava/lang/String;)V");
+  if (located_ctor == nullptr) {
+    return nullptr;
+  }
   jclass resource_cls = env->FindClass("app/opendocument/core/HtmlResource");
+  if (resource_cls == nullptr) {
+    return nullptr;
+  }
   jmethodID resource_ctor = env->GetMethodID(resource_cls, "<init>", "(J)V");
+  if (resource_ctor == nullptr) {
+    return nullptr;
+  }
 
   jobjectArray located_array = env->NewObjectArray(
       static_cast<jsize>(resources.size()), located_cls, nullptr);
+  if (located_array == nullptr) {
+    return nullptr;
+  }
   for (jsize i = 0; i < static_cast<jsize>(resources.size()); ++i) {
     const auto &[resource, location] = resources[i];
-    jobject resource_obj = env->NewObject(
-        resource_cls, resource_ctor, make_handle(odr::HtmlResource(resource)));
+    HandleGuard<odr::HtmlResource> guard(1);
+    jobject resource_obj =
+        env->NewObject(resource_cls, resource_ctor, guard.add(resource));
+    if (resource_obj == nullptr) {
+      return nullptr;
+    }
+    // the Java wrapper exists, so its post-mortem destroyer owns the handle now
+    guard.release();
     jstring location_str =
         location.has_value() ? to_jstring(env, *location) : nullptr;
     jobject located =
@@ -117,6 +149,9 @@ jobject make_content(JNIEnv *env, const std::string &html,
   jmethodID content_ctor = env->GetMethodID(
       content_cls, "<init>",
       "(Ljava/lang/String;[Lapp/opendocument/core/Html$LocatedResource;)V");
+  if (content_ctor == nullptr) {
+    return nullptr;
+  }
   jobject result = env->NewObject(content_cls, content_ctor,
                                   to_jstring(env, html), located_array);
   env->DeleteLocalRef(content_cls);

jni/src/jni_logger.cpp

@@ -4,6 +4,7 @@
 
 #include <chrono>
 #include <memory>
+#include <stdexcept>
 #include <string>
 #include <utility>
 
@@ -150,10 +151,16 @@ class JavaLogger final : public odr::ILogger {
     clear_pending(env.get());
   }
 
+  /// False when the constructor could not resolve `LoggerBridge`, which leaves
+  /// a Java exception pending.
+  [[nodiscard]] bool valid() const {
+    return m_sink != nullptr && m_bridge != nullptr && m_will_log != nullptr &&
+           m_log != nullptr && m_flush != nullptr;
+  }
+
 private:
   [[nodiscard]] bool usable(JNIEnv *env) const {
-    return env != nullptr && m_sink != nullptr && m_bridge != nullptr &&
-           m_will_log != nullptr && m_log != nullptr && m_flush != nullptr;
+    return env != nullptr && valid();
   }
 
   /// A logger must not derail the operation it is reporting on, so an exception
@@ -196,8 +203,17 @@ Java_app_opendocument_core_Logger_createStdio(JNIEnv *env, jclass, jstring name,
 extern "C" JNIEXPORT jlong JNICALL
 Java_app_opendocument_core_Logger_createFromSink(JNIEnv *env, jclass,
                                                  jobject sink) {
-  return guarded(env, [&] {
-    return make_handle(odr::Logger(std::make_shared<JavaLogger>(env, sink)));
+  return guarded(env, [&]() -> jlong {
+    auto logger = std::make_shared<JavaLogger>(env, sink);
+    if (!logger->valid()) {
+      // no handle is made, so the value the JVM discards while throwing cannot
+      // strand an odr::Logger
+      if (env->ExceptionCheck() == JNI_FALSE) {
+        throw std::runtime_error("could not bind the Java log sink");
+      }
+      return 0;
+    }
+    return make_handle(odr::Logger(std::move(logger)));
   });
 }
 

jni/src/odr_jni.hpp

@@ -2,10 +2,12 @@
 
 #include <jni.h>
 
+#include <cstddef>
 #include <string>
 #include <string_view>
 #include <type_traits>
 #include <utility>
+#include <vector>
 
 namespace odr_jni {
 
@@ -50,4 +52,34 @@ template <typename T> void destroy_handle(JNIEnv *env, jlong handle) {
   guarded(env, [&] { delete from_handle<T>(handle); });
 }
 
+/// Owns the handles made for one Java allocation until `release` hands them
+/// over. Without it a failing `NewLongArray`/`NewObject` strands the C++ copies
+/// the handles point at.
+template <typename T> class HandleGuard {
+public:
+  explicit HandleGuard(const std::size_t capacity = 0) {
+    m_handles.reserve(capacity);
+  }
+
+  HandleGuard(const HandleGuard &) = delete;
+  HandleGuard &operator=(const HandleGuard &) = delete;
+
+  ~HandleGuard() {
+    for (const jlong handle : m_handles) {
+      delete from_handle<T>(handle);
+    }
+  }
+
+  jlong add(T value) {
+    return m_handles.emplace_back(make_handle(std::move(value)));
+  }
+
+  [[nodiscard]] const std::vector<jlong> &handles() const { return m_handles; }
+
+  void release() { m_handles.clear(); }
+
+private:
+  std::vector<jlong> m_handles;
+};
+
 } // namespace odr_jni

python/src/bind_core.cpp

@@ -36,7 +36,10 @@ void odr_python::bind_core(py::module_ &m) {
                   py::arg("path"));
 
   // Mirrors odr::Exception, so `except odr.Error` catches the whole library.
-  const py::exception<odr::Exception> error(m, "Error");
+  // `register_exception`, not `py::exception`: the latter has no translator.
+  // Registered first so it is tried last - pybind11 reverses that order.
+  const py::exception<odr::Exception> &error =
+      py::register_exception<odr::Exception>(m, "Error", PyExc_RuntimeError);
 
   py::register_exception<odr::UnsupportedOperation>(m, "UnsupportedOperation",
                                                     error);
@@ -130,19 +133,22 @@ void odr_python::bind_functions(py::module_ &m) {
       py::arg("path"), py::arg("logger") = odr::Logger::null(),
       "Determine the MIME type of a file.");
 
+  // Decoding is long-running, so it must not hold the GIL. Re-entry is safe: a
+  // Python `ILogger` re-acquires it in the trampoline.
   m.def(
       "open",
       [](const std::string &path, const odr::Logger &logger) {
         return odr::open(path, logger);
       },
       py::arg("path"), py::arg("logger") = odr::Logger::null(),
-      "Open and decode a file.");
+      py::call_guard<py::gil_scoped_release>(), "Open and decode a file.");
   m.def(
       "open",
       [](const std::string &path, const odr::FileType as,
          const odr::Logger &logger) { return odr::open(path, as, logger); },
       py::arg("path"), py::arg("as_type"),
       py::arg("logger") = odr::Logger::null(),
+      py::call_guard<py::gil_scoped_release>(),
       "Open and decode a file as a specific file type.");
   m.def(
       "open",
@@ -152,5 +158,6 @@ void odr_python::bind_functions(py::module_ &m) {
       },
       py::arg("path"), py::arg("preference"),
       py::arg("logger") = odr::Logger::null(),
+      py::call_guard<py::gil_scoped_release>(),
       "Open and decode a file with a decode preference.");
 }

python/src/bind_document.cpp

@@ -313,14 +313,17 @@ void odr_python::bind_document(py::module_ &m) {
       .def("is_editable", &odr::Document::is_editable)
       .def("is_savable", &odr::Document::is_savable,
            py::arg("encrypted") = false)
+      // saving serialises the whole document; holding the GIL for it blocks
+      // every other Python thread
       .def("save",
            py::overload_cast<const std::string &>(&odr::Document::save,
                                                   py::const_),
-           py::arg("path"))
+           py::arg("path"), py::call_guard<py::gil_scoped_release>())
       .def("save",
            py::overload_cast<const std::string &, const std::string &>(
                &odr::Document::save, py::const_),
-           py::arg("path"), py::arg("password"))
+           py::arg("path"), py::arg("password"),
+           py::call_guard<py::gil_scoped_release>())
       .def("file_type", &odr::Document::file_type)
       .def("document_type", &odr::Document::document_type)
       .def("root_element", &odr::Document::root_element, keep_self_alive)

python/src/bind_file.cpp

@@ -95,6 +95,7 @@ void odr_python::bind_file(py::module_ &m) {
       .value("video", odr::FileCategory::video);
 
   py::enum_<odr::FileLocation>(m, "FileLocation")
+      .value("unknown", odr::FileLocation::unknown)
       .value("memory", odr::FileLocation::memory)
       .value("disk", odr::FileLocation::disk);
 
@@ -179,7 +180,10 @@ void odr_python::bind_file(py::module_ &m) {
       .def("file_meta", &odr::DecodedFile::file_meta)
       .def("password_encrypted", &odr::DecodedFile::password_encrypted)
       .def("encryption_state", &odr::DecodedFile::encryption_state)
-      .def("decrypt", &odr::DecodedFile::decrypt, py::arg("password"))
+      // decrypting rewrites the whole file; holding the GIL for it blocks
+      // every other Python thread
+      .def("decrypt", &odr::DecodedFile::decrypt, py::arg("password"),
+           py::call_guard<py::gil_scoped_release>())
       .def("is_decodable", &odr::DecodedFile::is_decodable)
       .def("capabilities", &odr::DecodedFile::capabilities)
       .def("is_text_file", &odr::DecodedFile::is_text_file)
@@ -214,11 +218,13 @@ void odr_python::bind_file(py::module_ &m) {
       .def_static("type_by_path", &odr::DocumentFile::type, py::arg("path"))
       .def_static("meta_by_path", &odr::DocumentFile::meta, py::arg("path"))
       .def("document_type", &odr::DocumentFile::document_type)
-      .def("decrypt", &odr::DocumentFile::decrypt, py::arg("password"))
+      .def("decrypt", &odr::DocumentFile::decrypt, py::arg("password"),
+           py::call_guard<py::gil_scoped_release>())
       .def("document", &odr::DocumentFile::document);
 
   py::class_<odr::PdfFile, odr::DecodedFile>(m, "PdfFile")
-      .def("decrypt", &odr::PdfFile::decrypt, py::arg("password"));
+      .def("decrypt", &odr::PdfFile::decrypt, py::arg("password"),
+           py::call_guard<py::gil_scoped_release>());
 
   py::class_<odr::FontFile, odr::DecodedFile>(m, "FontFile")
       .def("read", [](const odr::FontFile &file) {