fix: validate BIFF record and worksheet boundaries

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Legacy Excel rejects truncated BIFF records and a wrong substream type. It
+  drops cells outside the BIFF8 grid, and numbers ignore the host locale.
+
 - Legacy Word bounds font and piece tables by their declared lengths, checks
   piece coverage and offsets, and reads PLC entries without unaligned access.
 

src/odr/internal/oldms/spreadsheet/AGENTS.md

@@ -47,10 +47,16 @@ each hop, because the continuation re-declares compressed versus UTF-16 for
 the remainder (§2.5.293). Formatting runs (`cRun`·4 B) and phonetic data
 (`cbExtRst` B) are read and skipped.
 
-**Fail early.** Throw on: a missing or non-BIFF8 `BOF` (`vers != 0x0600`); a
-non-`CONTINUE` record where a continuation is required; an out-of-range SST
-index; a malformed `MulRk` body; an unknown `FormulaValue` type; a truncated
-stream. Skip records that are not modelled.
+**Fail early.** Throw on: a missing or non-BIFF8 `BOF` (`vers != 0x0600`), or
+one with the wrong substream type; a non-`CONTINUE` record where a continuation
+is required; an out-of-range SST index; a malformed `MulRk` body; an unknown
+`FormulaValue` type; a truncated record header or body. SST storage grows only
+as strings are read. Skip records that are not modelled.
+
+**Pass through**, as LibreOffice does: a cell outside the BIFF8 grid is
+dropped; `Dimensions` is clamped to the grid; `MulRk` takes its cell count from
+the body size and ignores `colLast`; a string formula without its `String`
+record stays empty; a clean end of stream ends a substream without `EOF`.
 
 **Cell formatting is resolved at parse time, per XF, in the `StyleRegistry`.**
 The parser fills both registries, because BIFF keeps styles and content in
@@ -82,9 +88,9 @@ returns the fill.
 - RK numbers (§2.5.217): the low 2 bits are flags, bit 0 `fX100` (divide by
   100), bit 1 `fInt` (a 30-bit signed int, else the high 30 bits of an IEEE
   double).
-- Numbers use `%.15g`, close to Excel's "General". Booleans are `TRUE` and
-  `FALSE`. Errors (§2.5.10) are `#DIV/0!`, `#VALUE!`, `#REF!`, `#NAME?`,
-  `#NUM!`, `#N/A` and `#NULL!`.
+- Numbers use locale-independent `fmt` with 15 significant digits, close to
+  Excel's "General". Booleans are `TRUE` and `FALSE`. Errors (§2.5.10) are
+  `#DIV/0!`, `#VALUE!`, `#REF!`, `#NAME?`, `#NUM!`, `#N/A` and `#NULL!`.
 - A date cell shows its raw serial number (open work §1).
 
 ## Tests

src/odr/internal/oldms/spreadsheet/xls_io.cpp

@@ -5,24 +5,26 @@
 #include <odr/internal/util/string_util.hpp>
 
 #include <algorithm>
-#include <array>
 #include <cmath>
-#include <cstdio>
 #include <istream>
 #include <stdexcept>
 
