From b12a6a93b75239c5bf850a1374676f6139b316ad Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 4 Oct 2026 22:49:55 +0200 Subject: [PATCH] fix: preserve CSV records and bound cell coordinates --- CHANGELOG.md | 4 ++ src/odr/internal/csv/csv_document.cpp | 54 ++++++++++++++++--------- src/odr/internal/csv/csv_file.cpp | 9 ++++- src/odr/internal/csv/csv_util.cpp | 16 ++++++-- src/odr/internal/csv/csv_util.hpp | 4 ++ test/src/internal/csv/csv_file_test.cpp | 42 ++++++++++++++++++- 6 files changed, 103 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 84c66de9a..668bf0e77 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 +- 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. + - Quantity parsing and integer formatting are locale independent. Invalid or out-of-range integer magnitudes throw instead of silently converting. diff --git a/src/odr/internal/csv/csv_document.cpp b/src/odr/internal/csv/csv_document.cpp index d8db671bf..4f5820bdc 100644 --- a/src/odr/internal/csv/csv_document.cpp +++ b/src/odr/internal/csv/csv_document.cpp @@ -14,8 +14,10 @@ #include #include +#include #include #include +#include #include #include @@ -40,8 +42,18 @@ constexpr std::uint64_t column_mask = (std::uint64_t{1} << row_shift) - 1; constexpr std::uint64_t row_mask = (std::uint64_t{1} << (kind_shift - row_shift)) - 1; +constexpr std::uint32_t max_columns = std::uint32_t{1} << row_shift; +constexpr std::uint32_t max_rows = std::numeric_limits::max(); + +void check_position(const std::uint32_t column, const std::uint32_t row) { + if (column >= max_columns || row >= max_rows) { + throw std::out_of_range("csv position out of range"); + } +} + ElementIdentifier make_id(const Kind kind, const std::uint32_t column = 0, const std::uint32_t row = 0) { + check_position(column, row); return static_cast(kind) << kind_shift | (static_cast(row) & row_mask) << row_shift | (static_cast(column) & column_mask); @@ -69,11 +81,16 @@ class ElementAdapter final : public AdapterBase { [[nodiscard]] ElementType element_type(const ElementIdentifier element_id) const override { + const auto coordinates = + element_id & ((std::uint64_t{1} << kind_shift) - 1); + if ((coordinates >> row_shift) >= max_rows) { + return ElementType::none; + } switch (kind_of(element_id)) { case Kind::root: - return ElementType::root; + return coordinates == 0 ? ElementType::root : ElementType::none; case Kind::sheet: - return ElementType::sheet; + return coordinates == 0 ? ElementType::sheet : ElementType::none; case Kind::cell: return ElementType::sheet_cell; case Kind::text: @@ -304,32 +321,23 @@ CsvDocument::CsvDocument(const abstract::File &file, encoding != TextEncoding::utf8 || bytes.starts_with("\xef\xbb\xbf"); std::string text = encoding::to_utf8(bytes, encoding); - const std::size_t first_break = text.find('\n'); - m_line_end = first_break != std::string::npos && - (first_break == 0 || text[first_break - 1] != '\r') - ? "\n" - : "\r\n"; m_final_line_end = text.empty() || text.back() == '\n' || text.back() == '\r'; - std::string_view remainder = text; + RecordReader reader(text, dialect); + std::vector fields; if (skip_first_line) { - if (const std::size_t body = remainder.find_first_not_of( - "\r\n", remainder.find_first_of("\r\n")); - body != std::string_view::npos) { - remainder = remainder.substr(body); - } else { - remainder = {}; - } + reader.read(fields); } - - RecordReader reader(remainder, dialect); - std::vector fields; std::uint32_t columns = 0; while (reader.read(fields)) { + if (fields.size() > max_columns || m_rows.size() >= max_rows) { + throw std::length_error("csv dimensions out of range"); + } columns = std::max(columns, static_cast(fields.size())); m_rows.push_back(fields); } + m_line_end = reader.line_end().empty() ? "\r\n" : reader.line_end(); m_dimensions = {static_cast(m_rows.size()), columns}; // Walks the fields that exist rather than the rectangle they span: one wide @@ -390,6 +398,7 @@ ValueType CsvDocument::value_type(const std::uint32_t column, void CsvDocument::set_cell(const std::uint32_t column, const std::uint32_t row, std::string text) { + check_position(column, row); if (row >= m_rows.size()) { m_rows.resize(row + 1); } @@ -424,6 +433,9 @@ void CsvDocument::insert_rows(const std::uint32_t row, if (row >= m_rows.size()) { return; } + if (count > max_rows - m_dimensions.rows) { + throw std::length_error("csv row count out of range"); + } // a field per column, as the lines around it state m_rows.insert(m_rows.begin() + row, count, std::vector(m_dimensions.columns)); @@ -438,8 +450,7 @@ void CsvDocument::delete_rows(const std::uint32_t row, } m_rows.erase(m_rows.begin() + row, m_rows.begin() + - std::min(static_cast(row) + count, - m_rows.size())); + (row + std::min(count, m_rows.size() - row))); m_dimensions.rows = static_cast(m_rows.size()); type_columns(); } @@ -449,6 +460,9 @@ void CsvDocument::insert_columns(const std::uint32_t column, if (column >= m_dimensions.columns) { return; } + if (count > max_columns - m_dimensions.columns) { + throw std::length_error("csv column count out of range"); + } for (std::vector &fields : m_rows) { if (column < fields.size()) { fields.insert(fields.begin() + column, count, std::string()); diff --git a/src/odr/internal/csv/csv_file.cpp b/src/odr/internal/csv/csv_file.cpp index 5a9f8a897..1f57826ec 100644 --- a/src/odr/internal/csv/csv_file.cpp +++ b/src/odr/internal/csv/csv_file.cpp @@ -18,8 +18,9 @@ void check(const Dialect dialect) { if (dialect.separator == dialect.quote) { throw std::invalid_argument("csv separator equals its quote"); } - if (dialect.separator == '\n' || dialect.separator == '\r') { - throw std::invalid_argument("csv separator is a line break"); + if (dialect.separator == '\n' || dialect.separator == '\r' || + dialect.quote == '\n' || dialect.quote == '\r') { + throw std::invalid_argument("csv delimiter is a line break"); } } @@ -33,6 +34,7 @@ CsvFile::CsvFile(std::shared_ptr file) } m_dialect = probe.dialect; m_separator_directive = probe.separator_directive; + check(m_dialect); } CsvFile::CsvFile(std::shared_ptr file, @@ -47,6 +49,9 @@ CsvFile::CsvFile(std::shared_ptr file, if (options.separator.has_value()) { m_dialect.separator = *options.separator; check(m_dialect); + const Probe probe = + csv::probe(*m_file->file(), m_file->encoding(), m_dialect.quote); + m_separator_directive = probe.separator_directive; return; } diff --git a/src/odr/internal/csv/csv_util.cpp b/src/odr/internal/csv/csv_util.cpp index bfef6dc23..22952b5b2 100644 --- a/src/odr/internal/csv/csv_util.cpp +++ b/src/odr/internal/csv/csv_util.cpp @@ -226,9 +226,15 @@ bool csv::RecordReader::read(std::vector &fields) { } else if (c == m_dialect.separator) { end_field(); started = true; - } else if (c == '\r') { - // CRLF, and a lone CR - } else if (c == '\n') { + } else if (c == '\r' || c == '\n') { + const std::size_t start = m_position; + if (c == '\r' && m_position + 1 < m_text.size() && + m_text[m_position + 1] == '\n') { + ++m_position; + } + if (m_line_end.empty()) { + m_line_end = m_text.substr(start, m_position - start + 1); + } if (!started) { continue; // empty line } @@ -255,6 +261,10 @@ bool csv::RecordReader::read(std::vector &fields) { bool csv::RecordReader::unterminated() const noexcept { return m_unterminated; } +std::string_view csv::RecordReader::line_end() const noexcept { + return m_line_end; +} + csv::Probe csv::probe(const std::string_view text, const bool complete, const char quote) { Probe result; diff --git a/src/odr/internal/csv/csv_util.hpp b/src/odr/internal/csv/csv_util.hpp index 647fa0b82..49bb6aa73 100644 --- a/src/odr/internal/csv/csv_util.hpp +++ b/src/odr/internal/csv/csv_util.hpp @@ -35,11 +35,15 @@ class RecordReader final { /// Whether the text ran out inside a quoted field. [[nodiscard]] bool unterminated() const noexcept; + /// The first record delimiter, excluding line breaks inside fields. + [[nodiscard]] std::string_view line_end() const noexcept; + private: std::string_view m_text; Dialect m_dialect; std::size_t m_position{0}; bool m_unterminated{false}; + std::string_view m_line_end; }; /// What a probe made of a file's opening bytes. diff --git a/test/src/internal/csv/csv_file_test.cpp b/test/src/internal/csv/csv_file_test.cpp index 68105a8e9..f27824f1a 100644 --- a/test/src/internal/csv/csv_file_test.cpp +++ b/test/src/internal/csv/csv_file_test.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -72,7 +73,8 @@ TEST(CsvFile, csv) { TEST(CsvProbe, a_consistent_field_count_is_what_makes_it_a_csv) { EXPECT_TRUE(probe("a,b,c\n1,2,3\n").is_csv); - EXPECT_TRUE(probe("a,b,c\n1,2,3").is_csv); // no trailing newline + EXPECT_TRUE(probe("a,b,c\n1,2,3").is_csv); // no trailing newline + EXPECT_TRUE(probe("a,b\r1,2\r").is_csv); EXPECT_TRUE(probe("a,b\r\n1,2\r\n").is_csv); // crlf EXPECT_TRUE(probe("a,b\n\"x,y\",2\n").is_csv); // a quoted separator EXPECT_TRUE(probe("a,b\n\"x\ny\",2\n").is_csv); // a quoted newline @@ -183,6 +185,10 @@ TEST(CsvProbe, excel_declares_its_separator) { } TEST(RecordReader, parses_rfc4180) { + EXPECT_EQ(records("a,b\r1,2\r", {}), + (std::vector>{{"a", "b"}, {"1", "2"}})); + EXPECT_EQ(records("\r\n\r\"a\rb\",c\r\n", {}), + (std::vector>{{"a\rb", "c"}})); EXPECT_EQ(records("a,b\n1,2\n", {}), (std::vector>{{"a", "b"}, {"1", "2"}})); EXPECT_EQ(records("\"x,y\",2\n", {}), @@ -258,6 +264,10 @@ TEST(CsvOptions, a_declared_separator_makes_anything_readable) { } TEST(CsvOptions, an_incoherent_dialect_is_a_caller_mistake) { + EXPECT_THROW( + (void)open(File::from_memory("a,b\n"), + DecodeOptions::as_csv({.separator = ',', .quote = '\r'})), + std::invalid_argument); EXPECT_THROW( (void)open(File::from_memory("a,b\n"), DecodeOptions::as_csv({.separator = '"', .quote = '"'})) @@ -522,6 +532,8 @@ std::string edited( } // namespace TEST(CsvDocument, an_unedited_file_saves_as_it_reads) { + EXPECT_EQ(edited("\"a\nb\",c\r\n1,2\r\n", ""), "\"a\nb\",c\r\n1,2\r\n"); + EXPECT_EQ(edited("a,b\r1,2\r", ""), "a,b\r1,2\r"); EXPECT_EQ(edited("a,b\n1,2\n", ""), "a,b\n1,2\n"); EXPECT_EQ(edited("a,b\r\n1,2", ""), "a,b\r\n1,2"); EXPECT_EQ(edited("a\nb,c,d\n", ""), "a\nb,c,d\n"); @@ -577,6 +589,9 @@ TEST(CsvDocument, a_written_number_types_its_column_again) { } TEST(CsvDocument, the_separator_directive_is_kept) { + EXPECT_EQ(edited("sep=;\ra;b\r1;2\r", "", + DecodeOptions::as_csv({.separator = ';'})), + "sep=;\ra;b\r1;2\r"); EXPECT_EQ(edited("sep=;\na;b\n1;2\n", "", DecodeOptions::as(FileType::comma_separated_values)), "sep=;\na;b\n1;2\n"); @@ -681,3 +696,28 @@ TEST(CsvDocument, a_column_edit_keeps_the_numbers_of_a_column) { EXPECT_EQ(sheet.cell(0, 1).value_type(), ValueType::float_number); EXPECT_EQ(sheet.dimensions().columns, 1); } + +TEST(CsvDocument, coordinates_and_growth_cannot_wrap) { + const Document document = open(File::from_memory("a,b\n1,2\n"), + DecodeOptions::as_csv({.separator = ','})) + .as_csv_file() + .document(); + const Sheet sheet = document.root_element().first_child().as_sheet(); + constexpr auto max = std::numeric_limits::max(); + constexpr std::uint32_t columns = std::uint32_t{1} << 24; + EXPECT_THROW((void)sheet.cell(columns, 0), std::out_of_range); + EXPECT_THROW(sheet.set_cell(columns, 0, CellValue("x")), std::out_of_range); + EXPECT_THROW(sheet.set_cell(0, max, CellValue("x")), std::out_of_range); + EXPECT_THROW(sheet.insert_rows(0, max), std::length_error); + EXPECT_THROW(sheet.insert_columns(0, columns), std::length_error); + EXPECT_EQ(sheet.dimensions().rows, 2); + EXPECT_EQ(sheet.dimensions().columns, 2); + EXPECT_EQ(sheet.cell(0, 0).value().text(), "a"); + EXPECT_EQ(document + .element_by_id(sheet.cell(0, 0).identifier() | + (std::uint64_t{1} << 56)) + .type(), + ElementType::none); + sheet.delete_rows(1, max); + EXPECT_EQ(sheet.dimensions().rows, 1); +}