From afc3a6e6ce81180043451ad4ae73be154df149fa Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 03:13:28 +0200 Subject: [PATCH] fix(jni): check integer ranges at binding boundaries --- CHANGELOG.md | 3 + jni/java/app/opendocument/core/Sheet.java | 3 + jni/src/jni_core.cpp | 15 +++-- jni/src/jni_document.cpp | 55 +++++++++++-------- jni/src/jni_style.cpp | 40 +++++++------- jni/src/odr_jni.hpp | 8 +++ .../app/opendocument/core/DocumentTest.java | 22 ++++++++ jni/tests/app/opendocument/core/FileTest.java | 2 + jni/tests/app/opendocument/core/HtmlTest.java | 33 +++++++++++ jni/tests/app/opendocument/core/MetaTest.java | 6 ++ 10 files changed, 138 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6209891fa..039e72241 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 reject out-of-range coordinates, style channels, CSV delimiters + and rendering limits instead of silently wrapping their integer values. + - Java bindings preserve supplementary Unicode in exception messages, reject unpaired UTF-16 surrogates, and replace invalid native UTF-8 sequences. diff --git a/jni/java/app/opendocument/core/Sheet.java b/jni/java/app/opendocument/core/Sheet.java index 2856a9515..142f10f94 100644 --- a/jni/java/app/opendocument/core/Sheet.java +++ b/jni/java/app/opendocument/core/Sheet.java @@ -26,6 +26,9 @@ public TableDimensions content(TableDimensions range) { if (range == null) { return contentNative(handle(), -1, -1); } + if (range.rows < 0 || range.columns < 0) { + throw new IllegalArgumentException("range dimensions must be nonnegative"); + } return contentNative(handle(), range.rows, range.columns); } diff --git a/jni/src/jni_core.cpp b/jni/src/jni_core.cpp index b94c9cd79..08a8a3801 100644 --- a/jni/src/jni_core.cpp +++ b/jni/src/jni_core.cpp @@ -13,6 +13,7 @@ namespace { +using odr_jni::checked_integer; using odr_jni::from_handle; using odr_jni::guarded; using odr_jni::make_file_type_capabilities; @@ -277,10 +278,12 @@ Java_app_opendocument_core_Odr_openWithOptionsNative( options.csv.encoding = static_cast(csv_encoding); } if (csv_separator >= 0) { - options.csv.separator = static_cast(csv_separator); + options.csv.separator = + static_cast(checked_integer(csv_separator)); } if (csv_quote >= 0) { - options.csv.quote = static_cast(csv_quote); + options.csv.quote = + static_cast(checked_integer(csv_quote)); } return make_handle(odr::open(to_string(env, path), options)); }); @@ -340,7 +343,7 @@ extern "C" JNIEXPORT jint JNICALL Java_app_opendocument_core_TablePosition_toColumnNum(JNIEnv *env, jclass, jstring string) { return guarded(env, [&] { - return static_cast( + return checked_integer( odr::TablePosition::to_column_num(to_string(env, string))); }); } @@ -349,7 +352,7 @@ extern "C" JNIEXPORT jint JNICALL Java_app_opendocument_core_TablePosition_toRowNum(JNIEnv *env, jclass, jstring string) { return guarded(env, [&] { - return static_cast( + return checked_integer( odr::TablePosition::to_row_num(to_string(env, string))); }); } @@ -359,7 +362,7 @@ Java_app_opendocument_core_TablePosition_toColumnString(JNIEnv *env, jclass, jint column) { return guarded(env, [&] { return to_jstring(env, odr::TablePosition::to_column_string( - static_cast(column))); + checked_integer(column))); }); } @@ -368,6 +371,6 @@ Java_app_opendocument_core_TablePosition_toRowString(JNIEnv *env, jclass, jint row) { return guarded(env, [&] { return to_jstring(env, odr::TablePosition::to_row_string( - static_cast(row))); + checked_integer(row))); }); } diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 6c0a089b9..936839a71 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -11,6 +11,7 @@ namespace { +using odr_jni::checked_integer; using odr_jni::destroy_handle; using odr_jni::from_handle; using odr_jni::guarded; @@ -76,14 +77,14 @@ Java_app_opendocument_core_Document_recalculateNative(JNIEnv *env, jobject, std::vector stated; for (const auto *positions : {&result.changed(), &result.circular(), &result.unevaluated()}) { - stated.push_back(static_cast(positions->size())); + stated.push_back(checked_integer(positions->size())); } for (const auto *positions : {&result.changed(), &result.circular(), &result.unevaluated()}) { for (const odr::SheetPosition &position : *positions) { - stated.push_back(static_cast(position.sheet)); - stated.push_back(static_cast(position.cell.column)); - stated.push_back(static_cast(position.cell.row)); + stated.push_back(checked_integer(position.sheet)); + stated.push_back(checked_integer(position.cell.column)); + stated.push_back(checked_integer(position.cell.row)); } } jintArray array = env->NewIntArray(static_cast(stated.size())); @@ -477,9 +478,9 @@ Java_app_opendocument_core_Sheet_contentNative(JNIEnv *env, jobject, jint columns) { return guarded(env, [&] { std::optional range; - if (rows >= 0 && columns >= 0) { - range = odr::TableDimensions(static_cast(rows), - static_cast(columns)); + if (rows != -1 || columns != -1) { + range = odr::TableDimensions(checked_integer(rows), + checked_integer(columns)); } return odr_jni::make_table_dimensions( env, element(handle).as_sheet().content(range)); @@ -489,8 +490,9 @@ Java_app_opendocument_core_Sheet_contentNative(JNIEnv *env, jobject, extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_Sheet_cellNative( JNIEnv *env, jobject, jlong handle, jint column, jint row) { return guarded(env, [&] { - return wrap_element(element(handle).as_sheet().cell( - static_cast(column), static_cast(row))); + return wrap_element( + element(handle).as_sheet().cell(checked_integer(column), + checked_integer(row))); }); } @@ -516,7 +518,7 @@ Java_app_opendocument_core_Sheet_columnStyleNative(JNIEnv *env, jobject, return guarded(env, [&] { return odr_jni::make_table_column_style( env, element(handle).as_sheet().column_style( - static_cast(column))); + checked_integer(column))); }); } @@ -525,8 +527,8 @@ Java_app_opendocument_core_Sheet_rowStyleNative(JNIEnv *env, jobject, jlong handle, jint row) { return guarded(env, [&] { return odr_jni::make_table_row_style( - env, - element(handle).as_sheet().row_style(static_cast(row))); + env, element(handle).as_sheet().row_style( + checked_integer(row))); }); } @@ -537,8 +539,8 @@ Java_app_opendocument_core_Sheet_cellStyleNative(JNIEnv *env, jobject, return guarded(env, [&] { return odr_jni::make_table_cell_style( env, element(handle).as_sheet().cell_style( - static_cast(column), - static_cast(row))); + checked_integer(column), + checked_integer(row))); }); } @@ -550,7 +552,8 @@ Java_app_opendocument_core_Sheet_setCellStyleNative(JNIEnv *env, jobject, jobject text_style) { guarded(env, [&] { element(handle).as_sheet().set_cell_style( - static_cast(column), static_cast(row), + checked_integer(column), + checked_integer(row), odr_jni::table_cell_style_from_java(env, cell_style), odr_jni::text_style_from_java(env, text_style)); }); @@ -563,7 +566,7 @@ Java_app_opendocument_core_Sheet_setRowStyleNative(JNIEnv *env, jobject, jobject text_style) { guarded(env, [&] { element(handle).as_sheet().set_row_style( - static_cast(row), + checked_integer(row), odr_jni::table_cell_style_from_java(env, cell_style), odr_jni::text_style_from_java(env, text_style)); }); @@ -576,7 +579,7 @@ Java_app_opendocument_core_Sheet_setColumnStyleNative(JNIEnv *env, jobject, jobject text_style) { guarded(env, [&] { element(handle).as_sheet().set_column_style( - static_cast(column), + checked_integer(column), odr_jni::table_cell_style_from_java(env, cell_style), odr_jni::text_style_from_java(env, text_style)); }); @@ -587,8 +590,9 @@ Java_app_opendocument_core_Sheet_insertRowsNative(JNIEnv *env, jobject, jlong handle, jint row, jint count) { guarded(env, [&] { - element(handle).as_sheet().insert_rows(static_cast(row), - static_cast(count)); + element(handle).as_sheet().insert_rows( + checked_integer(row), + checked_integer(count)); }); } @@ -597,8 +601,9 @@ Java_app_opendocument_core_Sheet_deleteRowsNative(JNIEnv *env, jobject, jlong handle, jint row, jint count) { guarded(env, [&] { - element(handle).as_sheet().delete_rows(static_cast(row), - static_cast(count)); + element(handle).as_sheet().delete_rows( + checked_integer(row), + checked_integer(count)); }); } @@ -608,7 +613,8 @@ Java_app_opendocument_core_Sheet_insertColumnsNative(JNIEnv *env, jobject, jint count) { guarded(env, [&] { element(handle).as_sheet().insert_columns( - static_cast(column), static_cast(count)); + checked_integer(column), + checked_integer(count)); }); } @@ -618,7 +624,8 @@ Java_app_opendocument_core_Sheet_deleteColumnsNative(JNIEnv *env, jobject, jint count) { guarded(env, [&] { element(handle).as_sheet().delete_columns( - static_cast(column), static_cast(count)); + checked_integer(column), + checked_integer(count)); }); } @@ -833,7 +840,7 @@ Java_app_opendocument_core_ListItem_numberNative(JNIEnv *env, jobject, if (const std::optional value = element(handle).as_list_item().number(); value.has_value()) { - number = static_cast(*value); + number = checked_integer(*value); } return odr_jni::make_integer_opt(env, number); }); diff --git a/jni/src/jni_style.cpp b/jni/src/jni_style.cpp index b9ab626ac..5ba42afb3 100644 --- a/jni/src/jni_style.cpp +++ b/jni/src/jni_style.cpp @@ -71,7 +71,8 @@ jobject box_long(JNIEnv *env, const std::optional &value) { return nullptr; } return call_static_object(env, "java/lang/Long", "valueOf", - "(J)Ljava/lang/Long;", static_cast(*value)); + "(J)Ljava/lang/Long;", + checked_integer(*value)); } jobject box_integer(JNIEnv *env, const std::optional &value) { @@ -80,7 +81,7 @@ jobject box_integer(JNIEnv *env, const std::optional &value) { } return call_static_object(env, "java/lang/Integer", "valueOf", "(I)Ljava/lang/Integer;", - static_cast(*value)); + checked_integer(*value)); } jobject box_long(JNIEnv *env, const std::optional &value) { @@ -88,7 +89,8 @@ jobject box_long(JNIEnv *env, const std::optional &value) { return nullptr; } return call_static_object(env, "java/lang/Long", "valueOf", - "(J)Ljava/lang/Long;", static_cast(*value)); + "(J)Ljava/lang/Long;", + checked_integer(*value)); } /// Looks up an enum constant by its C++ code (= Java ordinal). @@ -145,7 +147,7 @@ jobject make_integer_opt(JNIEnv *env, } return call_static_object(env, "java/lang/Integer", "valueOf", "(I)Ljava/lang/Integer;", - static_cast(*value)); + checked_integer(*value)); } jobject make_measure(JNIEnv *env, const odr::Measure &value) { @@ -263,7 +265,7 @@ std::optional color_from_java(JNIEnv *env, const jobject value) { } jclass cls = env->GetObjectClass(value); const auto channel = [&](const char *name) { - return static_cast( + return checked_integer( env->GetIntField(value, env->GetFieldID(cls, name, "I"))); }; const odr::Color result(channel("red"), channel("green"), channel("blue"), @@ -562,8 +564,8 @@ jobject make_page_layout(JNIEnv *env, const odr::PageLayout &layout) { jobject make_table_dimensions(JNIEnv *env, const odr::TableDimensions &dimensions) { return new_object(env, "app/opendocument/core/TableDimensions", "(II)V", - static_cast(dimensions.rows), - static_cast(dimensions.columns)); + checked_integer(dimensions.rows), + checked_integer(dimensions.columns)); } jobject make_html_sheet_cut(JNIEnv *env, @@ -580,8 +582,8 @@ jobject make_html_sheet_cut(JNIEnv *env, jobject make_table_position(JNIEnv *env, const odr::TablePosition &position) { return new_object(env, "app/opendocument/core/TablePosition", "(II)V", - static_cast(position.column), - static_cast(position.row)); + checked_integer(position.column), + checked_integer(position.row)); } jobject make_file_meta(JNIEnv *env, const odr::FileMeta &meta) { @@ -692,7 +694,7 @@ jobject html_config_to_java(JNIEnv *env, const odr::HtmlConfig &config) { box_long(env, config.spreadsheet_cell_limit)); set_boolean("spreadsheetLimitByContent", config.spreadsheet_limit_by_content); set_long("spreadsheetStyleBuffer", - static_cast(config.spreadsheet_style_buffer)); + checked_integer(config.spreadsheet_style_buffer)); set_object("spreadsheetGridlines", "Lapp/opendocument/core/HtmlTableGridlines;", enum_from_code(env, "app/opendocument/core/HtmlTableGridlines", @@ -717,7 +719,7 @@ jobject html_config_to_java(JNIEnv *env, const odr::HtmlConfig &config) { set_boolean("formatHtml", config.format_html); set_int("htmlIndent", config.html_indent); set_string("htmlIndentString", config.html_indent_string); - set_int("pageRangeBegin", static_cast(config.page_range_begin)); + set_int("pageRangeBegin", checked_integer(config.page_range_begin)); set_object("pageRangeEnd", "Ljava/lang/Integer;", box_integer(env, config.page_range_end)); set_object("pdfTextMode", "Lapp/opendocument/core/PdfTextMode;", @@ -841,8 +843,8 @@ odr::HtmlConfig html_config_from_java(JNIEnv *env, jobject config) { const jint columns = env->GetIntField( limit, env->GetFieldID(dimensions_cls, "columns", "I")); result.spreadsheet_limit = - odr::TableDimensions(static_cast(rows), - static_cast(columns)); + odr::TableDimensions(checked_integer(rows), + checked_integer(columns)); env->DeleteLocalRef(dimensions_cls); } } @@ -853,13 +855,13 @@ odr::HtmlConfig html_config_from_java(JNIEnv *env, jobject config) { } else { jclass long_cls = env->GetObjectClass(cell_limit); jmethodID long_value = env->GetMethodID(long_cls, "longValue", "()J"); - result.spreadsheet_cell_limit = static_cast( + result.spreadsheet_cell_limit = checked_integer( env->CallLongMethod(cell_limit, long_value)); env->DeleteLocalRef(long_cls); } } result.spreadsheet_style_buffer = - static_cast(get_long("spreadsheetStyleBuffer")); + checked_integer(get_long("spreadsheetStyleBuffer")); result.spreadsheet_limit_by_content = get_boolean("spreadsheetLimitByContent"); { @@ -895,7 +897,7 @@ odr::HtmlConfig html_config_from_java(JNIEnv *env, jobject config) { jclass integer_cls = env->GetObjectClass(width); jmethodID int_value = env->GetMethodID(integer_cls, "intValue", "()I"); result.viewport_width = - static_cast(env->CallIntMethod(width, int_value)); + checked_integer(env->CallIntMethod(width, int_value)); env->DeleteLocalRef(integer_cls); } env->DeleteLocalRef(width); @@ -920,10 +922,10 @@ odr::HtmlConfig html_config_from_java(JNIEnv *env, jobject config) { env->DeleteLocalRef(margin); } result.format_html = get_boolean("formatHtml"); - result.html_indent = static_cast(get_int("htmlIndent")); + result.html_indent = checked_integer(get_int("htmlIndent")); result.html_indent_string = get_string("htmlIndentString"); result.page_range_begin = - static_cast(get_int("pageRangeBegin")); + checked_integer(get_int("pageRangeBegin")); { jobject end = get_object("pageRangeEnd", "Ljava/lang/Integer;"); if (end == nullptr) { @@ -932,7 +934,7 @@ odr::HtmlConfig html_config_from_java(JNIEnv *env, jobject config) { jclass integer_cls = env->GetObjectClass(end); jmethodID int_value = env->GetMethodID(integer_cls, "intValue", "()I"); result.page_range_end = - static_cast(env->CallIntMethod(end, int_value)); + checked_integer(env->CallIntMethod(end, int_value)); env->DeleteLocalRef(integer_cls); } } diff --git a/jni/src/odr_jni.hpp b/jni/src/odr_jni.hpp index 2ec5721f9..30ec77e8f 100644 --- a/jni/src/odr_jni.hpp +++ b/jni/src/odr_jni.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -37,6 +38,13 @@ template auto guarded(JNIEnv *env, F &&f) -> decltype(f()) { } } +template To checked_integer(const From value) { + if (!std::in_range(value)) { + throw std::out_of_range("integer is outside the binding's range"); + } + return static_cast(value); +} + template T *from_handle(jlong handle) { return reinterpret_cast(handle); } diff --git a/jni/tests/app/opendocument/core/DocumentTest.java b/jni/tests/app/opendocument/core/DocumentTest.java index 8d5ea9734..3dd9484c6 100644 --- a/jni/tests/app/opendocument/core/DocumentTest.java +++ b/jni/tests/app/opendocument/core/DocumentTest.java @@ -219,6 +219,11 @@ void setStyleMarksARun() throws IOException { TextStyle style = new TextStyle(); style.fontWeight = FontWeight.BOLD; style.fontSize = new Measure(14, "pt"); + for (Color invalid : new Color[] {new Color(-1, 0, 0), new Color(0, 256, 0), + new Color(0, 0, -1), new Color(0, 0, 0, 256)}) { + style.backgroundColor = invalid; + assertThrows(OdrException.class, () -> run.setStyle(style)); + } style.backgroundColor = new Color(255, 255, 0); run.setStyle(style); @@ -321,6 +326,23 @@ void setRowAndColumnStyleReachPastTheCells() throws IOException { assertEquals(new Color(255, 255, 0), reloadedSheet.cellStyle(30, 90).backgroundColor); } + @Test + void negativeSheetCoordinatesAreRejected() throws IOException { + try (DecodedFile file = Odr.open(TestFiles.odsFile(tempDir).toString())) { + Document document = file.asDocumentFile().document(); + Sheet sheet = document.rootElement().firstChild().asSheet(); + assertThrows(OdrException.class, () -> sheet.cell(-1, 0)); + assertThrows(OdrException.class, () -> sheet.cell(0, -1)); + assertThrows(OdrException.class, () -> sheet.insertRows(0, -1)); + assertThrows(OdrException.class, () -> sheet.deleteColumns(-1, 1)); + assertThrows(IllegalArgumentException.class, + () -> sheet.content(new TableDimensions(-1, 1))); + assertThrows(IllegalArgumentException.class, + () -> sheet.content(new TableDimensions(-1, -1))); + assertNotNull(sheet.content(null)); + } + } + @Test void insertAndDeleteRowsMoveTheCells() throws IOException { Path ods = TestFiles.odsFile(tempDir); diff --git a/jni/tests/app/opendocument/core/FileTest.java b/jni/tests/app/opendocument/core/FileTest.java index 5a98a2aba..64a58fa74 100644 --- a/jni/tests/app/opendocument/core/FileTest.java +++ b/jni/tests/app/opendocument/core/FileTest.java @@ -165,6 +165,8 @@ 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.csv.separator = '\u017c'; + assertThrows(OdrException.class, () -> Odr.open(path.toString(), options)); options.csv.separator = '|'; try (DecodedFile file = Odr.open(path.toString(), options)) { diff --git a/jni/tests/app/opendocument/core/HtmlTest.java b/jni/tests/app/opendocument/core/HtmlTest.java index 0b6bb626d..83f1f27c0 100644 --- a/jni/tests/app/opendocument/core/HtmlTest.java +++ b/jni/tests/app/opendocument/core/HtmlTest.java @@ -10,6 +10,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.List; +import java.util.Arrays; +import java.util.function.Consumer; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -56,6 +58,37 @@ void htmlConfigDefaults() { assertNull(config.initialZoom); } + @Test + void invalidIntegerLimitsAreRejected() throws IOException { + List> invalid = Arrays.asList( + c -> c.spreadsheetLimit = new TableDimensions(-1, 1), + c -> c.spreadsheetLimit = new TableDimensions(1, -1), + c -> c.spreadsheetCellLimit = -1L, + c -> c.spreadsheetStyleBuffer = -1L, + c -> c.viewportWidth = -1, + c -> c.htmlIndent = -1, + c -> c.htmlIndent = 256, + c -> c.pageRangeBegin = -1, + c -> c.pageRangeEnd = -1); + try (DecodedFile file = Odr.open(TestFiles.odtFile(tempDir).toString())) { + for (Consumer change : invalid) { + HtmlConfig config = new HtmlConfig(); + change.accept(config); + assertThrows(OdrException.class, () -> Html.translate(file, config)); + } + HtmlConfig config = new HtmlConfig(); + config.htmlIndent = 255; + config.pageRangeEnd = Integer.MAX_VALUE; + config.spreadsheetCellLimit = Long.MAX_VALUE; + try (HtmlService service = Html.translate(file, config)) { + HtmlConfig result = service.config(); + assertEquals(255, result.htmlIndent); + assertEquals(Integer.valueOf(Integer.MAX_VALUE), result.pageRangeEnd); + assertEquals(Long.valueOf(Long.MAX_VALUE), result.spreadsheetCellLimit); + } + } + } + @Test void viewportConfigRoundTrips() throws IOException { HtmlConfig config = new HtmlConfig(); diff --git a/jni/tests/app/opendocument/core/MetaTest.java b/jni/tests/app/opendocument/core/MetaTest.java index c5a917de7..6179f92ee 100644 --- a/jni/tests/app/opendocument/core/MetaTest.java +++ b/jni/tests/app/opendocument/core/MetaTest.java @@ -1,6 +1,7 @@ package app.opendocument.core; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -99,5 +100,10 @@ void tablePosition() { assertEquals(0, TablePosition.toColumnNum("A")); assertEquals("A", TablePosition.toColumnString(0)); assertEquals("A1", new TablePosition(0, 0).toString()); + assertEquals(Integer.MAX_VALUE, TablePosition.toRowNum("2147483648")); + assertEquals("2147483648", TablePosition.toRowString(Integer.MAX_VALUE)); + assertThrows(OdrException.class, () -> TablePosition.toRowNum("2147483649")); + assertThrows(OdrException.class, () -> TablePosition.toColumnString(-1)); + assertThrows(OdrException.class, () -> TablePosition.toRowString(-1)); } }