diff --git a/CHANGELOG.md b/CHANGELOG.md index 07160e6dd..6209891fa 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 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. diff --git a/jni/src/odr_jni.cpp b/jni/src/odr_jni.cpp index c8f21350f..11bb2b5a2 100644 --- a/jni/src/odr_jni.cpp +++ b/jni/src/odr_jni.cpp @@ -4,10 +4,11 @@ #include #include +#include +#include +#include #include -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 guard(chars, release); std::string result; result.reserve(static_cast(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 units; units.reserve(string.size()); for (std::size_t i = 0; i < string.size();) { - const auto byte = static_cast(string[i]); + const auto byte = static_cast(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(string[i + j]); + const auto continuation = static_cast(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(code_point)); } } + if (units.size() > + static_cast(std::numeric_limits::max())) { + throw std::length_error("Java string is too long"); + } return env->NewString(units.data(), static_cast(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::numeric_limits::max())) { + throw std::length_error("Java byte array is too long"); + } const auto length = static_cast(bytes.size()); jbyteArray result = env->NewByteArray(length); if (result == nullptr) { @@ -111,13 +133,19 @@ 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 = @@ -125,39 +153,52 @@ bool throw_coded(JNIEnv *env, const char *class_name, const char *message, if (constructor == nullptr) { env->ExceptionClear(); env->DeleteLocalRef(cls); + env->DeleteLocalRef(text); return false; } - jstring text = env->NewStringUTF(message); auto throwable = static_cast( env->NewObject(cls, constructor, text, static_cast(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 diff --git a/jni/src/odr_jni.hpp b/jni/src/odr_jni.hpp index 88ca5420a..2ec5721f9 100644 --- a/jni/src/odr_jni.hpp +++ b/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). diff --git a/jni/tests/app/opendocument/core/HtmlTest.java b/jni/tests/app/opendocument/core/HtmlTest.java index 2a47de805..0b6bb626d 100644 --- a/jni/tests/app/opendocument/core/HtmlTest.java +++ b/jni/tests/app/opendocument/core/HtmlTest.java @@ -231,6 +231,10 @@ void htmlServiceViews() throws IOException { List 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)); diff --git a/jni/tests/app/opendocument/core/LoggerTest.java b/jni/tests/app/opendocument/core/LoggerTest.java index e34cb173c..f4bbc4458 100644 --- a/jni/tests/app/opendocument/core/LoggerTest.java +++ b/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);