fix(jni): preserve Unicode across string and exception boundaries

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Java bindings preserve supplementary Unicode in exception messages, reject
+  unpaired UTF-16 surrogates, and replace invalid native UTF-8 sequences.
+
 - Python `Frame.line()` returns a bound `DrawingLine` with its endpoints instead
   of raising a conversion error.
 

jni/src/odr_jni.cpp

@@ -4,10 +4,11 @@
 #include <odr/exceptions.hpp>
 
 #include <cstdint>
+#include <limits>
+#include <memory>
+#include <stdexcept>
 #include <vector>
 
-namespace odr_jni {
-
 namespace {
 
 void append_utf8(std::string &out, const std::uint32_t code_point) {
@@ -30,15 +31,19 @@ void append_utf8(std::string &out, const std::uint32_t code_point) {
 
 } // namespace
 
-std::string to_string(JNIEnv *env, jstring string) {
+std::string odr_jni::to_string(JNIEnv *env, jstring string) {
   if (string == nullptr) {
     return {};
   }
   const jsize length = env->GetStringLength(string);
   const jchar *chars = env->GetStringChars(string, nullptr);
   if (chars == nullptr) {
-    return {};
+    throw std::bad_alloc();
   }
+  const auto release = [env, string](const jchar *data) {
+    env->ReleaseStringChars(string, data);
+  };
+  const std::unique_ptr<const jchar, decltype(release)> guard(chars, release);
   std::string result;
   result.reserve(static_cast<std::size_t>(length));
   for (jsize i = 0; i < length; ++i) {
@@ -49,37 +54,39 @@ std::string to_string(JNIEnv *env, jstring string) {
           0x10000 + ((code_point - 0xd800) << 10) + (chars[i + 1] - 0xdc00);
       ++i;
     }
+    if (code_point >= 0xd800 && code_point <= 0xdfff) {
+      throw std::invalid_argument("unpaired UTF-16 surrogate");
+    }
     append_utf8(result, code_point);
   }
-  env->ReleaseStringChars(string, chars);
   return result;
 }
 
-jstring to_jstring(JNIEnv *env, const std::string_view string) {
+jstring odr_jni::to_jstring(JNIEnv *env, const std::string_view string) {
   std::vector<jchar> units;
   units.reserve(string.size());
   for (std::size_t i = 0; i < string.size();) {
-    const auto byte = static_cast<unsigned char>(string[i]);
+    const auto byte = static_cast<std::uint8_t>(string[i]);
     std::uint32_t code_point = 0xfffd;
     std::size_t sequence_length = 1;
     if (byte < 0x80) {
       code_point = byte;
-    } else if ((byte >> 5) == 0x6) {
+    } else if (byte >= 0xc2 && byte <= 0xdf) {
       code_point = byte & 0x1f;
       sequence_length = 2;
     } else if ((byte >> 4) == 0xe) {
       code_point = byte & 0x0f;
       sequence_length = 3;
-    } else if ((byte >> 3) == 0x1e) {
+    } else if (byte >= 0xf0 && byte <= 0xf4) {
       code_point = byte & 0x07;
       sequence_length = 4;
     }
-    if (i + sequence_length > string.size()) {
+    if (sequence_length > string.size() - i) {
       code_point = 0xfffd;
       sequence_length = 1;
     } else {
       for (std::size_t j = 1; j < sequence_length; ++j) {
-        const auto continuation = static_cast<unsigned char>(string[i + j]);
+        const auto continuation = static_cast<std::uint8_t>(string[i + j]);
         if ((continuation >> 6) != 0x2) {
           code_point = 0xfffd;
           sequence_length = 1;
@@ -88,6 +95,13 @@ jstring to_jstring(JNIEnv *env, const std::string_view string) {
         code_point = (code_point << 6) | (continuation & 0x3f);
       }
     }
+    if ((sequence_length == 2 && code_point < 0x80) ||
+        (sequence_length == 3 && code_point < 0x800) ||
+        (sequence_length == 4 && code_point < 0x10000) ||
+        (code_point >= 0xd800 && code_point <= 0xdfff) ||
+        code_point > 0x10ffff) {
+      code_point = 0xfffd;
+    }
     i += sequence_length;
     if (code_point >= 0x10000) {
       const std::uint32_t offset = code_point - 0x10000;
@@ -97,10 +111,18 @@ jstring to_jstring(JNIEnv *env, const std::string_view string) {
       units.push_back(static_cast<jchar>(code_point));
     }
   }
+  if (units.size() >
+      static_cast<std::size_t>(std::numeric_limits<jsize>::max())) {
+    throw std::length_error("Java string is too long");
+  }
   return env->NewString(units.data(), static_cast<jsize>(units.size()));
 }
 
-jbyteArray to_jbytes(JNIEnv *env, const std::string_view bytes) {
+jbyteArray odr_jni::to_jbytes(JNIEnv *env, const std::string_view bytes) {
+  if (bytes.size() >
+      static_cast<std::size_t>(std::numeric_limits<jsize>::max())) {
+    throw std::length_error("Java byte array is too long");
+  }
   const auto length = static_cast<jsize>(bytes.size());
   jbyteArray result = env->NewByteArray(length);
   if (result == nullptr) {
@@ -111,53 +133,72 @@ jbyteArray to_jbytes(JNIEnv *env, const std::string_view bytes) {
   return result;
 }
 
-/// Throws @p class_name, constructed from `(String, int)`. False where the
-/// class is absent.
+namespace {
+
+/// Returns false if the exception class or constructor is absent.
 bool throw_coded(JNIEnv *env, const char *class_name, const char *message,
                  const odr::ErrorCode code) {
+  jstring text = odr_jni::to_jstring(env, message);
+  if (text == nullptr) {
+    return true;
+  }
   jclass cls = env->FindClass(class_name);
   if (cls == nullptr) {
     env->ExceptionClear();
+    env->DeleteLocalRef(text);
     return false;
   }
   const jmethodID constructor =
       env->GetMethodID(cls, "<init>", "(Ljava/lang/String;I)V");
   if (constructor == nullptr) {
     env->ExceptionClear();
     env->DeleteLocalRef(cls);
+    env->DeleteLocalRef(text);
     return false;
   }
-  jstring text = env->NewStringUTF(message);
   auto throwable = static_cast<jthrowable>(
       env->NewObject(cls, constructor, text, static_cast<jint>(code)));
   env->DeleteLocalRef(text);
   env->DeleteLocalRef(cls);
   if (throwable == nullptr) {
-    return false; // an OutOfMemoryError is pending instead
+    return true; // Preserve the pending allocation exception.
   }
   env->Throw(throwable);
   env->DeleteLocalRef(throwable);
   return true;
 }
 
-void throw_java(JNIEnv *env) {
-  constexpr auto base = "app/opendocument/core/OdrException";
+} // namespace
+
+void odr_jni::throw_java(JNIEnv *env) noexcept {
+  if (env->ExceptionCheck()) {
+    return;
+  }
   try {
-    throw;
-  } catch (const std::exception &e) {
-    const odr::ErrorCode code = odr::error_code(e);
-    // The nested class is named after the code; one with no class of its own
-    // falls back to the base.
-    const std::string name =
-        std::string(base) + "$" + std::string(odr::error_code_name(code));
-    if (code != odr::ErrorCode::unknown &&
-        throw_coded(env, name.c_str(), e.what(), code)) {
-      return;
+    constexpr auto base = "app/opendocument/core/OdrException";
+    try {
+      throw;
+    } catch (const std::exception &e) {
+      const odr::ErrorCode code = odr::error_code(e);
+      // The nested class is named after the code; one with no class of its own
+      // falls back to the base.
+      const std::string name =
+          std::string(base) + "$" + std::string(odr::error_code_name(code));
+      if (code != odr::ErrorCode::unknown &&
+          throw_coded(env, name.c_str(), e.what(), code)) {
+        return;
+      }
+      throw_coded(env, base, e.what(), code);
+    } catch (...) {
+      throw_coded(env, base, "unknown native error", odr::ErrorCode::unknown);
     }
-    throw_coded(env, base, e.what(), code);
   } catch (...) {
-    throw_coded(env, base, "unknown native error", odr::ErrorCode::unknown);
+    if (!env->ExceptionCheck()) {
+      const jclass cls = env->FindClass("java/lang/OutOfMemoryError");
+      if (cls != nullptr) {
+        env->ThrowNew(cls, "could not construct native exception");
+        env->DeleteLocalRef(cls);
+      }
+    }
   }
 }
-
-} // namespace odr_jni

jni/src/odr_jni.hpp

@@ -11,17 +11,17 @@
 
 namespace odr_jni {
 
-/// Java string (UTF-16) to UTF-8. Handles nullptr and supplementary planes
-/// (unlike JNI's modified UTF-8 accessors).
+/// Converts UTF-16 to UTF-8; nullptr is empty and unpaired surrogates are
+/// rejected.
 std::string to_string(JNIEnv *env, jstring string);
-/// UTF-8 to Java string (UTF-16).
+/// Converts UTF-8 to UTF-16, replacing invalid sequences with U+FFFD.
 jstring to_jstring(JNIEnv *env, std::string_view string);
 
 jbyteArray to_jbytes(JNIEnv *env, std::string_view bytes);
 
 /// Rethrows the pending C++ exception as the matching Java exception
 /// (`app.opendocument.core.OdrException` and subclasses).
-void throw_java(JNIEnv *env);
+void throw_java(JNIEnv *env) noexcept;
 
 /// Runs `f`, converting any C++ exception into a pending Java exception and
 /// returning a value-initialized result (ignored by the JVM in that case).

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

@@ -231,6 +231,10 @@ void htmlServiceViews() throws IOException {
     List<HtmlView> views = service.listViews();
     assertEquals(1, views.size());
     assertTrue(service.exists(views.get(0).path()));
+    String missing = "missing-😀.html";
+    OdrException error =
+        assertThrows(OdrException.FileNotFound.class, () -> service.write(missing));
+    assertEquals("file not found: Unknown path: " + missing, error.getMessage());
 
     Html.Content content = views.get(0).writeHtml();
     assertTrue(content.html.contains(TestFiles.ODT_WORD));

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

@@ -4,6 +4,7 @@
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.io.IOException;
 import java.nio.file.Path;
@@ -59,15 +60,26 @@ void customSinkReceivesMessages() {
       assertTrue(logger.willLog(LogLevel.ERROR));
 
       logger.log(LogLevel.DEBUG, "dropped");
-      logger.log(LogLevel.ERROR, "kept");
+      logger.log(LogLevel.ERROR, "kept é😀\u0000");
       logger.flush();
     }
 
-    assertEquals(List.of("kept"), sink.messages);
+    assertEquals(List.of("kept é😀\u0000"), sink.messages);
     assertEquals(1, sink.flushes);
     assertNotNull(sink.locations.get(0).fileName);
   }
 
+  @Test
+  void malformedUnicodeIsRejected() {
+    Collecting sink = new Collecting(LogLevel.ERROR);
+    try (Logger logger = new Logger(sink)) {
+      for (String text : new String[] {"\ud800", "\udc00", "\ud800x", "\udc00\ud800"}) {
+        assertThrows(OdrException.class, () -> logger.log(LogLevel.ERROR, text));
+      }
+    }
+    assertTrue(sink.messages.isEmpty());
+  }
+
   @Test
   void customSinkIsUsableWhileOpening() throws IOException {
     Path odt = TestFiles.odtFile(tempDir);