From 2c0362c4b644a4d017cfcf5f8f8888a12da069d0 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 4 Oct 2026 22:53:18 +0200 Subject: [PATCH 1/2] fix: keep table spans compact and bound their extents --- CHANGELOG.md | 4 ++ .../internal/common/sheet_recalculation.cpp | 15 +++-- src/odr/internal/common/table_cursor.cpp | 66 +++++++++---------- src/odr/internal/common/table_cursor.hpp | 11 ++-- src/odr/internal/odf/AGENTS.md | 3 +- src/odr/internal/odf/odf_parser.cpp | 2 - src/odr/table_position.cpp | 2 +- .../src/internal/common/table_cursor_test.cpp | 29 ++++++++ test/src/sheet_recalculation_test.cpp | 9 +++ test/src/table_position_test.cpp | 4 ++ 10 files changed, 98 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 668bf0e77..b1a265742 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- Table spans use storage proportional to merged cells, survive skipped rows, + and reject overflowing dimensions. Recalculation bounds formula spans before + multiplying or accumulating their size. + - CSV preserves carriage-return record endings and separator directives when options are reapplied. Out-of-range coordinates and growth are rejected before cell IDs or allocation sizes can wrap. diff --git a/src/odr/internal/common/sheet_recalculation.cpp b/src/odr/internal/common/sheet_recalculation.cpp index 359a07724..cedc2da29 100644 --- a/src/odr/internal/common/sheet_recalculation.cpp +++ b/src/odr/internal/common/sheet_recalculation.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -245,19 +246,25 @@ internal::recalculate(const abstract::Document &document) { sheet_id, [&](const std::uint32_t column, const std::uint32_t row, const TableDimensions &span, const bool array, const std::string &text) { + constexpr auto max = std::numeric_limits::max(); + if (span.rows == 0 || span.columns == 0 || span.rows > max - row || + span.columns > max - column) { + throw UnsupportedOperation(); + } const SheetPosition position(index, column, row); const bool spanned = array || span.rows > 1 || span.columns > 1; formulas[position] = Formula{sheet_id, formula::parse(text, *syntax), spanned}; if (spanned) { + const std::uint64_t count = std::uint64_t{span.rows} * span.columns; + if (count > span_limit - spanned_positions) { + throw UnsupportedOperation(); + } + spanned_positions += static_cast(count); spans.push_back(Span{position, span, array}); - spanned_positions += std::size_t{span.rows} * span.columns; } }); } - if (spanned_positions > span_limit) { - throw UnsupportedOperation(); - } for (const Span &span : spans) { const ElementIdentifier sheet_id = formulas.at(span.first).sheet_id; for_each_position(span, [&](const SheetPosition &position) { diff --git a/src/odr/internal/common/table_cursor.cpp b/src/odr/internal/common/table_cursor.cpp index 7aca96278..ecf2df999 100644 --- a/src/odr/internal/common/table_cursor.cpp +++ b/src/odr/internal/common/table_cursor.cpp @@ -1,49 +1,51 @@ #include #include -#include +#include +#include namespace odr::internal { -TableCursor::TableCursor() { m_sparse.emplace_back(); } +namespace { -void TableCursor::add_column(const std::uint32_t repeat) noexcept { - m_column += repeat; +std::uint32_t advance(const std::uint32_t position, const std::uint64_t count) { + if (count == 0 || + count > std::numeric_limits::max() - position) { + throw std::out_of_range("table extent out of range"); + } + return position + static_cast(count); +} + +} // namespace + +void TableCursor::add_column(const std::uint32_t repeat) { + m_column = advance(m_column, repeat); } void TableCursor::add_row(const std::uint32_t repeat) { - m_row += repeat; + m_row = advance(m_row, repeat); m_column = 0; - if (repeat > 1) { - // TODO assert trivial - m_sparse.clear(); - } else if (repeat == 1) { - m_sparse.pop_front(); - } - if (m_sparse.empty()) { - m_sparse.emplace_back(); - } + std::erase_if(m_spans, + [&](const Range &span) { return span.end_row <= m_row; }); + m_next_span = 0; handle_rowspan_(); } void TableCursor::add_cell(const std::uint32_t colspan, const std::uint32_t rowspan, const std::uint32_t repeat) { - const std::uint32_t next_column = m_column + colspan * repeat; - - // handle rowspan; keep each row's ranges sorted by start so - // `handle_rowspan_` can skip them front-to-back - auto it = std::begin(m_sparse); - for (std::uint32_t i = 1; i < rowspan; ++i) { - if (std::next(it) == std::end(m_sparse)) { - m_sparse.emplace_back(); + const std::uint32_t next_column = + advance(m_column, std::uint64_t{colspan} * repeat); + const std::uint32_t end_row = advance(m_row, rowspan); + if (rowspan > 1) { + const auto position = + std::ranges::upper_bound(m_spans, m_column, {}, &Range::start); + const auto index = static_cast(position - m_spans.begin()); + m_spans.insert(position, Range{m_column, next_column, end_row}); + if (index < m_next_span) { + ++m_next_span; } - ++it; - const auto pos = std::ranges::find_if( - *it, [&](const Range &range) { return range.start > m_column; }); - it->insert(pos, Range{m_column, next_column}); } - m_column = next_column; handle_rowspan_(); } @@ -57,13 +59,11 @@ std::uint32_t TableCursor::column() const noexcept { return m_column; } std::uint32_t TableCursor::row() const noexcept { return m_row; } void TableCursor::handle_rowspan_() noexcept { - auto &s = m_sparse.front(); - auto it = std::begin(s); - while (it != std::end(s) && it->start <= m_column) { - m_column = std::max(m_column, it->end); - ++it; + while (m_next_span < m_spans.size() && + m_spans[m_next_span].start <= m_column) { + m_column = std::max(m_column, m_spans[m_next_span].end); + ++m_next_span; } - s.erase(std::begin(s), it); } } // namespace odr::internal diff --git a/src/odr/internal/common/table_cursor.hpp b/src/odr/internal/common/table_cursor.hpp index 0ec3f8ec0..e6c7f600c 100644 --- a/src/odr/internal/common/table_cursor.hpp +++ b/src/odr/internal/common/table_cursor.hpp @@ -2,16 +2,15 @@ #include +#include #include -#include +#include namespace odr::internal { class TableCursor final { public: - TableCursor(); - - void add_column(std::uint32_t repeat = 1) noexcept; + void add_column(std::uint32_t repeat = 1); void add_row(std::uint32_t repeat = 1); void add_cell(std::uint32_t colspan = 1, std::uint32_t rowspan = 1, std::uint32_t repeat = 1); @@ -24,11 +23,13 @@ class TableCursor final { struct Range { std::uint32_t start{0}; std::uint32_t end{0}; + std::uint32_t end_row{0}; }; std::uint32_t m_column{0}; std::uint32_t m_row{0}; - std::list> m_sparse; + std::vector m_spans; + std::size_t m_next_span{0}; void handle_rowspan_() noexcept; }; diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 5886c7d20..b08063b6f 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -206,8 +206,7 @@ unknown mimetype are tolerated. 2. **No streaming.** Crypto and save read whole files into memory and rebuild the whole ZIP (`// TODO stream`). 3. **Covered and repeated cells are heuristic.** `// TODO covered cells` and - `// TODO mark as repeated` in `odf_parser.cpp`. A rowspan out of a repeated - row is dropped. + `// TODO mark as repeated` in `odf_parser.cpp`. 4. **Style gaps.** `transparent` and alpha colours give `nullopt` (`// TODO use alpha`). The style-versus-element cascade is provisional (`// TODO use override?`). `text:outline-style` is indexed but not applied diff --git a/src/odr/internal/odf/odf_parser.cpp b/src/odr/internal/odf/odf_parser.cpp index 139e22cc1..b9adcfaa6 100644 --- a/src/odr/internal/odf/odf_parser.cpp +++ b/src/odr/internal/odf/odf_parser.cpp @@ -226,8 +226,6 @@ void index_sheet_rows(ElementRegistry ®istry, cursor.add_cell(colspan, rowspan, columns_repeated); } - // TODO a rowspan out of a repeated row is dropped - `add_row` clears the - // cursor's pending ranges for a repeat > 1 cursor.add_row(rows_repeated); }); diff --git a/src/odr/table_position.cpp b/src/odr/table_position.cpp index 1c1783e60..45b7d7e8e 100644 --- a/src/odr/table_position.cpp +++ b/src/odr/table_position.cpp @@ -91,7 +91,7 @@ std::string TablePosition::to_column_string(const std::uint32_t column) { } std::string TablePosition::to_row_string(const std::uint32_t row) { - return std::to_string(row + 1); + return std::to_string(std::uint64_t{row} + 1); } TablePosition::TablePosition(const std::string &s) { diff --git a/test/src/internal/common/table_cursor_test.cpp b/test/src/internal/common/table_cursor_test.cpp index 83c343240..ea36a9561 100644 --- a/test/src/internal/common/table_cursor_test.cpp +++ b/test/src/internal/common/table_cursor_test.cpp @@ -2,6 +2,9 @@ #include +#include +#include + using namespace odr::internal; TEST(TableCursor, test) { @@ -45,3 +48,29 @@ TEST(TableCursor, out_of_order_rowspans) { cursor.add_row(); EXPECT_EQ(2, cursor.column()); // A3 and B3 covered, skipped } + +TEST(TableCursor, long_spans_survive_skipped_rows) { + constexpr auto max = std::numeric_limits::max(); + TableCursor cursor; + cursor.add_cell(2, max); + cursor.add_row(max - 1); + EXPECT_EQ(cursor.row(), max - 1); + EXPECT_EQ(cursor.column(), 2); + cursor.add_row(); + EXPECT_EQ(cursor.column(), 0); +} + +TEST(TableCursor, rejects_invalid_extents_before_advancing) { + constexpr auto max = std::numeric_limits::max(); + TableCursor cursor; + cursor.add_column(1); + EXPECT_THROW(cursor.add_column(max), std::out_of_range); + EXPECT_THROW(cursor.add_cell(max, 1, max), std::out_of_range); + EXPECT_THROW(cursor.add_cell(0), std::out_of_range); + EXPECT_THROW(cursor.add_cell(1, 0), std::out_of_range); + EXPECT_EQ(cursor.column(), 1); + EXPECT_EQ(cursor.row(), 0); + cursor.add_row(max); + EXPECT_THROW(cursor.add_row(), std::out_of_range); + EXPECT_EQ(cursor.row(), max); +} diff --git a/test/src/sheet_recalculation_test.cpp b/test/src/sheet_recalculation_test.cpp index 50648a0db..b88c8a153 100644 --- a/test/src/sheet_recalculation_test.cpp +++ b/test/src/sheet_recalculation_test.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -288,3 +289,11 @@ TEST(SheetRecalculation, an_unedited_document_keeps_its_results) { EXPECT_TRUE(document.recalculate().changed().empty()); } + +TEST(SheetRecalculation, oversized_array_spans_are_rejected_before_expansion) { + const Document document = + ods(row(R"( )")); + EXPECT_THROW((void)document.recalculate(), UnsupportedOperation); +} diff --git a/test/src/table_position_test.cpp b/test/src/table_position_test.cpp index df7ab84f9..44b267bfc 100644 --- a/test/src/table_position_test.cpp +++ b/test/src/table_position_test.cpp @@ -2,6 +2,7 @@ #include +#include #include using namespace odr; @@ -25,6 +26,9 @@ TEST(TablePosition, a_column_letter_is_read_without_case) { } TEST(TablePosition, a_spelling_past_the_index_range_is_nothing) { + EXPECT_EQ( + TablePosition::to_row_string(std::numeric_limits::max()), + "4294967296"); EXPECT_FALSE(TablePosition::try_to_column_num("ABCDEFGHI").has_value()); EXPECT_FALSE(TablePosition::try_to_row_num("99999999999").has_value()); EXPECT_FALSE(TablePosition::try_to_row_num("0").has_value()); From 52fc542764d096cca1f015f5196cb01d4aecc128 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Mon, 5 Oct 2026 08:12:25 +0200 Subject: [PATCH 2/2] fix: keep a zero table count harmless and reject only overflow The ODF readers take repeat and span counts with as_uint(1), whose default applies only to a missing attribute. So a damaged number-columns-repeated="0" reached the cursor, which now threw, and the document no longer opened. Before, a zero count advanced nothing. The cursor again accepts zero and still rejects overflow. The recalculation also threw for a zero matrix span. Because every save recalculates, such a document no longer saved. A zero span covers no cell, so only overflow is rejected there too. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Gb1fLafqqzehpqPuU6uBfn --- src/odr/internal/common/sheet_recalculation.cpp | 3 +-- src/odr/internal/common/table_cursor.cpp | 4 ++-- test/src/internal/common/table_cursor_test.cpp | 6 ++++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/odr/internal/common/sheet_recalculation.cpp b/src/odr/internal/common/sheet_recalculation.cpp index cedc2da29..97af3f783 100644 --- a/src/odr/internal/common/sheet_recalculation.cpp +++ b/src/odr/internal/common/sheet_recalculation.cpp @@ -247,8 +247,7 @@ internal::recalculate(const abstract::Document &document) { const TableDimensions &span, const bool array, const std::string &text) { constexpr auto max = std::numeric_limits::max(); - if (span.rows == 0 || span.columns == 0 || span.rows > max - row || - span.columns > max - column) { + if (span.rows > max - row || span.columns > max - column) { throw UnsupportedOperation(); } const SheetPosition position(index, column, row); diff --git a/src/odr/internal/common/table_cursor.cpp b/src/odr/internal/common/table_cursor.cpp index ecf2df999..fc9757f5a 100644 --- a/src/odr/internal/common/table_cursor.cpp +++ b/src/odr/internal/common/table_cursor.cpp @@ -8,9 +8,9 @@ namespace odr::internal { namespace { +/// A count of zero, which a damaged file can state, advances nothing. std::uint32_t advance(const std::uint32_t position, const std::uint64_t count) { - if (count == 0 || - count > std::numeric_limits::max() - position) { + if (count > std::numeric_limits::max() - position) { throw std::out_of_range("table extent out of range"); } return position + static_cast(count); diff --git a/test/src/internal/common/table_cursor_test.cpp b/test/src/internal/common/table_cursor_test.cpp index ea36a9561..77b8340b2 100644 --- a/test/src/internal/common/table_cursor_test.cpp +++ b/test/src/internal/common/table_cursor_test.cpp @@ -66,9 +66,11 @@ TEST(TableCursor, rejects_invalid_extents_before_advancing) { cursor.add_column(1); EXPECT_THROW(cursor.add_column(max), std::out_of_range); EXPECT_THROW(cursor.add_cell(max, 1, max), std::out_of_range); - EXPECT_THROW(cursor.add_cell(0), std::out_of_range); - EXPECT_THROW(cursor.add_cell(1, 0), std::out_of_range); EXPECT_EQ(cursor.column(), 1); + // a zero count from a damaged file is no extent + cursor.add_cell(0); + EXPECT_EQ(cursor.column(), 1); + cursor.add_row(0); EXPECT_EQ(cursor.row(), 0); cursor.add_row(max); EXPECT_THROW(cursor.add_row(), std::out_of_range);