diff --git a/CHANGELOG.md b/CHANGELOG.md index f6cb68083..d0bab41f2 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 +- 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. diff --git a/src/odr/internal/oldms/spreadsheet/AGENTS.md b/src/odr/internal/oldms/spreadsheet/AGENTS.md index 75276f0d6..5f6bf2db2 100644 --- a/src/odr/internal/oldms/spreadsheet/AGENTS.md +++ b/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 diff --git a/src/odr/internal/oldms/spreadsheet/xls_io.cpp b/src/odr/internal/oldms/spreadsheet/xls_io.cpp index 2778eaeda..e9624dd55 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_io.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_io.cpp @@ -5,24 +5,26 @@ #include #include -#include #include -#include #include #include +#include + 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(m_remaining)); - m_remaining = 0; + skip_bytes(m_remaining); } const std::optional header = util::byte_stream::try_read(*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(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(); + 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 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) { diff --git a/src/odr/internal/oldms/spreadsheet/xls_io.hpp b/src/odr/internal/oldms/spreadsheet/xls_io.hpp index 1476c7cba..8b753133f 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_io.hpp +++ b/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}; diff --git a/src/odr/internal/oldms/spreadsheet/xls_parser.cpp b/src/odr/internal/oldms/spreadsheet/xls_parser.cpp index 61f97dff8..ea3bfabe4 100644 --- a/src/odr/internal/oldms/spreadsheet/xls_parser.cpp +++ b/src/odr/internal/oldms/spreadsheet/xls_parser.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -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 ®istry, 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 ®istry, const ElementIdentifier sheet_id, void parse_globals(BiffReader &reader, std::vector &sheets, std::vector &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 &sheets, if (head.cstUnique < 0) { throw std::runtime_error("xls: negative SST string count"); } - shared_strings.reserve(static_cast(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 ®istry, const ElementIdentifier sheet_id, const BoundSheet &info, const std::vector &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 ®istry, switch (reader.record_type()) { case biff_dimensions: { const auto dimensions = reader.read(); - sheet.dimensions = TableDimensions(dimensions.rwMac, dimensions.colMac); + sheet.dimensions = TableDimensions( + std::min(dimensions.rwMac, max_rows), + std::min(dimensions.colMac, max_columns)); } break; case biff_labelsst: { const auto label = reader.read(); @@ -171,6 +180,8 @@ void parse_sheet(BiffReader &reader, ElementRegistry ®istry, : (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(); const TablePosition position(formula.cell.col, formula.cell.rw); if (formula.val.is_xnum()) { @@ -222,7 +233,11 @@ ElementIdentifier spreadsheet::parse_tree(ElementRegistry ®istry, 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 bound_sheets; diff --git a/test/src/internal/oldms/xls_test.cpp b/test/src/internal/oldms/xls_test.cpp index 935b891ae..40d9a5624 100644 --- a/test/src/internal/oldms/xls_test.cpp +++ b/test/src/internal/oldms/xls_test.cpp @@ -18,7 +18,9 @@ #include #include +#include #include +#include #include #include #include @@ -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 &cells) { + const std::vector &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(open_workbook(valid))); + EXPECT_NO_THROW( + static_cast(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::max()); + append_u32(sst, std::numeric_limits::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( + 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(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"); +}