fix: keep table spans compact and bound their extents

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.

src/odr/internal/common/sheet_recalculation.cpp

@@ -11,6 +11,7 @@
 #include <odr/internal/formula/formula_parser.hpp>
 
 #include <algorithm>
+#include <limits>
 #include <optional>
 #include <unordered_map>
 #include <unordered_set>
@@ -245,19 +246,24 @@ 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<std::uint32_t>::max();
+          if (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<std::size_t>(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) {

src/odr/internal/common/table_cursor.cpp

@@ -1,49 +1,51 @@
 #include <odr/internal/common/table_cursor.hpp>
 
 #include <algorithm>
-#include <list>
+#include <limits>
+#include <stdexcept>
 
 namespace odr::internal {
 
-TableCursor::TableCursor() { m_sparse.emplace_back(); }
+namespace {
 
-void TableCursor::add_column(const std::uint32_t repeat) noexcept {
-  m_column += repeat;
+/// 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 > std::numeric_limits<std::uint32_t>::max() - position) {
+    throw std::out_of_range("table extent out of range");
+  }
+  return position + static_cast<std::uint32_t>(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<std::size_t>(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

src/odr/internal/common/table_cursor.hpp

@@ -2,16 +2,15 @@
 
 #include <odr/table_position.hpp>
 
+#include <cstddef>
 #include <cstdint>
-#include <list>
+#include <vector>
 
 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<std::list<Range>> m_sparse;
+  std::vector<Range> m_spans;
+  std::size_t m_next_span{0};
 
   void handle_rowspan_() noexcept;
 };

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

src/odr/internal/odf/odf_parser.cpp

@@ -226,8 +226,6 @@ void index_sheet_rows(ElementRegistry &registry,
       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);
   });
 

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) {

test/src/internal/common/table_cursor_test.cpp

@@ -2,6 +2,9 @@
 
 #include <gtest/gtest.h>
 
+#include <limits>
+#include <stdexcept>
+
 using namespace odr::internal;
 
 TEST(TableCursor, test) {
@@ -45,3 +48,31 @@ 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<std::uint32_t>::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<std::uint32_t>::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_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);
+  EXPECT_EQ(cursor.row(), max);
+}

test/src/sheet_recalculation_test.cpp

@@ -1,5 +1,6 @@
 #include <odr/document.hpp>
 #include <odr/document_element.hpp>
+#include <odr/exceptions.hpp>
 #include <odr/file.hpp>
 #include <odr/logger.hpp>
 #include <odr/sheet_position.hpp>
@@ -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"(<table:table-cell table:formula="of:=1" )"
+              R"(table:number-matrix-columns-spanned="65536" )"
+              R"(table:number-matrix-rows-spanned="65536"/> )"));
+  EXPECT_THROW((void)document.recalculate(), UnsupportedOperation);
+}

test/src/table_position_test.cpp

@@ -2,6 +2,7 @@
 
 #include <gtest/gtest.h>
 
+#include <limits>
 #include <string>
 
 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<std::uint32_t>::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());