+#include <fmt/format.h>
+
 namespace odr::internal::oldms::spreadsheet {
 
 BiffReader::BiffReader(std::istream &in) : m_in{&in} {}
 
 bool BiffReader::next_record() {
   if (m_remaining > 0) {
-    m_in->ignore(static_cast<std::streamsize>(m_remaining));
-    m_remaining = 0;
+    skip_bytes(m_remaining);
   }
 
   const std::optional header = util::byte_stream::try_read<RecordHeader>(*m_in);
   if (!header) {
+    if (m_in->bad() || !m_in->eof() || m_in->gcount() != 0) {
+      throw std::runtime_error("xls: truncated or unreadable record header");
+    }
     return false;
   }
 
@@ -88,7 +90,7 @@ void BiffReader::skip_bytes(const std::size_t count) {
       next_continue();
     }
     const std::size_t take = std::min(left, m_remaining);
-    m_in->ignore(static_cast<std::streamsize>(take));
+    util::byte_stream::skip(*m_in, take);
     left -= take;
     m_remaining -= take;
   }
@@ -156,11 +158,14 @@ std::string BiffReader::read_xl_unicode_rich_extended_string() {
   return result;
 }
 
-void BiffReader::expect_bof() {
+void BiffReader::expect_bof(const std::uint16_t substream_type) {
   if (!next_record() || record_type() != biff_bof) {
     throw std::runtime_error("xls: expected BOF record");
   }
   const auto bof = read<BofFixed>();
+  if (bof.dt != substream_type) {
+    throw std::runtime_error("xls: unexpected BIFF substream type");
+  }
   if (bof.vers != bof_vers_biff8) {
     throw std::runtime_error("xls: unsupported BIFF version " +
                              std::to_string(bof.vers));
@@ -176,9 +181,7 @@ std::string spreadsheet::format_number(const double value) {
     return "NaN";
   }
 
-  std::array<char, 32> buffer{};
-  std::snprintf(buffer.data(), buffer.size(), "%.15g", value);
-  return buffer.data();
+  return fmt::format("{:.15g}", value);
 }
 
 std::string spreadsheet::error_code_string(const std::uint8_t error) {

src/odr/internal/oldms/spreadsheet/xls_io.hpp

@@ -61,7 +61,7 @@ class BiffReader final {
   /// formatting runs and phonetic data. Used for the SST string array.
   std::string read_xl_unicode_rich_extended_string();
 
-  void expect_bof();
+  void expect_bof(std::uint16_t substream_type);
 
 private:
   std::istream *m_in{nullptr};

src/odr/internal/oldms/spreadsheet/xls_parser.cpp

@@ -8,6 +8,7 @@
 #include <odr/internal/oldms/spreadsheet/xls_structs.hpp>
 #include <odr/internal/oldms/spreadsheet/xls_style.hpp>
 
+#include <algorithm>
 #include <optional>
 #include <stdexcept>
 #include <string>
@@ -17,6 +18,9 @@
 namespace odr::internal::oldms::spreadsheet {
 namespace {
 
+constexpr std::uint32_t max_columns = 256;
+constexpr std::uint32_t max_rows = 65536;
+
 struct BoundSheet {
   std::uint32_t offset{0};
   std::string name;
@@ -34,6 +38,10 @@ struct GlobalStyles {
 void add_cell(ElementRegistry &registry, const ElementIdentifier sheet_id,
               const std::uint32_t column, const std::uint32_t row,
               const std::uint16_t ixfe, std::string text) {
+  // LibreOffice also drops a cell outside the BIFF8 grid.
+  if (column >= max_columns || row >= max_rows) {
+    return;
+  }
   auto [cell_id, cell_element, cell] =
       registry.create_sheet_cell_element(TablePosition(column, row));
   cell.ixfe = ixfe;
@@ -53,7 +61,7 @@ void add_cell(ElementRegistry &registry, const ElementIdentifier sheet_id,
 void parse_globals(BiffReader &reader, std::vector<BoundSheet> &sheets,
                    std::vector<std::string> &shared_strings,
                    GlobalStyles &styles) {
-  reader.expect_bof();
+  reader.expect_bof(0x0005);
 
   while (reader.next_record() && reader.record_type() != biff_eof) {
     switch (reader.record_type()) {
@@ -88,7 +96,6 @@ void parse_globals(BiffReader &reader, std::vector<BoundSheet> &sheets,
       if (head.cstUnique < 0) {
         throw std::runtime_error("xls: negative SST string count");
       }
-      shared_strings.reserve(static_cast<std::size_t>(head.cstUnique));
       for (std::int32_t i = 0; i < head.cstUnique; ++i) {
         shared_strings.push_back(reader.read_xl_unicode_rich_extended_string());
       }
@@ -104,7 +111,7 @@ void parse_sheet(BiffReader &reader, ElementRegistry &registry,
                  const ElementIdentifier sheet_id, const BoundSheet &info,
                  const std::vector<std::string> &shared_strings) {
   reader.seek(info.offset);
-  reader.expect_bof();
+  reader.expect_bof(0x0010);
 
   ElementRegistry::Sheet &sheet = registry.sheet_element_at(sheet_id);
   sheet.name = info.name;
@@ -121,7 +128,9 @@ void parse_sheet(BiffReader &reader, ElementRegistry &registry,
     switch (reader.record_type()) {
     case biff_dimensions: {
       const auto dimensions = reader.read<DimensionsBody>();
-      sheet.dimensions = TableDimensions(dimensions.rwMac, dimensions.colMac);
+      sheet.dimensions = TableDimensions(
+          std::min<std::uint32_t>(dimensions.rwMac, max_rows),
+          std::min<std::uint32_t>(dimensions.colMac, max_columns));
     } break;
     case biff_labelsst: {
       const auto label = reader.read<LabelSstBody>();
@@ -171,6 +180,8 @@ void parse_sheet(BiffReader &reader, ElementRegistry &registry,
                    : (boolerr.bBoolErr != 0 ? "TRUE" : "FALSE"));
     } break;
     case biff_formula: {
+      // Without its String record, a formula string result stays empty.
+      pending_string_cell.reset();
       const auto formula = reader.read<FormulaFixed>();
       const TablePosition position(formula.cell.col, formula.cell.rw);
       if (formula.val.is_xnum()) {
@@ -222,7 +233,11 @@ ElementIdentifier
 spreadsheet::parse_tree(ElementRegistry &registry,
                         StyleRegistry &style_registry,
                         const abstract::ReadableFilesystem &files) {
-  const auto workbook_stream = files.open(AbsPath("/Workbook"))->stream();
+  const auto workbook_file = files.open(AbsPath("/Workbook"));
+  if (workbook_file == nullptr) {
+    throw std::runtime_error("xls: missing Workbook stream");
+  }
+  const auto workbook_stream = workbook_file->stream();
   BiffReader reader(*workbook_stream);
 
   std::vector<BoundSheet> bound_sheets;

test/src/internal/oldms/xls_test.cpp

@@ -18,7 +18,9 @@
 #include <internal/oldms/oldms_test_util.hpp>
 
 #include <bit>
+#include <clocale>
 #include <cstdint>
+#include <limits>
 #include <memory>
 #include <sstream>
 #include <string>
@@ -177,7 +179,8 @@ std::string make_bof(const std::uint16_t dt) {
 /// One-sheet workbook stream: `globals` records are wrapped with BOF/
 /// BoundSheet8/EOF, the sheet substream holds the given `cells`.
 std::string make_workbook(const std::string &globals,
-                          const std::vector<std::string> &cells) {
+                          const std::vector<std::string> &cells,
+                          const std::uint16_t cell_type = 0x0204) {
   const auto build_globals = [&](const std::uint32_t sheet_offset) {
     std::string result;
     append_record(result, 0x0809 /* BOF */, make_bof(0x0005));
@@ -197,7 +200,7 @@ std::string make_workbook(const std::string &globals,
   std::string sheet;
   append_record(sheet, 0x0809 /* BOF */, make_bof(0x0010));
   for (const std::string &cell : cells) {
-    append_record(sheet, 0x0204 /* Label */, cell);
+    append_record(sheet, cell_type, cell);
   }
   append_record(sheet, 0x000A /* EOF */, "");
 
@@ -385,3 +388,88 @@ TEST(OldMs, xls_file_example_5000) {
   EXPECT_EQ(collect_text(sheet.cell(2, 5000)), "Alkire");
   EXPECT_EQ(collect_text(sheet.cell(7, 5000)), "6125");
 }
+
+TEST(OldMs, xls_truncated_records_are_not_end_of_stream) {
+  using internal::oldms::spreadsheet::BiffReader;
+  for (const std::size_t bytes : {1u, 2u, 3u}) {
+    std::istringstream in(std::string(bytes, '\0'));
+    BiffReader reader(in);
+    EXPECT_THROW(reader.next_record(), std::runtime_error);
+  }
+  std::string record;
+  append_record(record, 0x7777, "payload");
+  record.pop_back();
+  std::istringstream in(record);
+  BiffReader reader(in);
+  ASSERT_TRUE(reader.next_record());
+  EXPECT_THROW(reader.next_record(), std::runtime_error);
+
+  std::istringstream skipped(record);
+  BiffReader skipping(skipped);
+  ASSERT_TRUE(skipping.next_record());
+  EXPECT_THROW(skipping.skip_bytes(7), std::runtime_error);
+  std::istringstream empty;
+  BiffReader exhausted(empty);
+  EXPECT_FALSE(exhausted.next_record());
+}
+
+TEST(OldMs, xls_substreams_and_counts_are_validated) {
+  const std::string valid = make_workbook({}, {});
+  EXPECT_NO_THROW(static_cast<void>(open_workbook(valid)));
+  EXPECT_NO_THROW(
+      static_cast<void>(open_workbook(valid.substr(0, valid.size() - 4))));
+  EXPECT_THROW(open_workbook(valid.substr(0, valid.size() - 5)),
+               std::runtime_error);
+  std::string wrong_bof = valid;
+  wrong_bof[6] = '\x10'; // Worksheet BOF where workbook globals are required.
+  EXPECT_THROW(open_workbook(wrong_bof), std::runtime_error);
+
+  std::string sst;
+  append_u32(sst, std::numeric_limits<std::int32_t>::max());
+  append_u32(sst, std::numeric_limits<std::int32_t>::max());
+  std::string globals;
+  append_record(globals, 0x00FC, sst);
+  EXPECT_THROW(open_workbook(make_workbook(globals, {})), std::runtime_error);
+}
+
+TEST(OldMs, xls_tolerates_cells_outside_the_grid_and_missing_results) {
+  const Document outside =
+      open_workbook(make_workbook({}, {make_label(0, 256, 0, "x")}));
+  EXPECT_EQ(collect_text(
+                outside.root_element().first_child().as_sheet().cell(256, 0)),
+            "");
+
+  std::string formula(12, '\0'); // CellRef and string FormulaValue prefix.
+  append_u16(formula, 0xFFFF);
+  formula.resize(20, '\0');
+  EXPECT_NO_THROW(static_cast<void>(
+      open_workbook(make_workbook({}, {formula, formula}, 0x0006))));
+}
+
+TEST(OldMs, xls_mulrk_takes_its_cells_from_the_body_size) {
+  std::string cells;
+  append_u16(cells, 0); // row
+  append_u16(cells, 2); // first column
+  append_u16(cells, 0); // XF
+  append_u32(cells, (12u << 2) | 2u);
+  append_u16(cells, 2); // final column
+  const Document document = open_workbook(make_workbook({}, {cells}, 0x00BD));
+  EXPECT_EQ(
+      collect_text(document.root_element().first_child().as_sheet().cell(2, 0)),
+      "12");
+  cells[cells.size() - 2] = 3;
+  EXPECT_NO_THROW(
+      static_cast<void>(open_workbook(make_workbook({}, {cells}, 0x00BD))));
+}
+
+TEST(OldMs, xls_number_format_ignores_numeric_locale) {
+  struct LocaleGuard final {
+    std::string previous{std::setlocale(LC_NUMERIC, nullptr)};
+    ~LocaleGuard() { std::setlocale(LC_NUMERIC, previous.c_str()); }
+  } guard;
+  if (std::setlocale(LC_NUMERIC, "de_DE.UTF-8") == nullptr &&
+      std::setlocale(LC_NUMERIC, "de_DE.utf8") == nullptr) {
+    GTEST_SKIP() << "German locale unavailable";
+  }
+  EXPECT_EQ(internal::oldms::spreadsheet::format_number(123.45), "123.45");
+}