From 93b8bc0b3fdffec8af4a2413458571b309add189 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Fri, 7 Aug 2026 19:50:14 -0400 Subject: [PATCH 01/12] Return errors for invalid style indexes --- src/spreadsheet/mod.rs | 23 ++++++++ src/spreadsheet/xls.rs | 19 +++++- src/spreadsheet/xlsb.rs | 12 +++- src/spreadsheet/xlsx.rs | 124 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 5 deletions(-) diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index 6ab6f4d..ae71930 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -27,6 +27,25 @@ pub(crate) mod xlsx; pub(crate) mod criteria; pub(crate) mod sheet; +pub(crate) fn resolve_number_format( + number_formats: &[CellType], + file_name: &str, + sheet_name: &str, + row: usize, + col: usize, + style_index: usize, +) -> Result { + number_formats.get(style_index).copied().ok_or_else(|| { + SpreadsheetError::CellStyleIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + crate::spreadsheet::reference::index_to_reference(row, col), + style_index, + number_formats.len(), + ) + }) +} + #[derive(Error, Debug)] pub(crate) enum SpreadsheetError { /// Error indicating the spreadsheet format is not supported @@ -48,6 +67,10 @@ pub(crate) enum SpreadsheetError { /// Error indicating a specific cell value is invalid #[error("Cell '[{0}]{1}!{2}': {3}")] CellValueError(String, String, String, String), + + /// Error indicating a cell style index is outside the workbook's style table. + #[error("Cell '[{0}]{1}!{2}': invalid style index {3}; workbook defines {4} styles")] + CellStyleIndexError(String, String, String, usize, usize), } pub(crate) trait Spreadsheet { diff --git a/src/spreadsheet/xls.rs b/src/spreadsheet/xls.rs index 0e68ef1..02119c4 100644 --- a/src/spreadsheet/xls.rs +++ b/src/spreadsheet/xls.rs @@ -10,6 +10,7 @@ use crate::spreadsheet::cell::CellType; use crate::spreadsheet::criteria::Criteria; use crate::spreadsheet::excel::load_number_formats; use crate::spreadsheet::reference::index_to_reference; +use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; use crate::spreadsheet::Spreadsheet; use crate::spreadsheet::SpreadsheetError; @@ -201,7 +202,14 @@ impl Spreadsheet for XlsSpreadsheet { } last_row = Some(row); let index = self.reader.read_u16()? as usize; - let kind = self.number_formats[index]; + let kind = resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?; let value = self.reader.read_rk_number()?; sheet.push(Cell { row, @@ -234,7 +242,14 @@ impl Spreadsheet for XlsSpreadsheet { }; let kind = match either { Either::Left(kind) => kind, - Either::Right(index) => self.number_formats[index], + Either::Right(index) => resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?, }; if kind != CellType::Error { if !criteria.nulls.contains(&value) { diff --git a/src/spreadsheet/xlsb.rs b/src/spreadsheet/xlsb.rs index bb1231e..12dafe7 100644 --- a/src/spreadsheet/xlsb.rs +++ b/src/spreadsheet/xlsb.rs @@ -10,6 +10,7 @@ use crate::spreadsheet::criteria::Criteria; use crate::spreadsheet::excel; use crate::spreadsheet::excel::load_relationships; use crate::spreadsheet::reference::index_to_reference; +use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; use crate::spreadsheet::Spreadsheet; use crate::spreadsheet::SpreadsheetError; @@ -221,7 +222,14 @@ impl Spreadsheet for XlsbSpreadsheet { }; let kind = match either { Either::Left(kind) => kind, - Either::Right(index) => (self.number_formats)[index], + Either::Right(index) => resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?, }; if kind != CellType::Error { if !criteria.nulls.contains(&value) { @@ -451,4 +459,4 @@ fn read_rk_cell(reader: &mut Biff12Reader>>) -> }; (Either::Right(index), value) - } \ No newline at end of file + } diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index 026954b..1cda225 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -15,6 +15,7 @@ use crate::spreadsheet::excel; use crate::spreadsheet::excel::load_relationships; use crate::spreadsheet::reference::index_to_reference; use crate::spreadsheet::reference::reference_to_index; +use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; use quick_xml::events::Event; use quick_xml::name::QName; @@ -231,7 +232,14 @@ impl Spreadsheet for XlsxSpreadsheet { if let Some(format_id) = event.get_attribute_value("s")? { if kind == CellType::Number && !format_id.is_empty() { let index = format_id.parse::()?; - kind = self.number_formats[index]; + kind = resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?; } } } else { @@ -519,3 +527,117 @@ fn read_string_value( }); Ok(text) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::spreadsheet::criteria::Criteria; + use std::collections::HashSet; + use std::fs::File; + use std::io::Write; + use std::path::Path; + use std::path::PathBuf; + use std::time::SystemTime; + use std::time::UNIX_EPOCH; + use zip::write::SimpleFileOptions; + use zip::ZipWriter; + + #[test] + fn invalid_style_index_returns_error() { + let path = invalid_style_workbook_path(); + write_invalid_style_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&Criteria { + sheet_name_patterns: None, + sheet_limit: None, + range: None, + rows_limit: None, + nulls: HashSet::from(["".to_string()]), + error_as_null: false, + skip_empty_rows: false, + end_at_empty_row: false, + spread_merged_cells: false, + }); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid style index error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A11")); + assert!(error.contains("invalid style index 999")); + assert!(error.contains("workbook defines 1 styles")); + } + + fn invalid_style_workbook_path() -> PathBuf { + let id = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + std::env::temp_dir().join(format!("rusty-sheet-invalid-style-{id}.xlsx")) + } + + fn write_invalid_style_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + let rows = (2..11) + .map(|row| format!("{row}")) + .collect::(); + let sheet = format!( + r#" + + value{rows}11 +"# + ); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + + +"#.to_string(), + ), + ( + "xl/styles.xml", + r#" + + +"#.to_string(), + ), + ("xl/worksheets/sheet1.xml", sheet), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } +} From f5b73ac95a403313dcd89ee004c61682730e1643 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 12:53:36 -0400 Subject: [PATCH 02/12] Return errors for invalid shared string indexes --- src/extension/writer.rs | 10 +-- src/spreadsheet/mod.rs | 145 ++++++++++++++++++++++++++++++---------- src/spreadsheet/xlsx.rs | 113 ++++++++++++++++++++++++++----- 3 files changed, 212 insertions(+), 56 deletions(-) diff --git a/src/extension/writer.rs b/src/extension/writer.rs index 6a5e890..2b5b038 100644 --- a/src/extension/writer.rs +++ b/src/extension/writer.rs @@ -3,15 +3,16 @@ use crate::database::column::Column; use crate::database::column::ColumnType; use crate::error::RustySheetError; +use crate::spreadsheet::SpreadsheetError; use crate::spreadsheet::cell::Cell; use crate::spreadsheet::cell::CellType; +use crate::spreadsheet::resolve_loaded_shared_string; +use crate::spreadsheet::sheet::Sheet; use duckdb::core::FlatVector; use duckdb::core::Inserter; use libduckdb_sys::duckdb_date; use libduckdb_sys::duckdb_time; use libduckdb_sys::duckdb_timestamp; -use crate::spreadsheet::sheet::Sheet; -use crate::spreadsheet::SpreadsheetError; /// Writes a cell value to a DuckDB vector based on column type. /// Handles type conversion and error mapping for different data types. @@ -25,8 +26,9 @@ pub(super) fn write_to_vector(sheet: &Sheet, column: &Column, cell: &Cell, vecto ) }; let cell = if cell.kind == CellType::SharedString { - let index = cell.value.parse::()?; - if let Some(shared_string) = &shared_strings[index] { + if let Some(shared_string) = + resolve_loaded_shared_string(shared_strings, &sheet.file_name, &sheet.name, cell)? + { &Cell { row: cell.row, col: cell.col, diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index ae71930..623fbe3 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -46,6 +46,58 @@ pub(crate) fn resolve_number_format( }) } +pub(crate) fn resolve_shared_string<'a>( + shared_strings: &'a [String], + mappings: &HashMap, + file_name: &str, + sheet_name: &str, + cell: &Cell, +) -> Result<&'a str, RustySheetError> { + let shared_string_index = cell.value.parse::()?; + let mapped_index = mappings.get(&shared_string_index).copied().ok_or_else(|| { + SpreadsheetError::CellSharedStringIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + shared_string_index, + ) + })?; + + shared_strings + .get(mapped_index) + .map(String::as_str) + .ok_or_else(|| { + SpreadsheetError::CellSharedStringIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + shared_string_index, + ) + .into() + }) +} + +pub(crate) fn resolve_loaded_shared_string<'a>( + shared_strings: &'a [Option], + file_name: &str, + sheet_name: &str, + cell: &Cell, +) -> Result, RustySheetError> { + let shared_string_index = cell.value.parse::()?; + shared_strings + .get(shared_string_index) + .map(Option::as_deref) + .ok_or_else(|| { + SpreadsheetError::CellSharedStringIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + shared_string_index, + ) + .into() + }) +} + #[derive(Error, Debug)] pub(crate) enum SpreadsheetError { /// Error indicating the spreadsheet format is not supported @@ -71,6 +123,10 @@ pub(crate) enum SpreadsheetError { /// Error indicating a cell style index is outside the workbook's style table. #[error("Cell '[{0}]{1}!{2}': invalid style index {3}; workbook defines {4} styles")] CellStyleIndexError(String, String, String, usize, usize), + + /// Error indicating a shared string index is outside the workbook's shared string table. + #[error("Cell '[{0}]{1}!{2}': invalid shared string index {3}")] + CellSharedStringIndexError(String, String, String, usize), } pub(crate) trait Spreadsheet { @@ -132,56 +188,71 @@ pub(crate) trait Spreadsheet { let (shared_strings, mappings) = self.load_shared_strings(Some(shared_indexes))?; let mut tables = Vec::::new(); - for (name, header, data, row_lower_bound, col_lower_bound, col_upper_bound) in sheets.into_iter() { - let names = (col_lower_bound..=col_upper_bound).map(|col| { - let index = col - col_lower_bound; - if let Some(cell) = &header[index] { - let value = if cell.kind == CellType::SharedString { - let id = cell.value.parse::().expect("Shared string index"); - let index = mappings[&id]; - shared_strings[index].to_owned() - } else { - cell.to_string() - }; - if !criteria.nulls.contains(&value) { - value + let file_name = self.name(); + for (sheet_name, header, data, row_lower_bound, col_lower_bound, col_upper_bound) in sheets.into_iter() { + let names = (col_lower_bound..=col_upper_bound) + .map(|col| -> Result { + let index = col - col_lower_bound; + if let Some(cell) = &header[index] { + let value = if cell.kind == CellType::SharedString { + resolve_shared_string( + &shared_strings, + &mappings, + &file_name, + &sheet_name, + cell, + )?.to_owned() + } else { + cell.to_string() + }; + Ok(if !criteria.nulls.contains(&value) { + value + } else { + index_to_col(col).to_owned() + }) } else { - index_to_col(col).to_owned() + Ok(index_to_col(col).to_owned()) } - } else { - index_to_col(col).to_owned() - } - }).collect::>(); + }) + .collect::, _>>()?; let columns = names.iter().zip(data) - .map(|(name, cells)| { + .map(|(column_name, cells)| -> Result { let types = cells.iter() - .map(|cell| { + .map(|cell| -> Result, RustySheetError> { if cell.kind == CellType::SharedString { - let id = cell.value.parse::().expect("Shared string index"); - let index = mappings[&id]; - let value = shared_strings[index].as_str(); - ColumnType::from(if criteria.nulls.contains(value) { - &CellType::Empty - } else { - &cell.kind - }, value) + let value = resolve_shared_string( + &shared_strings, + &mappings, + &file_name, + &sheet_name, + cell, + )?; + Ok(ColumnType::from( + if criteria.nulls.contains(value) { + &CellType::Empty + } else { + &cell.kind + }, + value, + )) } else { - ColumnType::from(&cell.kind, &cell.value) + Ok(ColumnType::from(&cell.kind, &cell.value)) } }) - .collect::>(); - Column { - name: name.to_owned(), - kind: presets.iter() - .find(|(pattern, _)| pattern.matches(name)) + .collect::, _>>()?; + Ok(Column { + name: column_name.to_owned(), + kind: presets + .iter() + .find(|(pattern, _)| pattern.matches(column_name)) .map(|(_, kind)| kind.to_owned()) .unwrap_or(ColumnType::detect(types)), - } + }) }) - .collect::>(); + .collect::, _>>()?; tables.push(Table { - name, + name: sheet_name, columns, row_lower_bound, col_lower_bound, diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index 1cda225..e983145 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -544,21 +544,11 @@ mod tests { #[test] fn invalid_style_index_returns_error() { - let path = invalid_style_workbook_path(); + let path = invalid_workbook_path("style"); write_invalid_style_workbook(&path); let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); - let result = spreadsheet.read_sheets(&Criteria { - sheet_name_patterns: None, - sheet_limit: None, - range: None, - rows_limit: None, - nulls: HashSet::from(["".to_string()]), - error_as_null: false, - skip_empty_rows: false, - end_at_empty_row: false, - spread_merged_cells: false, - }); + let result = spreadsheet.read_sheets(&default_criteria()); std::fs::remove_file(path).unwrap(); let error = match result { @@ -570,18 +560,50 @@ mod tests { assert!(error.contains("workbook defines 1 styles")); } - fn invalid_style_workbook_path() -> PathBuf { + #[test] + fn invalid_shared_string_index_returns_error() { + let path = invalid_workbook_path("shared-string"); + write_invalid_shared_string_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.analyze_sheets(true, &default_criteria(), &Vec::new()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid shared string index error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A1")); + assert!(error.contains("invalid shared string index 999")); + } + + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_nanos(); - std::env::temp_dir().join(format!("rusty-sheet-invalid-style-{id}.xlsx")) + std::env::temp_dir().join(format!("rusty-sheet-invalid-{kind}-{id}.xlsx")) + } + + fn default_criteria() -> Criteria { + Criteria { + sheet_name_patterns: None, + sheet_limit: None, + range: None, + rows_limit: None, + nulls: HashSet::from(["".to_string()]), + error_as_null: false, + skip_empty_rows: false, + end_at_empty_row: false, + spread_merged_cells: false, + } } fn write_invalid_style_workbook(path: &Path) { let file = File::create(path).unwrap(); let mut zip = ZipWriter::new(file); - let options = SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); let rows = (2..11) .map(|row| format!("{row}")) .collect::(); @@ -640,4 +662,65 @@ mod tests { } zip.finish().unwrap(); } + + fn write_invalid_shared_string_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + + +"#.to_string(), + ), + ( + "xl/sharedStrings.xml", + r#" + + name +"#.to_string(), + ), + ( + "xl/worksheets/sheet1.xml", + r#" + + 999 +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } } From 9e8925fc39a3b0d5fb254e11948dc3e442a7a1de Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 12:58:38 -0400 Subject: [PATCH 03/12] Return errors for invalid cell references --- src/spreadsheet/mod.rs | 27 +++++++++ src/spreadsheet/reference.rs | 103 ++++++++++++++++++++--------------- src/spreadsheet/xlsx.rs | 83 +++++++++++++++++++++++++++- 3 files changed, 166 insertions(+), 47 deletions(-) diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index 623fbe3..c9e7fa4 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -46,6 +46,29 @@ pub(crate) fn resolve_number_format( }) } +#[cfg(test)] +mod number_format_tests { + use super::*; + + #[test] + fn invalid_style_with_out_of_range_column_returns_error() { + let error = resolve_number_format( + &[CellType::Number], + "workbook.xlsb", + "Sheet1", + 10, + 16_384, + 999, + ) + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Cell '[workbook.xlsb]Sheet1!R11C16385': invalid style index 999; workbook defines 1 styles" + ); + } +} + pub(crate) fn resolve_shared_string<'a>( shared_strings: &'a [String], mappings: &HashMap, @@ -127,6 +150,10 @@ pub(crate) enum SpreadsheetError { /// Error indicating a shared string index is outside the workbook's shared string table. #[error("Cell '[{0}]{1}!{2}': invalid shared string index {3}")] CellSharedStringIndexError(String, String, String, usize), + + /// Error indicating a worksheet cell reference is outside the supported spreadsheet bounds. + #[error("Sheet '[{0}]{1}': invalid cell reference '{2}'")] + CellReferenceError(String, String, String), } pub(crate) trait Spreadsheet { diff --git a/src/spreadsheet/reference.rs b/src/spreadsheet/reference.rs index ce9a7f6..c409022 100644 --- a/src/spreadsheet/reference.rs +++ b/src/spreadsheet/reference.rs @@ -1,3 +1,6 @@ +const MAX_COL_INDEX: usize = 16_383; +const MAX_ROW_NUMBER: usize = 1_048_576; + /// Converts a zero-based column index to Excel column letter notation /// /// Uses a precomputed lookup table for optimal performance. Supports @@ -44,40 +47,12 @@ pub(crate) fn col_to_index(letters: &str) -> Option { let mut column = 0usize; for char in letters.chars() { if 'A' <= char && char <= 'Z' { - column = column * 26 + char as usize - 64; + column = column.checked_mul(26)?.checked_add(char as usize - 64)?; } else { break; } } - if column > 0 { - Some(column - 1) - } else { - None - } -} - -/// Converts a zero-based row index to Excel row number string -/// -/// Transforms internal zero-based row indexing into Excel's 1-based row numbering system. -/// This conversion bridges the gap between internal storage (0-based) and Excel's -/// user-facing representation (1-based). -/// -/// The conversion is straightforward: adds 1 to the zero-based index and converts -/// the result to a string. This maintains compatibility with Excel's row numbering -/// convention while using efficient zero-based indexing internally. -/// -/// # Arguments -/// * `index` - Zero-based row index (0 = row 1, 1 = row 2, etc.) -/// -/// # Returns -/// Excel-style row number as a string (1-based) -/// -/// # Examples -/// - 0 -> "1" // First row -/// - 1 -> "2" // Second row -/// - 99 -> "100" // 100th row -pub(crate) fn index_to_row(index: usize) -> String { - (index + 1).to_string() + zero_based_column(column) } /// Converts an Excel row number string to a zero-based row index @@ -89,22 +64,25 @@ pub(crate) fn index_to_row(index: usize) -> String { /// `Some(usize)` containing the zero-based row index if valid, /// `None` if the input cannot be parsed as a valid row number pub(crate) fn row_to_index(row: &str) -> Option { - row.parse::().ok().map(|row| row - 1) + row.parse::().ok().and_then(zero_based_row) } -/// Converts zero-based row and column indices to an Excel-style cell reference +/// Converts zero-based row and column indices to a cell reference /// /// # Arguments /// * `row_index` - Zero-based row index (0 = row 1) /// * `col_index` - Zero-based column index (0 = column "A") /// /// # Returns -/// Excel-style cell reference as a string (e.g., "A1", "B2", "AB100") +/// A1 notation for valid Excel coordinates, or R1C1 notation for out-of-range +/// coordinates so malformed-file error reporting remains panic-free. pub(crate) fn index_to_reference(row_index: usize, col_index: usize) -> String { - let mut reference = String::new(); - reference.push_str(index_to_col(col_index)); - reference.push_str(&index_to_row(row_index)); - reference + let row_number = row_index.saturating_add(1); + if let Some(column) = INDEXES_TO_COLUMNS.get(col_index) { + format!("{column}{row_number}") + } else { + format!("R{row_number}C{}", col_index.saturating_add(1)) + } } /// Converts an Excel-style cell reference to zero-based row and column indices @@ -135,17 +113,32 @@ pub(crate) fn index_to_reference(row_index: usize, col_index: usize) -> String { pub(crate) fn reference_to_index(letters: &str) -> Option<(usize, usize)> { let mut column = 0usize; let mut row = 0usize; + let mut row_started = false; for char in letters.chars() { - if 'A' <= char && char <= 'Z' { - column = column * 26 + char as usize - 64; + if 'A' <= char && char <= 'Z' && !row_started { + column = column.checked_mul(26)?.checked_add(char as usize - 64)?; } else if '0' <= char && char <= '9' { - row = row * 10 + char as usize - 48; + row_started = true; + row = row.checked_mul(10)?.checked_add(char as usize - 48)?; } else { - break; + return None; } } - if column > 0 { - Some((row - 1, column - 1)) + Some((zero_based_row(row)?, zero_based_column(column)?)) +} + +fn zero_based_column(column: usize) -> Option { + let index = column.checked_sub(1)?; + if index <= MAX_COL_INDEX { + Some(index) + } else { + None + } +} + +fn zero_based_row(row: usize) -> Option { + if (1..=MAX_ROW_NUMBER).contains(&row) { + Some(row - 1) } else { None } @@ -1418,4 +1411,26 @@ const INDEXES_TO_COLUMNS: [&'static str; 16384] = [ "XDS", "XDT", "XDU", "XDV", "XDW", "XDX", "XDY", "XDZ", "XEA", "XEB", "XEC", "XED", "XEE", "XEF", "XEG", "XEH", "XEI", "XEJ", "XEK", "XEL", "XEM", "XEN", "XEO", "XEP", "XEQ", "XER", "XES", "XET", "XEU", "XEV", "XEW", "XEX", "XEY", "XEZ", "XFA", "XFB", "XFC", "XFD", -]; \ No newline at end of file +]; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_reference_bounds() { + assert_eq!(col_to_index("XFD"), Some(16_383)); + assert_eq!(row_to_index("1048576"), Some(1_048_575)); + assert_eq!(reference_to_index("XFD1048576"), Some((1_048_575, 16_383))); + } + + #[test] + fn rejects_out_of_bounds_references() { + assert_eq!(col_to_index("XFE"), None); + assert_eq!(row_to_index("0"), None); + assert_eq!(row_to_index("1048577"), None); + assert_eq!(reference_to_index("A0"), None); + assert_eq!(reference_to_index("XFE1"), None); + assert_eq!(reference_to_index("A1048577"), None); + } +} diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index e983145..c0a256e 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -208,9 +208,17 @@ impl Spreadsheet for XlsxSpreadsheet { col_count = 0; } Event::Start(event) if event.name() == TAG_CELL => { - (row, col) = event.get_attribute_value("r")? - .and_then(|reference| reference_to_index(&reference)) - .unwrap_or((row_count, col_count)); + if let Some(reference) = event.get_attribute_value("r")? { + (row, col) = reference_to_index(&reference).ok_or_else(|| { + SpreadsheetError::CellReferenceError( + sheet.file_name.to_owned(), + sheet.name.to_owned(), + reference.to_string(), + ) + })?; + } else { + (row, col) = (row_count, col_count); + } col_count += 1; if sheet.after_row_upper_bound(row) { break; @@ -577,6 +585,23 @@ mod tests { assert!(error.contains("invalid shared string index 999")); } + #[test] + fn invalid_cell_reference_returns_error() { + let path = invalid_workbook_path("cell-reference"); + write_invalid_cell_reference_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&default_criteria()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid cell reference error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1")); + assert!(error.contains("invalid cell reference 'XFE1'")); + } + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -715,6 +740,58 @@ mod tests { r#" 999 +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn write_invalid_cell_reference_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/worksheets/sheet1.xml", + r#" + + value "#.to_string(), ), ] { From 1ac73cc60c08f9a707d9e5322271682addb9aeb7 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 13:01:52 -0400 Subject: [PATCH 04/12] Return errors for invalid display values --- src/extension/writer.rs | 2 +- src/spreadsheet/cell.rs | 90 ++++++++++++----------------------------- src/spreadsheet/mod.rs | 9 ++++- src/spreadsheet/xlsx.rs | 78 +++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 67 deletions(-) diff --git a/src/extension/writer.rs b/src/extension/writer.rs index 2b5b038..5f82228 100644 --- a/src/extension/writer.rs +++ b/src/extension/writer.rs @@ -43,7 +43,7 @@ pub(super) fn write_to_vector(sheet: &Sheet, column: &Column, cell: &Cell, vecto cell }; match column.kind { - ColumnType::Varchar => vector.insert(row, &cell.to_string()), + ColumnType::Varchar => vector.insert(row, &cell.to_display_string().map_err(mapper)?), ColumnType::Boolean => write_primitive(vector, row, cell.to_boolean()), ColumnType::BigInt => write_primitive(vector, row, cell.to_bigint().map_err(mapper)?), ColumnType::Double => write_primitive(vector, row, cell.to_double().map_err(mapper)?), diff --git a/src/spreadsheet/cell.rs b/src/spreadsheet/cell.rs index 8f4e5c5..715b471 100644 --- a/src/spreadsheet/cell.rs +++ b/src/spreadsheet/cell.rs @@ -243,76 +243,36 @@ impl Cell { _ => Err(format!("parse '{}' to datetime failed", self.value))?, } } -} -impl Display for Cell { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let value = match self.kind { - CellType::Boolean => if self.value == "1" { "true" } else { "false" }.to_owned(), - CellType::NumberDateTime1900 => { - if let Ok(value) = to_datetime_string(&self.value, false) { - value - } else { - panic!( - "Parse cell value '{}' at {} to DateTime(1900) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberDate1900 => { - if let Ok(value) = to_date_string(&self.value, false) { - value - } else { - panic!( - "Parse cell value '{}' at {} to Date(1900) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberDateTime1904 => { - if let Ok(value) = to_datetime_string(&self.value, true) { - value - } else { - panic!( - "Parse cell value '{}' at {} to DateTime(1904) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberDate1904 => { - if let Ok(value) = to_date_string(&self.value, true) { - value - } else { - panic!( - "Parse cell value '{}' at {} to Date(1904) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberTime1900 | CellType::NumberTime1904 => { - if let Ok(value) = to_time_string(&self.value) { - value - } else { - panic!( - "Parse cell value '{}' at {} to Time failed", - self.value, - self.reference() - ); - } - } - CellType::IsoDateTime => self.value.replace("T", " "), - CellType::IsoDuration => self + /// Converts the cell to the user-visible string representation. + pub(crate) fn to_display_string(&self) -> Result { + match self.kind { + CellType::Boolean => Ok(if self.value == "1" { "true" } else { "false" }.to_owned()), + CellType::NumberDateTime1900 => to_datetime_string(&self.value, false) + .map_err(|_| format!("parse '{}' to DateTime(1900) failed", self.value)), + CellType::NumberDate1900 => to_date_string(&self.value, false) + .map_err(|_| format!("parse '{}' to Date(1900) failed", self.value)), + CellType::NumberDateTime1904 => to_datetime_string(&self.value, true) + .map_err(|_| format!("parse '{}' to DateTime(1904) failed", self.value)), + CellType::NumberDate1904 => to_date_string(&self.value, true) + .map_err(|_| format!("parse '{}' to Date(1904) failed", self.value)), + CellType::NumberTime1900 | CellType::NumberTime1904 => to_time_string(&self.value) + .map_err(|_| format!("parse '{}' to Time failed", self.value)), + CellType::IsoDateTime => Ok(self.value.replace("T", " ")), + CellType::IsoDuration => Ok(self .value .replace("PT", "") .replace("H", ":") .replace("M", ":") - .replace("S", ""), - _ => self.value.to_owned(), - }; + .replace("S", "")), + _ => Ok(self.value.to_owned()), + } + } +} + +impl Display for Cell { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let value = self.to_display_string().unwrap_or_else(|_| self.value.to_owned()); write!(f, "{}", value) } } diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index c9e7fa4..2a49881 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -230,7 +230,14 @@ pub(crate) trait Spreadsheet { cell, )?.to_owned() } else { - cell.to_string() + cell.to_display_string().map_err(|message| { + SpreadsheetError::CellValueError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + message, + ) + })? }; Ok(if !criteria.nulls.contains(&value) { value diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index c0a256e..b0fe368 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -602,6 +602,23 @@ mod tests { assert!(error.contains("invalid cell reference 'XFE1'")); } + #[test] + fn invalid_date_header_value_returns_error() { + let path = invalid_workbook_path("date-header"); + write_invalid_date_header_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.analyze_sheets(true, &default_criteria(), &Vec::new()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid date header value error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A1")); + assert!(error.contains("parse 'not-a-number' to Date(1900) failed")); + } + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -792,6 +809,67 @@ mod tests { r#" value +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn write_invalid_date_header_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + + +"#.to_string(), + ), + ( + "xl/styles.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/worksheets/sheet1.xml", + r#" + + not-a-number "#.to_string(), ), ] { From 8ed150c5fdede1cb7d9b8cac10ca26ec90ceabd3 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 13:26:14 -0400 Subject: [PATCH 05/12] Return errors for short BIFF12 records --- src/helpers/biff12.rs | 87 +++++++++++++++++++++++++++++++---------- src/spreadsheet/xlsb.rs | 81 +++++++++++++++++++++----------------- 2 files changed, 112 insertions(+), 56 deletions(-) diff --git a/src/helpers/biff12.rs b/src/helpers/biff12.rs index c0cc814..3bab2a7 100644 --- a/src/helpers/biff12.rs +++ b/src/helpers/biff12.rs @@ -42,15 +42,14 @@ impl Biff12Reader { &'_ self, at: usize, ) -> Result<(Cow<'_, str>, usize), RustySheetError> { - let lower_bound = at + 4usize; - let size = to_usize(&self.buffer[at..at + lower_bound]); - let upper_bound = lower_bound + size * 2; - if self.buffer.len() >= upper_bound { - let (value, _, _) = UTF_16LE.decode(&self.buffer[lower_bound..upper_bound]); - Ok((value, upper_bound)) - } else { - Err(Biff12Error::NoEnoughData(upper_bound, self.buffer.len()))? - } + let size = self.get_usize(at)?; + let lower_bound = checked_offset(at, 4, self.buffer.len())?; + let byte_length = size + .checked_mul(2) + .ok_or(Biff12Error::NoEnoughData(usize::MAX, self.buffer.len()))?; + let upper_bound = checked_offset(lower_bound, byte_length, self.buffer.len())?; + let (value, _, _) = UTF_16LE.decode(self.slice(lower_bound, byte_length)?); + Ok((value, upper_bound)) } /// Reads a UTF-16 string from the specified position @@ -60,33 +59,39 @@ impl Biff12Reader { } /// Reads a usize value from the specified position - pub(crate) fn get_usize(&'_ self, at: usize) -> usize { - to_usize(&self.buffer[at..at + 4]) + pub(crate) fn get_usize(&'_ self, at: usize) -> Result { + Ok(to_usize(self.slice(at, 4)?)) } /// Reads a u16 value from the specified position - pub(crate) fn get_u16(&'_ self, at: usize) -> u16 { - to_u16(&self.buffer[at..at + 2]) + pub(crate) fn get_u16(&'_ self, at: usize) -> Result { + Ok(to_u16(self.slice(at, 2)?)) } /// Reads a u32 value from the specified position - pub(crate) fn get_u32(&'_ self, at: usize) -> u32 { - to_u32(&self.buffer[at..at + 4]) + pub(crate) fn get_u32(&'_ self, at: usize) -> Result { + Ok(to_u32(self.slice(at, 4)?)) } /// Reads an i32 value from the specified position - pub(crate) fn get_i32(&'_ self, at: usize) -> i32 { - to_i32(&self.buffer[at..at + 4]) + pub(crate) fn get_i32(&'_ self, at: usize) -> Result { + Ok(to_i32(self.slice(at, 4)?)) } /// Reads an f64 value from the specified position - pub(crate) fn get_f64(&'_ self, at: usize) -> f64 { - to_f64(&self.buffer[at..at + 8]) + pub(crate) fn get_f64(&'_ self, at: usize) -> Result { + Ok(to_f64(self.slice(at, 8)?)) } /// Reads a style index from the specified position (3 bytes, padded to 4) - pub(crate) fn get_style(&'_ self, at: usize) -> usize { - to_usize(&[self.buffer[at], self.buffer[at + 1], self.buffer[at + 2], 0]) + pub(crate) fn get_style(&'_ self, at: usize) -> Result { + let bytes = self.slice(at, 3)?; + Ok(to_usize(&[bytes[0], bytes[1], bytes[2], 0])) + } + + /// Reads a byte from the specified position + pub(crate) fn get_u8(&'_ self, at: usize) -> Result { + Ok(self.slice(at, 1)?[0]) } /// Reads a 7-bit continuation integer with the specified byte limit @@ -150,6 +155,46 @@ impl Biff12Reader { pub(crate) fn find(&mut self, target: u16) -> Result { self.find_with(target, &[]) } + + fn slice(&self, at: usize, length: usize) -> Result<&[u8], RustySheetError> { + let upper_bound = checked_offset(at, length, self.buffer.len())?; + Ok(&self.buffer[at..upper_bound]) + } +} + +fn checked_offset(at: usize, length: usize, actual: usize) -> Result { + let upper_bound = at + .checked_add(length) + .ok_or(Biff12Error::NoEnoughData(usize::MAX, actual))?; + if upper_bound <= actual { + Ok(upper_bound) + } else { + Err(Biff12Error::NoEnoughData(upper_bound, actual)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Cursor; + + #[test] + fn fixed_width_reads_return_errors_for_short_records() { + let mut reader = Biff12Reader::new(Cursor::new(Vec::new())); + reader.buffer = vec![0; 8]; + + assert!(reader.get_f64(1).unwrap_err().to_string().contains("No enough data")); + assert!(reader.get_style(6).unwrap_err().to_string().contains("No enough data")); + } + + #[test] + fn string_reads_return_errors_for_short_records() { + let mut reader = Biff12Reader::new(Cursor::new(Vec::new())); + reader.buffer = vec![2, 0, 0, 0, b'a']; + + assert!(reader.get_str(0).unwrap_err().to_string().contains("No enough data")); + assert!(reader.get_str(3).unwrap_err().to_string().contains("No enough data")); + } } #[macro_export] diff --git a/src/spreadsheet/xlsb.rs b/src/spreadsheet/xlsb.rs index 12dafe7..11396ef 100644 --- a/src/spreadsheet/xlsb.rs +++ b/src/spreadsheet/xlsb.rs @@ -135,7 +135,7 @@ impl Spreadsheet for XlsbSpreadsheet { }; reader.find(BRT_BEGIN_SST)?; - for id in 0..reader.get_usize(4) { + for id in 0..reader.get_usize(4)? { reader.find_with(BRT_SST_ITEM, &[(BRT_FRT_BEGIN, BRT_FRT_END)])?; if let Some(keys) = &mut indexes { if keys.contains(&id) { @@ -190,7 +190,7 @@ impl Spreadsheet for XlsbSpreadsheet { match tag { BRT_END_SHEET_DATA => break, BRT_ROW_HDR => { - row = reader.get_usize(0); + row = reader.get_usize(0)?; if sheet.after_row_upper_bound(row) { break; } @@ -203,7 +203,7 @@ impl Spreadsheet for XlsbSpreadsheet { | BRT_CELL_ISST | BRT_CELL_ERROR | BRT_FMLA_ERROR if !sheet.before_row_lower_bound(row) => { - let col = reader.get_usize(0); + let col = reader.get_usize(0)?; if sheet.contains(row, col) { if let Some(last_row) = last_row { if criteria.end_at_empty_row && ((sheet.is_empty() && last_row != row) || (!sheet.is_empty() && last_row + 1 < row)) { @@ -212,13 +212,13 @@ impl Spreadsheet for XlsbSpreadsheet { } last_row = Some(row); let (either, value) = match tag { - BRT_CELL_BOOL | BRT_FMLA_BOOL => read_bool_cell(&mut reader), - BRT_CELL_REAL | BRT_FMLA_NUM => read_real_cell(&mut reader), + BRT_CELL_BOOL | BRT_FMLA_BOOL => read_bool_cell(&mut reader)?, + BRT_CELL_REAL | BRT_FMLA_NUM => read_real_cell(&mut reader)?, BRT_CELL_ST | BRT_FMLA_STRING => read_st_cell(&mut reader)?, BRT_CELL_R_STRING => read_rich_string_cell(&mut reader)?, - BRT_CELL_ISST => read_shared_string_cell(&mut reader), - BRT_CELL_ERROR | BRT_FMLA_ERROR => read_error_cell(&mut reader), - _ => read_rk_cell(&mut reader), + BRT_CELL_ISST => read_shared_string_cell(&mut reader)?, + BRT_CELL_ERROR | BRT_FMLA_ERROR => read_error_cell(&mut reader)?, + _ => read_rk_cell(&mut reader)?, }; let kind = match either { Either::Left(kind) => kind, @@ -290,7 +290,7 @@ fn load_workbook(zip: &mut ZipArchive) -> Result<(Vec<(String, St } } BRT_WB_PROP => { - is_1904 = (&reader.buffer[0] & 0x1) != 0; + is_1904 = (reader.get_u8(0)? & 0x1) != 0; } }); Ok((sheets, is_1904)) @@ -317,9 +317,9 @@ fn load_number_formats(zip: &mut ZipArchive, is_1904: bool) -> Re let mut format_indexes: Vec = Vec::new(); match_biff12_record!(reader => { BRT_BEGIN_FMTS => { - for _ in 0..reader.get_usize(0) { + for _ in 0..reader.get_usize(0)? { reader.find(BRT_FMT)?; - let id = reader.get_u16(0); + let id = reader.get_u16(0)?; let format = reader.get_str(2)?; custom_formats.insert( id.to_string(), @@ -328,9 +328,9 @@ fn load_number_formats(zip: &mut ZipArchive, is_1904: bool) -> Re } } BRT_BEGIN_CELL_XFS => { - for _ in 0..reader.get_usize(0) { + for _ in 0..reader.get_usize(0)? { reader.find(BRT_XF)?; - let id = reader.get_u16(2); + let id = reader.get_u16(2)?; format_indexes.push(id.to_string()); } break; @@ -349,9 +349,11 @@ fn load_number_formats(zip: &mut ZipArchive, is_1904: bool) -> Re /// * `(Either, String)` - Tuple containing: /// - Cell type (boolean) and format index /// - String representation of boolean value ("1" or "0") -fn read_bool_cell(reader: &mut Biff12Reader>>) -> (Either, String) { - let value = if reader.buffer[8] != 0 { "1" } else { "0" }; - (Either::Left(CellType::Boolean), value.to_owned()) +fn read_bool_cell( + reader: &mut Biff12Reader>>, +) -> Result<(Either, String), RustySheetError> { + let value = if reader.get_u8(8)? != 0 { "1" } else { "0" }; + Ok((Either::Left(CellType::Boolean), value.to_owned())) } /// Reads a real number (double precision) cell value from BIFF12 data @@ -363,10 +365,12 @@ fn read_bool_cell(reader: &mut Biff12Reader>>) /// * `(Either, String)` - Tuple containing: /// - Format index reference and cell type /// - String representation of numeric value -fn read_real_cell(reader: &mut Biff12Reader>>) -> (Either, String) { - let index = reader.get_style(4); - let value = reader.get_f64(8).to_string(); - (Either::Right(index), value) +fn read_real_cell( + reader: &mut Biff12Reader>>, +) -> Result<(Either, String), RustySheetError> { + let index = reader.get_style(4)?; + let value = reader.get_f64(8)?.to_string(); + Ok((Either::Right(index), value)) } /// Reads an inline string cell value from BIFF12 data @@ -406,9 +410,11 @@ fn read_rich_string_cell(reader: &mut Biff12Reader, String)` - Tuple containing: /// - Cell type (shared string) and format index /// - String representation of shared string index -fn read_shared_string_cell(reader: &mut Biff12Reader>>) -> (Either, String) { - let value = reader.get_usize(8).to_string(); - (Either::Left(CellType::SharedString), value) +fn read_shared_string_cell( + reader: &mut Biff12Reader>>, +) -> Result<(Either, String), RustySheetError> { + let value = reader.get_usize(8)?.to_string(); + Ok((Either::Left(CellType::SharedString), value)) } /// Reads an error cell value from BIFF12 data @@ -420,9 +426,11 @@ fn read_shared_string_cell(reader: &mut Biff12Reader, String)` - Tuple containing: /// - Cell type (error) and format index /// - String representation of error value -fn read_error_cell(reader: &mut Biff12Reader>>) -> (Either, String) { - let value = to_error_value(reader.buffer[8]).to_owned(); - (Either::Left(CellType::Error), value) +fn read_error_cell( + reader: &mut Biff12Reader>>, +) -> Result<(Either, String), RustySheetError> { + let value = to_error_value(reader.get_u8(8)?).to_owned(); + Ok((Either::Left(CellType::Error), value)) } /// Reads an RK (compressed floating point) cell value from BIFF12 data @@ -437,16 +445,19 @@ fn read_error_cell(reader: &mut Biff12Reader>>) /// * `(Either, String)` - Tuple containing: /// - Format index reference and cell type /// - String representation of decompressed numeric value -fn read_rk_cell(reader: &mut Biff12Reader>>) -> (Either, String) { - let index = reader.get_style(4); - let is_percentage = (reader.buffer[8] & 0x01) != 0; - let is_integer = (reader.buffer[8] & 0x02) != 0; - reader.buffer[8] &= 0xFC; // Clear A and B flag bits +fn read_rk_cell( + reader: &mut Biff12Reader>>, +) -> Result<(Either, String), RustySheetError> { + let index = reader.get_style(4)?; + let flags = reader.get_u8(8)?; + let is_percentage = (flags & 0x01) != 0; + let is_integer = (flags & 0x02) != 0; + reader.buffer[8] = flags & 0xFC; // Clear A and B flag bits let mut value = if is_integer { - (reader.get_i32(8) >> 2) as f64 + (reader.get_i32(8)? >> 2) as f64 } else { - let value = (reader.get_u32(8) >> 2) as u64; + let value = (reader.get_u32(8)? >> 2) as u64; f64::from_bits(value << 34) }; if is_percentage { @@ -458,5 +469,5 @@ fn read_rk_cell(reader: &mut Biff12Reader>>) -> value.to_string() }; - (Either::Right(index), value) - } + Ok((Either::Right(index), value)) +} From 1bb8143d71bdc0c11814151cd1264eccdad9b5e6 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 13:33:48 -0400 Subject: [PATCH 06/12] Return errors for missing spreadsheet XML parts --- src/spreadsheet/ods.rs | 152 ++++++++++++++++++++++++++++++++++++++-- src/spreadsheet/xlsx.rs | 76 +++++++++++++++++++- 2 files changed, 218 insertions(+), 10 deletions(-) diff --git a/src/spreadsheet/ods.rs b/src/spreadsheet/ods.rs index e02a678..1ab2384 100644 --- a/src/spreadsheet/ods.rs +++ b/src/spreadsheet/ods.rs @@ -1,6 +1,7 @@ use crate::error::RustySheetError; use crate::helpers::reader::UnifiedReader; use crate::helpers::xml::XmlNodeHelper; +use crate::helpers::xml::XmlReader; use crate::helpers::xml::XmlTextContextHelper; use crate::helpers::zip::ZipHelper; use crate::match_xml_events; @@ -15,8 +16,10 @@ use quick_xml::events::Event; use quick_xml::name::QName; use std::collections::HashMap; use std::collections::HashSet; +use std::io::BufReader; use std::io::Read; use thiserror::Error; +use zip::read::ZipFile; use zip::ZipArchive; /// ODS file MIME type identifier @@ -111,14 +114,14 @@ impl Spreadsheet for OdsSpreadsheet { let mut sheets = Vec::::new(); let mut sheet_count = 0usize; let mut sheet_name = String::new(); - let mut reader = self.zip - .xml_reader("content.xml")? - .expect("content.xml"); + let mut reader = required_xml_reader(&mut self.zip, "content.xml")?; 'sheets: loop { match_xml_events!(reader => { Event::End(event) if event.name() == SPREADSHEET => break 'sheets, Event::Start(event) if event.name() == TABLE => { - let table_name = event.get_attribute_value("table:name")?.expect("Sheet name"); + let table_name = event + .get_attribute_value("table:name")? + .ok_or_else(|| SpreadsheetError::FileError("content.xml".to_string()))?; sheet_name.clear(); sheet_name.push_str(&table_name); if criteria.sheet_limit.map(|limit| sheet_count >= limit).unwrap_or(false) { @@ -298,9 +301,7 @@ fn check_mime(zip: &mut ZipArchive) -> Result<(), RustySheetError /// # Returns /// * `Result` - True if password protected, false otherwise fn is_password_protected(zip: &mut ZipArchive) -> Result { - let mut reader = zip - .xml_reader("META-INF/manifest.xml")? - .expect("META-INF/manifest.xml"); + let mut reader = required_xml_reader(zip, "META-INF/manifest.xml")?; let mut in_file_entry = false; match_xml_events!(reader => { Event::Start(event) if event.name() == QName(b"manifest:file-entry") => in_file_entry = true, @@ -310,3 +311,140 @@ fn is_password_protected(zip: &mut ZipArchive) -> Result( + zip: &'a mut ZipArchive, + path: &str, +) -> Result>>, RustySheetError> { + zip.xml_reader(path)?.ok_or_else(|| { + RustySheetError::from(SpreadsheetError::FileError(path.to_string())) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::spreadsheet::criteria::Criteria; + use std::collections::HashSet; + use std::fs::File; + use std::io::Write; + use std::path::Path; + use std::path::PathBuf; + use std::time::SystemTime; + use std::time::UNIX_EPOCH; + use zip::write::SimpleFileOptions; + use zip::ZipWriter; + + #[test] + fn missing_manifest_xml_returns_error() { + let path = ods_path("missing-manifest"); + write_ods(&path, false, Some(valid_content_xml())); + + let result = OdsSpreadsheet::open(path.to_str().unwrap()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected missing manifest XML error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("META-INF/manifest.xml")); + assert!(error.contains("missing or corrupted")); + } + + #[test] + fn missing_content_xml_returns_error() { + let path = ods_path("missing-content"); + write_ods(&path, true, None); + + let mut spreadsheet = OdsSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&default_criteria()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected missing content XML error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("content.xml")); + assert!(error.contains("missing or corrupted")); + } + + #[test] + fn missing_table_name_returns_error() { + let path = ods_path("missing-table-name"); + write_ods(&path, true, Some(unnamed_table_content_xml())); + + let mut spreadsheet = OdsSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&default_criteria()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected missing table name error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("content.xml")); + assert!(error.contains("missing or corrupted")); + } + + fn default_criteria() -> Criteria { + Criteria { + sheet_name_patterns: None, + sheet_limit: None, + range: None, + rows_limit: None, + nulls: HashSet::from(["".to_string()]), + error_as_null: false, + skip_empty_rows: false, + end_at_empty_row: false, + spread_merged_cells: false, + } + } + + fn ods_path(kind: &str) -> PathBuf { + let id = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + std::env::temp_dir().join(format!("rusty-sheet-invalid-{kind}-{id}.ods")) + } + + fn write_ods(path: &Path, include_manifest: bool, content_xml: Option<&str>) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + + zip.start_file("mimetype", options).unwrap(); + zip.write_all(MIME_TYPE).unwrap(); + + if include_manifest { + zip.start_file("META-INF/manifest.xml", options).unwrap(); + zip.write_all(manifest_xml().as_bytes()).unwrap(); + } + if let Some(content_xml) = content_xml { + zip.start_file("content.xml", options).unwrap(); + zip.write_all(content_xml.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn manifest_xml() -> &'static str { + r#" + + +"# + } + + fn valid_content_xml() -> &'static str { + r#" + + +"# + } + + fn unnamed_table_content_xml() -> &'static str { + r#" + + +"# + } +} diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index b0fe368..ad809a9 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -172,7 +172,7 @@ impl Spreadsheet for XlsxSpreadsheet { }; // Single pass: Read cells and collect merged cell ranges - let mut reader = self.zip.xml_reader(zip_path)?.expect(sheet_name); + let mut reader = required_xml_reader(&mut self.zip, zip_path)?; let mut in_merge_cells = false; match_xml_events!(reader => { @@ -383,8 +383,7 @@ impl Spreadsheet for XlsxSpreadsheet { /// Tuple of (worksheets, is_1904_date_system) where worksheets are (name, zip_path) pairs fn load_workbook(zip: &mut ZipArchive) -> Result<(Vec<(String, String)>, bool), RustySheetError> { let relationships = load_relationships(zip, "xl/_rels/workbook.xml.rels")?; - let mut reader = zip.xml_reader("xl/workbook.xml")? - .ok_or_else(|| SpreadsheetError::FileError("xl/workbook.xml".to_string()))?; + let mut reader = required_xml_reader(zip, "xl/workbook.xml")?; let mut sheets: Vec<(String, String)> = Vec::new(); let mut is_1904 = false; match_xml_events!(reader => { @@ -503,6 +502,15 @@ fn parse_merge_range(range_ref: &str) -> Option<(usize, usize, usize, usize)> { Some((top_row, top_col, bottom_row, bottom_col)) } +fn required_xml_reader<'a>( + zip: &'a mut ZipArchive, + path: &str, +) -> Result>>, RustySheetError> { + zip.xml_reader(path)?.ok_or_else(|| { + RustySheetError::from(SpreadsheetError::FileError(path.to_string())) + }) +} + /// Reads string value from XML content, handling text and CDATA sections /// /// Extracts string content from XML elements, skipping phonetic text annotations @@ -619,6 +627,23 @@ mod tests { assert!(error.contains("parse 'not-a-number' to Date(1900) failed")); } + #[test] + fn missing_sheet_xml_returns_error() { + let path = invalid_workbook_path("missing-sheet"); + write_missing_sheet_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&default_criteria()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected missing sheet XML error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("xl/worksheets/sheet1.xml")); + assert!(error.contains("missing or corrupted")); + } + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -878,4 +903,49 @@ mod tests { } zip.finish().unwrap(); } + + fn write_missing_sheet_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } } From b31254cce8cea54c137b8edf428a0e48e0085fdf Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Fri, 7 Aug 2026 18:35:26 -0400 Subject: [PATCH 07/12] Stream local XLSX scans with bounded memory --- Cargo.lock | 32 ++++- Cargo.toml | 1 + src/extension/read_sheet.rs | 115 +++++++++--------- src/helpers/cfb.rs | 27 ++++- src/spreadsheet/mod.rs | 76 ++++++++++++ src/spreadsheet/sheet.rs | 56 ++++++++- src/spreadsheet/xlsx.rs | 236 ++++++++++++++++++++++++++++++++++++ 7 files changed, 476 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51fb164..a8e6c5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -672,6 +672,12 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7360491ce676a36bf9bb3c56c1aa791658183a54d2744120f27285738d90465a" +[[package]] +name = "fastrand" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" + [[package]] name = "filetime" version = "0.2.26" @@ -1249,9 +1255,9 @@ checksum = "2c4a545a15244c7d945065b5d392b2d2d7f21526fba56ce51467b06ed445e8f7" [[package]] name = "libc" -version = "0.2.178" +version = "0.2.189" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "37c93d8daa9d8a012fd8ab92f088405fb202ea0b6ab73ee2482ae66af4f42091" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" [[package]] name = "libduckdb-sys" @@ -1300,9 +1306,9 @@ dependencies = [ [[package]] name = "linux-raw-sys" -version = "0.11.0" +version = "0.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df1d3c3b53da64cf5760482273a98e575c651a67eec7f77df96b5b642de8f039" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" [[package]] name = "litemap" @@ -1871,9 +1877,9 @@ checksum = "357703d41365b4b27c590e3ed91eabb1b663f07c4c084095e60cbed4362dff0d" [[package]] name = "rustix" -version = "1.1.2" +version = "1.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd15f8a2c5551a84d56efdc1cd049089e409ac19a3072d5037a17fd70719ff3e" +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" dependencies = [ "bitflags", "errno", @@ -1938,6 +1944,7 @@ dependencies = [ "libduckdb-sys", "quick-xml", "regex", + "tempfile", "thiserror", "url", "zip", @@ -2189,6 +2196,19 @@ dependencies = [ "xattr", ] +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom 0.3.4", + "once_cell", + "rustix", + "windows-sys 0.61.2", +] + [[package]] name = "thiserror" version = "2.0.17" diff --git a/Cargo.toml b/Cargo.toml index 846b26c..0dad3e3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,5 +32,6 @@ libduckdb-sys = { version = "1.4.3", features = ["loadable-extension"] } quick-xml = "0.38.4" regex = "1.12.2" thiserror = "2.0.17" +tempfile = "3.23.0" url = "2.5.7" zip = { version = "6.0.0", features = ["deflate"] } diff --git a/src/extension/read_sheet.rs b/src/extension/read_sheet.rs index 5449320..f8d73d7 100644 --- a/src/extension/read_sheet.rs +++ b/src/extension/read_sheet.rs @@ -1,4 +1,3 @@ -use std::collections::HashSet; use crate::database::column::Column; use crate::database::column::ColumnType; use crate::error::ResultMessage; @@ -23,7 +22,7 @@ use crate::extension::SheetParam; use crate::extension::SkipEmptyRowsParam; use crate::spreadsheet::criteria::Criteria; use crate::spreadsheet::open_spreadsheet; -use crate::spreadsheet::sheet::Sheet; +use crate::spreadsheet::SheetBatch; use anyhow::Result; use duckdb::core::DataChunkHandle; use duckdb::core::Inserter; @@ -33,9 +32,11 @@ use duckdb::vtab::InitInfo; use duckdb::vtab::TableFunctionInfo; use duckdb::vtab::VTab; use glob::Pattern; +use std::collections::HashSet; use std::error::Error; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::Ordering; +use std::sync::mpsc::sync_channel; +use std::sync::mpsc::Receiver; +use std::sync::Mutex; /// Parameters for reading a single sheet from a spreadsheet file. struct ReadSheetParameters { @@ -101,12 +102,10 @@ pub(crate) struct ReadSheetBindData { file_name_column: Option, /// sheet name column index sheet_name_column: Option, - /// Loaded sheet data organized in chunks for efficient processing - sheets: Vec, - /// Shared string table for efficient string storage (XLSX/XLSB format) - shared_strings: Vec>, - /// Whether to spread merged cells across merged ranges (default: false) - spread_merged_cells: bool, + /// Spreadsheet path reopened by the scan worker after binding. + file_name: String, + /// Criteria for the full scan after the bounded schema sample. + criteria: Criteria, } impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { @@ -118,9 +117,8 @@ impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { // Prepare sheet name pattern for matching let sheet_name_pattern = parameters.sheet_name.as_ref().map(|pattern| vec![pattern.to_owned()]); - // Open the spreadsheet file and load shared strings (for XLSX/XLSB formats) + // Open the spreadsheet file for the bounded schema sample. let mut spreadsheet = open_spreadsheet(¶meters.file_name)?; - let (shared_strings, _) = spreadsheet.load_shared_strings(None)?; // Set default values for optional parameters let header = parameters.header.unwrap_or(true); @@ -143,7 +141,7 @@ impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { }, parameters.columns.as_ref().unwrap_or(&vec![]))?; // Extract the first matching sheet or return error if no match found - let table = tables.get(0).ok_or_else(|| ExtensionError::SheetWildcardError( + let table = tables.first().ok_or_else(|| ExtensionError::SheetWildcardError( spreadsheet.name().to_owned(), parameters.sheet_name.as_ref().map(|it| it.to_string()).unwrap_or(String::new()), ))?; @@ -163,8 +161,7 @@ impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { }); } - // Read the actual data from the spreadsheet using the analyzed structure - let sheets = spreadsheet.read_sheets(&Criteria { + let criteria = Criteria { sheet_name_patterns: sheet_name_pattern.to_owned(), sheet_limit: Some(1), range: Some(Range { @@ -179,25 +176,13 @@ impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { skip_empty_rows, end_at_empty_row, spread_merged_cells, - })?; - - let shared_strings = shared_strings - .into_iter() - .map(|shared_string| { - if !nulls.contains(&shared_string) { - Some(shared_string) - } else { - None - } - }) - .collect(); + }; Ok(ReadSheetBindData { columns, file_name_column, sheet_name_column, - sheets, - shared_strings, - spread_merged_cells, + file_name: parameters.file_name.clone(), + criteria, }) } } @@ -206,8 +191,8 @@ impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { /// Initialization data for the table function execution phase. /// This tracks the current processing state and column projections. pub(crate) struct ReadSheetInitData { - /// Atomic counter tracking the current chunk being processed - index: AtomicUsize, + /// Bounded queue fed by the spreadsheet parser. + receiver: Mutex>>, /// Column indices that should be projected (output) from the source data projections: Vec, } @@ -235,12 +220,25 @@ impl VTab for ReadSheetTableFunction { /// Initializes the table function for execution. /// This sets up the processing state and column projections for the current query. fn init(init: &InitInfo) -> Result> { + let bind: *const Self::BindData = init.get_bind_data(); + let (file_name, criteria) = unsafe { + ((*bind).file_name.clone(), (*bind).criteria.clone()) + }; let projections = init.get_column_indices() .into_iter() .map(|index| index as usize) .collect::>(); + let (sender, receiver) = sync_channel(2); + std::thread::spawn(move || { + let result = open_spreadsheet(&file_name).and_then(|mut spreadsheet| { + spreadsheet.stream_sheets(&criteria, &mut |batch| sender.send(Ok(batch)).is_ok()) + }); + if let Err(error) = result { + let _ = sender.send(Err(error.to_string())); + } + }); Ok(ReadSheetInitData { - index: AtomicUsize::new(0), + receiver: Mutex::new(receiver), projections, }) } @@ -253,33 +251,38 @@ impl VTab for ReadSheetTableFunction { ) -> Result<(), Box> { let bind = func.get_bind_data(); let init = func.get_init_data(); - let sheet = &bind.sheets[0]; - let shared_strings = &bind.shared_strings; - let index = init.index.fetch_add(1, Ordering::Relaxed); - if index < sheet.chunks.len() { - let mut vectors: Vec<_> = (0..init.projections.len()).map(|index| output.flat_vector(index)).collect(); - if let Some(table) = sheet.chunk(index) { - output.set_len(table.len()); - for (row, record) in table.iter().enumerate() { - for (index, col) in init.projections.iter().enumerate() { - let vector = &mut vectors[index]; - if bind.file_name_column.map(|column| column == *col).unwrap_or(false) { - vector.insert(row, sheet.file_name.as_str()); - } else if bind.sheet_name_column.map(|column| column == *col).unwrap_or(false) { - vector.insert(row, sheet.name.as_str()); - } else if let Some(cell) = record[*col] { - let column = &bind.columns[*col]; - write_to_vector(sheet, column, cell, vector, row, shared_strings)?; - } else { - vector.set_null(row); - } + let batch = init.receiver + .lock() + .map_err(|_| std::io::Error::other("spreadsheet stream lock poisoned"))? + .recv(); + let batch = match batch { + Ok(Ok(batch)) => batch, + Ok(Err(error)) => return Err(std::io::Error::other(error).into()), + Err(_) => { + output.set_len(0); + return Ok(()); + } + }; + let sheet = &batch.sheet; + let mut vectors: Vec<_> = (0..init.projections.len()).map(|index| output.flat_vector(index)).collect(); + if let Some(table) = sheet.chunk(0) { + output.set_len(table.len()); + for (row, record) in table.iter().enumerate() { + for (index, col) in init.projections.iter().enumerate() { + let vector = &mut vectors[index]; + if bind.file_name_column.map(|column| column == *col).unwrap_or(false) { + vector.insert(row, sheet.file_name.as_str()); + } else if bind.sheet_name_column.map(|column| column == *col).unwrap_or(false) { + vector.insert(row, sheet.name.as_str()); + } else if let Some(cell) = record[*col] { + let column = &bind.columns[*col]; + write_to_vector(sheet, column, cell, vector, row, batch.shared_strings.as_ref())?; + } else { + vector.set_null(row); } } - } else { - output.set_len(0); } } else { - // No more data to process output.set_len(0); } Ok(()) diff --git a/src/helpers/cfb.rs b/src/helpers/cfb.rs index 7f00b90..f4bb69a 100644 --- a/src/helpers/cfb.rs +++ b/src/helpers/cfb.rs @@ -61,17 +61,22 @@ pub(crate) struct Cfb { impl Cfb { /// Creates a new CFB structure by reading and parsing the entire file pub(crate) fn new(reader: &mut RS) -> Result { - // Load the entire CFB content into memory let size = reader.seek(SeekFrom::End(0))?; if size < 512 { Err(CfbError::FileFormatError)?; } reader.seek(SeekFrom::Start(0))?; + let mut header_bytes = [0u8; 512]; + reader.read_exact(&mut header_bytes)?; + let header = Header::new(&header_bytes)?; + let sector_size = header.sector_size()?; + + // Load the complete file only after its header identifies it as CFB. let mut data: Vec = vec![0u8; size as usize]; - reader.read_exact(&mut data)?; + data[..512].copy_from_slice(&header_bytes); + reader.read_exact(&mut data[512..])?; // Parse the data - let header = Header::new(&data[..512])?; - let sectors = Sectors { data, size: header.sector_size()? }; + let sectors = Sectors { data, size: sector_size }; let file_allocation_table = Self::load_file_allocation_table(§ors, &header)?; let directories = Self::load_directories(&file_allocation_table, §ors, header.directory_shift)?; let mini_file_allocation_table = Self::load_mini_file_allocation_table(&file_allocation_table, §ors, &header)?; @@ -276,3 +281,17 @@ impl Directory { (name, Directory { index, count }) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Cursor; + + #[test] + fn non_cfb_input_stops_after_header() { + let mut reader = Cursor::new(vec![0_u8; 4096]); + + assert!(Cfb::new(&mut reader).is_err()); + assert_eq!(reader.stream_position().unwrap(), 512); + } +} diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index 2a49881..df30488 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -15,6 +15,7 @@ use glob::Pattern; use sheet::Sheet; use std::collections::HashMap; use std::collections::HashSet; +use std::sync::Arc; use thiserror::Error; pub(crate) mod cell; @@ -156,6 +157,29 @@ pub(crate) enum SpreadsheetError { CellReferenceError(String, String, String), } +pub(crate) struct SheetBatch { + pub(crate) sheet: Sheet, + pub(crate) shared_strings: Arc>>, +} + +pub(crate) fn stream_materialized_sheets( + sheets: Vec, + shared_strings: Arc>>, + consumer: &mut dyn FnMut(SheetBatch) -> bool, +) -> bool { + for mut sheet in sheets { + while let Some(sheet) = sheet.take_ready_chunk() { + if !consumer(SheetBatch { + sheet, + shared_strings: Arc::clone(&shared_strings), + }) { + return false; + } + } + } + true +} + pub(crate) trait Spreadsheet { /// Returns the name of the spreadsheet file fn name(&self) -> String; @@ -305,6 +329,24 @@ pub(crate) trait Spreadsheet { &mut self, criteria: &Criteria, ) -> Result, RustySheetError>; + + /// Sends sheets to the consumer as they become available. Formats can + /// override this to avoid materializing the complete sheet in memory. + fn stream_sheets( + &mut self, + criteria: &Criteria, + consumer: &mut dyn FnMut(SheetBatch) -> bool, + ) -> Result<(), RustySheetError> { + let shared_strings = Arc::new( + self.load_shared_strings(None)? + .0 + .into_iter() + .map(|value| (!criteria.nulls.contains(&value)).then_some(value)) + .collect(), + ); + stream_materialized_sheets(self.read_sheets(criteria)?, shared_strings, consumer); + Ok(()) + } } /// Opens a spreadsheet file based on its format @@ -353,3 +395,37 @@ pub(crate) fn open_spreadsheets(files: &Vec, patterns: &Option>(); Ok(spreadsheets) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::spreadsheet::cell::{Cell, CellType}; + + #[test] + fn materialized_stream_emits_every_chunk() { + let mut sheet = Sheet::new("file.xlsx", "Sheet1", None, None, false); + for row in 0..2049 { + sheet.push(Cell { + row, + col: 0, + kind: CellType::Number, + value: row.to_string(), + }); + } + sheet.finish(false); + + let mut batches = 0; + let mut rows = 0; + assert!(stream_materialized_sheets( + vec![sheet], + Arc::new(Vec::new()), + &mut |batch| { + batches += 1; + rows += batch.sheet.chunk(0).expect("batch table").len(); + true + }, + )); + assert_eq!(batches, 2); + assert_eq!(rows, 2049); + } +} diff --git a/src/spreadsheet/sheet.rs b/src/spreadsheet/sheet.rs index 4eeba60..6aeb133 100644 --- a/src/spreadsheet/sheet.rs +++ b/src/spreadsheet/sheet.rs @@ -196,6 +196,38 @@ impl Sheet { } } + /// Removes the next completed chunk while retaining cells that belong to + /// the chunk currently being assembled. + pub(super) fn take_ready_chunk(&mut self) -> Option { + let (row_lower, row_upper, index_lower, index_upper) = self.chunks.first().copied()?; + debug_assert_eq!(index_lower, 0); + + self.chunks.remove(0); + let remaining_cells = self.cells.split_off(index_upper); + let cells = std::mem::replace(&mut self.cells, remaining_cells); + self.chunk_index_lower -= index_upper; + for (_, _, lower, upper) in &mut self.chunks { + *lower -= index_upper; + *upper -= index_upper; + } + + Some(Self { + file_name: self.file_name.clone(), + name: self.name.clone(), + chunks: vec![(row_lower, row_upper, 0, cells.len())], + chunk_index_lower: cells.len(), + chunk_row_lower: Some(row_lower), + range: self.range, + limit: None, + skip_empty_rows: self.skip_empty_rows, + row_lower_bound: Some(row_lower), + row_upper_bound: Some(row_upper), + col_lower_bound: self.col_lower_bound, + col_upper_bound: self.col_upper_bound, + cells, + }) + } + /// Retrieves a chunk of data as a 2D table of optional cell references. /// Returns None if the chunk index is out of bounds. pub(crate) fn chunk(&self, index: usize) -> Option>>> { @@ -394,4 +426,26 @@ mod tests { assert_eq!(*index_lower, 0); assert_eq!(*index_upper, 5); } -} \ No newline at end of file + + #[test] + fn sheet_take_ready_chunk_releases_completed_cells() { + let mut sheet = Sheet::new("file.xlsx", "Sheet1", None, None, false); + push(&mut sheet, 0, 0); + push(&mut sheet, Sheet::CHUNK_SIZE, 0); + + let chunk = sheet.take_ready_chunk().expect("first chunk"); + assert_eq!(chunk.cells.len(), 1); + assert_eq!(chunk.chunks, vec![(0, Sheet::CHUNK_SIZE - 1, 0, 1)]); + assert_eq!(sheet.cells.len(), 1); + assert!(sheet.chunks.is_empty()); + + sheet.finish(false); + let chunk = sheet.take_ready_chunk().expect("final chunk"); + assert_eq!(chunk.cells.len(), 1); + assert_eq!( + chunk.chunks, + vec![(Sheet::CHUNK_SIZE, Sheet::CHUNK_SIZE, 0, 1)] + ); + assert!(sheet.cells.is_empty()); + } +} diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index ad809a9..c8cf4c2 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -6,8 +6,10 @@ use crate::helpers::xml::XmlReader; use crate::helpers::xml::XmlTextContextHelper; use crate::helpers::zip::ZipHelper; use crate::match_xml_events; +use crate::spreadsheet::SheetBatch; use crate::spreadsheet::Spreadsheet; use crate::spreadsheet::SpreadsheetError; +use crate::spreadsheet::stream_materialized_sheets; use crate::spreadsheet::cell::Cell; use crate::spreadsheet::cell::CellType; use crate::spreadsheet::criteria::Criteria; @@ -22,7 +24,14 @@ use quick_xml::name::QName; use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; +use std::fs::File; use std::io::BufReader; +use std::io::ErrorKind; +use std::io::Read; +use std::io::Seek; +use std::io::SeekFrom; +use std::io::Write; +use std::sync::Arc; use zip::ZipArchive; use zip::read::ZipFile; @@ -55,6 +64,67 @@ pub(crate) struct XlsxSpreadsheet { sheets: Vec<(String, String)>, } +struct SharedStringStore { + index: File, + values: File, +} + +impl SharedStringStore { + const RECORD_SIZE: u64 = 16; + const NULL_OFFSET: u64 = u64::MAX; + + fn load( + zip: &mut ZipArchive, + nulls: &HashSet, + ) -> Result { + let mut index = tempfile::tempfile()?; + let mut values = tempfile::tempfile()?; + let Some(mut reader) = zip.xml_reader("xl/sharedStrings.xml")? else { + return Ok(Self { index, values }); + }; + + match_xml_events!(reader => { + Event::Start(event) if event.name() == TAG_SHARED_STRING_ITEM => { + let value = read_string_value(&mut reader, TAG_SHARED_STRING_ITEM, false)?; + let (offset, length) = if nulls.contains(&value) { + (Self::NULL_OFFSET, 0) + } else { + let offset = values.stream_position()?; + values.write_all(value.as_bytes())?; + (offset, value.len() as u64) + }; + index.write_all(&offset.to_le_bytes())?; + index.write_all(&length.to_le_bytes())?; + } + }); + + Ok(Self { index, values }) + } + + fn get(&mut self, id: usize) -> Result, RustySheetError> { + let record_offset = (id as u64) + .checked_mul(Self::RECORD_SIZE) + .ok_or_else(|| std::io::Error::new(ErrorKind::InvalidData, "shared string index overflow"))?; + self.index.seek(SeekFrom::Start(record_offset))?; + let mut record = [0_u8; Self::RECORD_SIZE as usize]; + self.index.read_exact(&mut record)?; + + let offset = u64::from_le_bytes(record[..8].try_into().expect("shared string offset")); + if offset == Self::NULL_OFFSET { + return Ok(None); + } + let length = u64::from_le_bytes(record[8..].try_into().expect("shared string length")); + let length = usize::try_from(length) + .map_err(|_| std::io::Error::new(ErrorKind::InvalidData, "shared string is too large"))?; + let mut bytes = vec![0_u8; length]; + self.values.seek(SeekFrom::Start(offset))?; + self.values.read_exact(&mut bytes)?; + String::from_utf8(bytes) + .map(Some) + .map_err(|error| std::io::Error::new(ErrorKind::InvalidData, error).into()) + } +} + impl XlsxSpreadsheet { /// Opens an XLSX spreadsheet file and parses its structure /// @@ -72,6 +142,153 @@ impl XlsxSpreadsheet { sheets, }) } + + fn stream_xlsx_sheets( + &mut self, + criteria: &Criteria, + consumer: &mut dyn FnMut(SheetBatch) -> bool, + ) -> Result<(), RustySheetError> { + let mut shared_strings = SharedStringStore::load(&mut self.zip, &criteria.nulls)?; + let empty_shared_strings = Arc::new(Vec::new()); + let mut sheet_count = 0_usize; + + for (sheet_name, zip_path) in &self.sheets { + if criteria + .sheet_limit + .map(|limit| sheet_count >= limit) + .unwrap_or(false) + { + break; + } else if criteria.accept(sheet_name) { + sheet_count += 1; + } else { + continue; + } + + let mut sheet = Sheet::new( + &self.name, + sheet_name, + criteria.range, + criteria.rows_limit, + criteria.skip_empty_rows, + ); + let mut last_row = sheet.chunk_row_lower; + let mut has_data = false; + let mut row_count = 0_usize; + let mut col_count = 0_usize; + let mut row = 0_usize; + let mut col = 0_usize; + let mut kind = CellType::default(); + let mut value = String::new(); + let mut reader = self.zip.xml_reader(zip_path)?.expect(sheet_name); + + match_xml_events!(reader => { + Event::End(event) if event.name() == TAG_ROW => { + row_count += 1; + col_count = 0; + } + Event::Start(event) if event.name() == TAG_CELL => { + (row, col) = event.get_attribute_value("r")? + .and_then(|reference| reference_to_index(&reference)) + .unwrap_or((row_count, col_count)); + col_count += 1; + if sheet.after_row_upper_bound(row) { + break; + } + if sheet.contains(row, col) { + kind = event.get_attribute_value("t")?.map(|value| { + match value.as_ref() { + "inlineStr" | "str" => CellType::InlineString, + "s" => CellType::SharedString, + "d" => CellType::IsoDateTime, + "b" => CellType::Boolean, + "e" => if criteria.error_as_null { CellType::Empty } else { CellType::Error }, + _ => CellType::Number, + } + }).unwrap_or(CellType::Number); + if let Some(format_id) = event.get_attribute_value("s")? { + if kind == CellType::Number && !format_id.is_empty() { + kind = self.number_formats[format_id.parse::()?]; + } + } + } else { + kind = CellType::default(); + } + } + Event::Start(event) if kind != CellType::Empty && event.name() == TAG_INLINE_STRING => { + value = read_string_value(&mut reader, TAG_INLINE_STRING, false)?; + } + Event::Start(event) if kind != CellType::Empty && event.name() == TAG_VALUE => { + value = read_string_value(&mut reader, TAG_VALUE, true)?; + } + Event::End(event) if kind != CellType::Empty && event.name() == TAG_CELL => { + if kind == CellType::Error { + let reference = index_to_reference(row, col); + Err(SpreadsheetError::CellValueError( + sheet.file_name.clone(), + sheet.name.clone(), + reference, + value.clone(), + ))? + } + + let (kind, resolved_value) = if kind == CellType::SharedString { + ( + CellType::InlineString, + shared_strings.get(value.parse::()?)?, + ) + } else if criteria.nulls.contains(&value) { + (kind, None) + } else { + (kind, Some(value.clone())) + }; + + if let Some(resolved_value) = resolved_value { + if let Some(last_row) = last_row { + if criteria.end_at_empty_row + && ((!has_data && last_row != row) + || (has_data && last_row + 1 < row)) + { + break; + } + } + last_row = Some(row); + has_data = true; + sheet.push(Cell { + row, + col, + kind, + value: resolved_value, + }); + while let Some(chunk) = sheet.take_ready_chunk() { + if !consumer(SheetBatch { + sheet: chunk, + shared_strings: Arc::clone(&empty_shared_strings), + }) { + return Ok(()); + } + } + } + value.clear(); + } + Event::End(event) if event.name() == TAG_CELL => { + value.clear(); + }, + }); + + sheet.finish(criteria.end_at_empty_row); + while let Some(chunk) = sheet.take_ready_chunk() { + if !consumer(SheetBatch { + sheet: chunk, + shared_strings: Arc::clone(&empty_shared_strings), + }) { + return Ok(()); + } + } + } + + Ok(()) + } } impl Spreadsheet for XlsxSpreadsheet { @@ -369,6 +586,25 @@ impl Spreadsheet for XlsxSpreadsheet { Ok(sheets) } + + fn stream_sheets( + &mut self, + criteria: &Criteria, + consumer: &mut dyn FnMut(SheetBatch) -> bool, + ) -> Result<(), RustySheetError> { + if criteria.spread_merged_cells { + let shared_strings = Arc::new( + self.load_shared_strings(None)? + .0 + .into_iter() + .map(|value| (!criteria.nulls.contains(&value)).then_some(value)) + .collect(), + ); + stream_materialized_sheets(self.read_sheets(criteria)?, shared_strings, consumer); + return Ok(()); + } + self.stream_xlsx_sheets(criteria, consumer) + } } /// Loads workbook structure and worksheet information from XLSX file From 13fd6d6112a279fcc880a02c507345ff996e5567 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Fri, 7 Aug 2026 19:13:13 -0400 Subject: [PATCH 08/12] Stream legacy XLS scans with bounded memory --- Cargo.lock | 12 ++ Cargo.toml | 1 + src/error.rs | 21 -- src/helpers/biff8.rs | 344 ++++++++++++++++++++++++------ src/helpers/cfb.rs | 294 +++---------------------- src/helpers/string.rs | 16 -- src/spreadsheet/mod.rs | 1 + src/spreadsheet/shared_strings.rs | 124 +++++++++++ src/spreadsheet/xls.rs | 218 +++++++++++++++++-- src/spreadsheet/xlsx.rs | 90 ++------ 10 files changed, 648 insertions(+), 473 deletions(-) create mode 100644 src/spreadsheet/shared_strings.rs diff --git a/Cargo.lock b/Cargo.lock index a8e6c5f..cd504b4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -379,6 +379,17 @@ dependencies = [ "shlex", ] +[[package]] +name = "cfb" +version = "0.14.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a347dcabdae9c31b0825fd6a8bed285ec9c2acb89c47827126d52fa4f59cece3" +dependencies = [ + "fnv", + "uuid", + "web-time", +] + [[package]] name = "cfg-if" version = "1.0.4" @@ -1934,6 +1945,7 @@ name = "rusty_sheet" version = "0.4.2" dependencies = [ "anyhow", + "cfb", "chrono", "codepage", "duckdb", diff --git a/Cargo.toml b/Cargo.toml index 0dad3e3..3170a3d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,6 +22,7 @@ crate-type = ["staticlib"] [dependencies] anyhow = "1.0.100" chrono = { version = "0.4.42", features = ["serde"] } +cfb = "0.14.0" codepage = "0.1.2" duckdb = { version = "1.4.3", features = ["vtab-loadable"] } either = "1.15.0" diff --git a/src/error.rs b/src/error.rs index 9154b16..328466c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -46,9 +46,6 @@ pub(crate) enum RustySheetError { XmlAttributeError(#[from] quick_xml::events::attributes::AttrError), // Helper module errors - #[error("{0}")] - CfbHelperError(#[from] crate::helpers::cfb::CfbError), - #[error("{0}")] XmlHelperError(#[from] crate::helpers::xml::XmlError), @@ -83,24 +80,6 @@ pub(crate) enum RustySheetError { ExtensionError(#[from] crate::extension::ExtensionError), } -pub(crate) trait ResultOptionChain { - fn ok_none_else(self, f: F) -> Self - where - F: FnOnce() -> Self; -} - -impl ResultOptionChain for Result, E> { - fn ok_none_else(self, f: F) -> Self - where - F: FnOnce() -> Self, - { - match self { - Ok(None) => f(), - _ => self, - } - } -} - pub(crate) trait ResultMessage { fn with_prefix(self, message: &str) -> Self; } diff --git a/src/helpers/biff8.rs b/src/helpers/biff8.rs index 06a3cc9..ad71c6a 100644 --- a/src/helpers/biff8.rs +++ b/src/helpers/biff8.rs @@ -9,9 +9,15 @@ use crate::helpers::string::to_u32; use crate::helpers::string::to_u64; use crate::helpers::string::to_usize; use encoding_rs::Encoding; +use std::fs::File; +use std::io::BufReader; +use std::io::Read; +use std::io::Seek; +use std::io::SeekFrom; use thiserror::Error; const CONTINUE: u16 = 60; +const MAX_CACHED_RECORD_SIZE: usize = 64 * 1024; /// Errors specific to BIFF8 format parsing #[derive(Error, Debug)] @@ -24,47 +30,102 @@ pub(crate) enum Biff8Error { /// Handles the record-based structure with continuation records pub(crate) struct Biff8Reader { pub(crate) encoding: &'static Encoding, - buffer: Vec, - pointer: usize, // Next read position in buffer - chunks: Vec<(usize, usize)>, // Current record chunks (start, end) + file: File, + file_position: Option, + buffered_file: BufReader, + buffered_position: Option, + length: usize, + pointer: usize, // Next record position in the file + cache: Vec, + cache_chunks: Vec<(usize, usize)>, // Current cached record chunks (start, end) + cached: bool, + record_start: usize, + record_end: usize, + record_length: usize, + file_chunk: (usize, usize), index: usize, // Current chunk index offset: usize, // Offset within current chunk } impl Biff8Reader { - /// Creates a new BIFF8 reader with the given data buffer - pub(crate) fn new(data: Vec) -> Biff8Reader { - Biff8Reader { + /// Creates a new BIFF8 reader backed by a seekable temporary file. + pub(crate) fn new(file: File, length: usize) -> Result { + let buffered_file = BufReader::with_capacity(64 * 1024, file.try_clone()?); + Ok(Biff8Reader { encoding: &encoding_rs::UTF_16LE, - buffer: data, + file, + file_position: Some(0), + buffered_file, + buffered_position: None, + length, pointer: 0, - chunks: Vec::new(), + cache: Vec::new(), + cache_chunks: Vec::new(), + cached: false, + record_start: 0, + record_end: 0, + record_length: 0, + file_chunk: (0, 0), index: 0, offset: 0, - } + }) } /// Reads the next record type and prepares for reading record data /// Returns None when no more records are available pub(crate) fn next(&mut self) -> Result, RustySheetError> { - if self.pointer + 4 < self.buffer.len() { + if self.pointer + 4 < self.length { self.index = 0; self.offset = 0; + self.cache.clear(); + self.cache_chunks.clear(); - let kind = self.get_u16_at(self.pointer)?; - let size = self.get_u16_at(self.pointer + 2)? as usize; + let record_start = self.pointer; + let kind = self.read_file_u16_at(self.pointer)?; + let size = self.read_file_u16_at(self.pointer + 2)? as usize; let mut lower = self.pointer + 4; let mut upper = lower + size; self.pointer = upper; + let first_chunk = (lower, upper); + let mut record_length = size; + let mut cacheable = record_length <= MAX_CACHED_RECORD_SIZE; + if cacheable { + self.cache_chunks.push((lower, upper)); + } - self.chunks.clear(); - self.chunks.push((lower, upper)); - while self.pointer + 4 < self.buffer.len() && self.get_u16_at(self.pointer)? == CONTINUE { - let size = self.get_u16_at(self.pointer + 2)? as usize; + while self.pointer + 4 < self.length && self.read_file_u16_at(self.pointer)? == CONTINUE + { + let size = self.read_file_u16_at(self.pointer + 2)? as usize; lower = self.pointer + 4; upper = lower + size; self.pointer = upper; - self.chunks.push((lower, upper)); + record_length += size; + if cacheable && record_length <= MAX_CACHED_RECORD_SIZE { + self.cache_chunks.push((lower, upper)); + } else { + cacheable = false; + self.cache_chunks.clear(); + } + } + + self.cached = cacheable; + self.record_start = record_start; + self.record_end = upper; + self.record_length = record_length; + self.file_chunk = first_chunk; + + if self.cached { + self.cache.reserve(record_length); + for index in 0..self.cache_chunks.len() { + let (file_lower, file_upper) = self.cache_chunks[index]; + let cache_lower = self.cache.len(); + self.seek_file(file_lower)?; + self.cache.resize(cache_lower + file_upper - file_lower, 0); + self.file.read_exact(&mut self.cache[cache_lower..])?; + self.file_position = Some(file_upper); + let cache_upper = self.cache.len(); + self.cache_chunks[index] = (cache_lower, cache_upper); + } } Ok(Some(kind)) @@ -79,91 +140,124 @@ impl Biff8Reader { } /// Reads exactly `length` bytes, returning an error if insufficient data - fn read_extract(&mut self, length: usize) -> Result<&[u8], RustySheetError> { - let (data, size) = self.read(length); - if size == length { - Ok(data) - } else { - Err(Biff8Error::NoEnoughDataError(length))? + /// Reads up to `length` bytes from the current record + /// Returns the data slice and actual number of bytes read + fn read(&mut self, length: usize) -> Result<(Vec, usize), RustySheetError> { + if let Some((source, size)) = self.take_range(length)? { + return Ok((self.read_at(source, size)?, size)); } + Ok((Vec::new(), 0)) } - /// Reads up to `length` bytes from the current record - /// Returns the data slice and actual number of bytes read - fn read(&mut self, length: usize) -> (&[u8], usize) { - if let Some((lower, upper)) = self.chunks.get(self.index) { - let source = (*upper).min(*lower + self.offset); - let target = (*upper).min(source + length); - let size = target - source; - if source < *upper { - if target == *upper { - self.index += 1; - self.offset = 0; - } else { - self.offset += size; - } - return (&self.buffer[source..target], size); + fn take_range(&mut self, length: usize) -> Result, RustySheetError> { + let chunk = if self.cached { + self.cache_chunks.get(self.index).copied() + } else if self.file_chunk.0 < self.file_chunk.1 { + Some(self.file_chunk) + } else { + None + }; + + let Some((lower, upper)) = chunk else { + return Ok(None); + }; + let source = upper.min(lower + self.offset); + let target = upper.min(source + length); + let size = target - source; + if source >= upper { + return Ok(None); + } + + if target == upper { + self.offset = 0; + if self.cached { + self.index += 1; + } else if upper < self.record_end { + let mut header = [0_u8; 4]; + self.read_buffered_at(upper, &mut header)?; + let size = to_u16(&header[2..]) as usize; + self.file_chunk = (upper + 4, upper + 4 + size); + } else { + self.file_chunk = (upper, upper); } + } else { + self.offset += size; } - (&[], 0) + Ok(Some((source, size))) } - /// Skips `length` bytes and returns the skipped data - pub(crate) fn skip(&mut self, length: usize) -> Result<&[u8], RustySheetError> { - self.read_extract(length) + /// Skips `length` bytes in the current record. + pub(crate) fn skip(&mut self, length: usize) -> Result<(), RustySheetError> { + let size = self.take_range(length)?.map_or(0, |(_, size)| size); + if size == length { + Ok(()) + } else { + Err(Biff8Error::NoEnoughDataError(length))? + } } /// Reads a single byte pub(crate) fn read_u8(&mut self) -> Result { - self.read_extract(1).map(|data| data[0]) + self.read_array::<1>().map(|data| data[0]) } /// Reads a 16-bit unsigned integer pub(crate) fn read_u16(&mut self) -> Result { - self.read_extract(2).map(to_u16) + self.read_array::<2>().map(|data| to_u16(&data)) } /// Gets a 16-bit unsigned integer from the specified offset from the end - pub(crate) fn get_u16_back(&self, offset: usize) -> Result { - let mut offset = offset; - for (lower, upper) in self.chunks.iter().rev() { - if *lower + offset < *upper { - let index = *upper - offset; - return self.get_u16_at(index); - } else { - offset -= *upper - *lower; - } - } - Err(Biff8Error::NoEnoughDataError(2))? - } + pub(crate) fn get_u16_back(&mut self, offset: usize) -> Result { + let Some(mut logical_offset) = self.record_length.checked_sub(offset) else { + return Err(Biff8Error::NoEnoughDataError(2).into()); + }; - /// Gets a 16-bit unsigned integer from the specified absolute position - pub(crate) fn get_u16_at(&self, index: usize) -> Result { - if index + 2 <= self.buffer.len() { - Ok(to_u16(&self.buffer[index..index + 2])) + if self.cached { + for (lower, upper) in self.cache_chunks.iter().copied() { + let chunk_length = upper - lower; + if logical_offset < chunk_length && logical_offset + 2 <= chunk_length { + return Ok(to_u16( + &self.cache[lower + logical_offset..lower + logical_offset + 2], + )); + } + logical_offset = logical_offset.saturating_sub(chunk_length); + } } else { - Err(Biff8Error::NoEnoughDataError(2))? + let mut header = self.record_start; + loop { + let size = self.read_file_u16_at(header + 2)? as usize; + if logical_offset < size && logical_offset + 2 <= size { + return self.read_file_u16_at(header + 4 + logical_offset); + } + logical_offset = logical_offset.saturating_sub(size); + let next_header = header + 4 + size; + if next_header >= self.record_end { + break; + } + header = next_header; + } } + Err(Biff8Error::NoEnoughDataError(2).into()) } /// Reads a 32-bit unsigned integer pub(crate) fn read_u32(&mut self) -> Result { - self.read_extract(4).map(to_u32) + self.read_array::<4>().map(|data| to_u32(&data)) } /// Reads a usize value pub(crate) fn read_usize(&mut self) -> Result { - self.read_extract(4).map(to_usize) + self.read_array::<4>().map(|data| to_usize(&data)) } /// Reads a 64-bit unsigned integer pub(crate) fn read_u64(&mut self) -> Result { - self.read_extract(8).map(to_u64) + self.read_array::<8>().map(|data| to_u64(&data)) } /// Reads a 64-bit floating point number pub(crate) fn read_f64(&mut self) -> Result { - self.read_extract(8).map(to_f64) + self.read_array::<8>().map(|data| to_f64(&data)) } /// Reads an RK number (compressed numeric format used in Excel) @@ -206,7 +300,9 @@ impl Biff8Reader { } /// Reads a rich extended Unicode string with formatting information - pub(crate) fn read_xl_unicode_rich_extended_string(&mut self) -> Result { + pub(crate) fn read_xl_unicode_rich_extended_string( + &mut self, + ) -> Result { let mut string = String::new(); let mut expected = self.read_u16()? as usize; let mut actual = self.read_string_into(expected, true, &mut string)?; @@ -219,24 +315,31 @@ impl Biff8Reader { /// Reads string data into the provided content buffer /// Handles rich text formatting and phonetic information - fn read_string_into(&mut self, chars: usize, is_extend: bool, content: &mut String) -> Result { + fn read_string_into( + &mut self, + chars: usize, + is_extend: bool, + content: &mut String, + ) -> Result { let encoding = self.encoding; let flag = self.read_u8()?; let is_high_byte = (flag & 0x1) > 0; let expected = Self::chars_to_bytes(is_high_byte, chars); - let rich_string_count = if is_extend && (flag & 0x8) > 0 { // is_rich_string + let rich_string_count = if is_extend && (flag & 0x8) > 0 { + // is_rich_string self.read_u16()? as usize } else { 0 }; - let phonetic_count = if is_extend && (flag & 0x4) > 0 { // contains_phonetic + let phonetic_count = if is_extend && (flag & 0x4) > 0 { + // contains_phonetic self.read_usize()? } else { 0 }; - let (bytes, actual) = self.read(expected); + let (bytes, actual) = self.read(expected)?; if is_high_byte { - let (string, _, _) = encoding.decode(bytes); + let (string, _, _) = encoding.decode(&bytes); content.push_str(&string); } else { let u16s = bytes.iter().map(|byte| *byte as u16).collect::>(); @@ -250,6 +353,62 @@ impl Biff8Reader { Ok(Self::bytes_to_chars(is_high_byte, actual)) } + fn read_array(&mut self) -> Result<[u8; N], RustySheetError> { + let Some((source, size)) = self.take_range(N)? else { + return Err(Biff8Error::NoEnoughDataError(N).into()); + }; + if size != N { + return Err(Biff8Error::NoEnoughDataError(N).into()); + } + + let mut bytes = [0_u8; N]; + if self.cached { + bytes.copy_from_slice(&self.cache[source..source + N]); + } else { + self.read_buffered_at(source, &mut bytes)?; + } + Ok(bytes) + } + + fn read_at(&mut self, offset: usize, length: usize) -> Result, RustySheetError> { + if self.cached { + return Ok(self.cache[offset..offset + length].to_vec()); + } + let mut bytes = vec![0_u8; length]; + self.read_buffered_at(offset, &mut bytes)?; + Ok(bytes) + } + + fn read_file_u16_at(&mut self, offset: usize) -> Result { + if offset + 2 > self.length { + return Err(Biff8Error::NoEnoughDataError(2).into()); + } + self.seek_file(offset)?; + let mut bytes = [0_u8; 2]; + self.file.read_exact(&mut bytes)?; + self.file_position = Some(offset + bytes.len()); + Ok(to_u16(&bytes)) + } + + fn seek_file(&mut self, offset: usize) -> Result<(), RustySheetError> { + self.buffered_position = None; + if self.file_position != Some(offset) { + self.file.seek(SeekFrom::Start(offset as u64))?; + self.file_position = Some(offset); + } + Ok(()) + } + + fn read_buffered_at(&mut self, offset: usize, bytes: &mut [u8]) -> Result<(), RustySheetError> { + self.file_position = None; + if self.buffered_position != Some(offset) { + self.buffered_file.seek(SeekFrom::Start(offset as u64))?; + } + self.buffered_file.read_exact(bytes)?; + self.buffered_position = Some(offset + bytes.len()); + Ok(()) + } + /// Converts character count to byte count based on encoding #[inline] fn chars_to_bytes(is_high_byte: bool, chars: usize) -> usize { @@ -263,6 +422,49 @@ impl Biff8Reader { } } +#[cfg(test)] +mod tests { + use super::*; + use std::io::Write; + + #[test] + fn small_records_are_cached() { + let mut file = tempfile::tempfile().unwrap(); + file.write_all(&1_u16.to_le_bytes()).unwrap(); + file.write_all(&4_u16.to_le_bytes()).unwrap(); + file.write_all(&42_u32.to_le_bytes()).unwrap(); + let length = file.stream_position().unwrap() as usize; + file.seek(SeekFrom::Start(0)).unwrap(); + + let mut reader = Biff8Reader::new(file, length).unwrap(); + assert_eq!(reader.next().unwrap(), Some(1)); + assert!(reader.cached); + assert_eq!(reader.read_u32().unwrap(), 42); + } + + #[test] + fn large_continued_records_remain_file_backed() { + let mut file = tempfile::tempfile().unwrap(); + for index in 0..9_u16 { + let kind = if index == 0 { 1 } else { CONTINUE }; + file.write_all(&kind.to_le_bytes()).unwrap(); + file.write_all(&8192_u16.to_le_bytes()).unwrap(); + file.write_all(&vec![index as u8; 8192]).unwrap(); + } + let length = file.stream_position().unwrap() as usize; + file.seek(SeekFrom::Start(0)).unwrap(); + + let mut reader = Biff8Reader::new(file, length).unwrap(); + assert_eq!(reader.next().unwrap(), Some(1)); + assert!(!reader.cached); + assert!(reader.cache.is_empty()); + assert!(reader.cache_chunks.is_empty()); + assert_eq!(reader.read_u8().unwrap(), 0); + reader.skip(8191).unwrap(); + assert_eq!(reader.read_u8().unwrap(), 1); + } +} + #[macro_export] macro_rules! match_biff8_record { ($reader:expr => { $($arms:tt)* }) => { diff --git a/src/helpers/cfb.rs b/src/helpers/cfb.rs index f4bb69a..76b9488 100644 --- a/src/helpers/cfb.rs +++ b/src/helpers/cfb.rs @@ -1,284 +1,38 @@ -//! OLE Compound File Binary (CFB) reader for legacy Excel (.xls) format -//! Implements parsing of the Compound File Binary format used in older Office documents +//! Bounded reader for Microsoft Compound File Binary (CFB) containers. use crate::error::RustySheetError; -use crate::helpers::string::to_u16; -use crate::helpers::string::to_u64; -use crate::helpers::string::to_usize; -use crate::helpers::string::to_usize_iter; -use encoding_rs::UTF_16LE; -use std::collections::HashMap; use std::io::Read; use std::io::Seek; -use std::io::SeekFrom; -use std::ops::Range; -use thiserror::Error; +use std::io::Write; -// Sector type constants (commented out as they're not currently used) -// const FREE_SECT: usize = 0xFFFFFFFF; -// const END_OF_CHAIN: usize = 0xFFFFFFFE; -// const FAT_SECT: usize = 0xFFFFFFFD; -// const DIF_SECT: usize = 0xFFFFFFFC; -const MAX_REG_SECT: usize = 0xFFFFFFFB; - -/// Errors specific to Compound File Binary format parsing -#[derive(Error, Debug)] -pub(crate) enum CfbError { - #[error("The file is corrupted or has an invalid CFB structure")] - FileFormatError, - - #[error("Invalid OLE signature (not an office document?)")] - OleSignatureError, - - #[error("Invalid Sector size '2 ^ {1}' for major version '{0}'")] - SectorSizeError(u16, u16), - - #[error("The number of double indirect file allocation table error: expect '{0}', actual '{1}'")] - DoubleIndirectFileAllocationTableError(usize, usize), - - #[error("The number of file allocation table error: expect '{0}', actual '{1}'")] - FileAllocationTableError(usize, usize), - - #[error("Empty Root directory")] - RootDirectoryError, -} - -/// Compound File Binary structure representing the entire OLE file -/// Contains directory entries, file allocation tables, and sector data -pub(crate) struct Cfb { - /// Directory index mapping names to directory entries - directories: HashMap, - /// File allocation table for regular sectors - file_allocation_table: Vec, - /// Regular sectors containing file data - sectors: Sectors, - /// Mini file allocation table for small files - mini_file_allocation_table: Vec, - /// Mini sectors for small files (64-byte sectors) - mini_sectors: Sectors, +pub(crate) struct Cfb { + inner: cfb::CompoundFile, } -impl Cfb { - /// Creates a new CFB structure by reading and parsing the entire file - pub(crate) fn new(reader: &mut RS) -> Result { - let size = reader.seek(SeekFrom::End(0))?; - if size < 512 { - Err(CfbError::FileFormatError)?; - } - reader.seek(SeekFrom::Start(0))?; - let mut header_bytes = [0u8; 512]; - reader.read_exact(&mut header_bytes)?; - let header = Header::new(&header_bytes)?; - let sector_size = header.sector_size()?; - - // Load the complete file only after its header identifies it as CFB. - let mut data: Vec = vec![0u8; size as usize]; - data[..512].copy_from_slice(&header_bytes); - reader.read_exact(&mut data[512..])?; - // Parse the data - let sectors = Sectors { data, size: sector_size }; - let file_allocation_table = Self::load_file_allocation_table(§ors, &header)?; - let directories = Self::load_directories(&file_allocation_table, §ors, header.directory_shift)?; - let mini_file_allocation_table = Self::load_mini_file_allocation_table(&file_allocation_table, §ors, &header)?; - let mini_sectors= if directories.contains_key("Root Entry") { - Self::load_mini_file_allocation_sectors(&file_allocation_table, §ors, &directories["Root Entry"])? - } else { - Sectors { data: Vec::new(), size: 64 } - }; - - Ok(Cfb { - directories, - file_allocation_table, - sectors, - mini_file_allocation_table, - mini_sectors, - }) +impl Cfb { + pub(crate) fn new(reader: F) -> Result { + let inner = cfb::OpenOptions::new() + .max_buffer_size(64 * 1024) + .open_with(reader)?; + Ok(Self { inner }) } - /// Checks if a file exists in the CFB structure pub(crate) fn exists(&self, name: &str) -> bool { - self.directories.contains_key(name) - } - - /// Reads the contents of a file from the CFB structure - pub(crate) fn read(&self, name: &str) -> Result>, RustySheetError> { - if let Some(directory) = self.directories.get(name) { - let mut bytes = if directory.count < 4096 { - Self::read_bytes(&self.mini_file_allocation_table, &self.mini_sectors, directory.index)? - } else { - Self::read_bytes(&self.file_allocation_table, &self.sectors, directory.index)? - }; - bytes.truncate(directory.count); - Ok(Some(bytes)) - } else { - Ok(None) - } - } - - /// Loads the file allocation table using the double indirect file allocation table - fn load_file_allocation_table(sectors: &Sectors, header: &Header) -> Result, RustySheetError> { - let mut double_indirect_file_allocation_table = Vec::::new(); - double_indirect_file_allocation_table.extend(to_usize_iter(sectors.slice(76..512))); - - let mut count = 0usize; - let mut index = header.double_indirect_file_allocation_table_shift; - while index < MAX_REG_SECT { - double_indirect_file_allocation_table.extend(to_usize_iter(sectors.get(index))); - index = double_indirect_file_allocation_table.pop().expect("Next Sector ID"); - count += 1; - } - if count != header.double_indirect_file_allocation_table_count { - Err(CfbError::DoubleIndirectFileAllocationTableError(header.double_indirect_file_allocation_table_count, count))? - } - - let mut file_allocation_table: Vec = Vec::new(); - let mut count = 0usize; - for index in double_indirect_file_allocation_table { - if index < MAX_REG_SECT { - file_allocation_table.extend(to_usize_iter(sectors.get(index))); - count += 1; - } - } - if count != header.file_allocation_table_count { - Err(CfbError::FileAllocationTableError(header.file_allocation_table_count, count))? - } - - Ok(file_allocation_table) - } - - /// Loads directory entries from the specified sector index - fn load_directories(file_allocation_table: &Vec, sectors: &Sectors, index: usize) -> Result, RustySheetError> { - let bytes = Self::read_bytes(&file_allocation_table, §ors, index)?; - let directories: HashMap = bytes.chunks(128).map(Directory::new).collect(); - if directories.is_empty() { - Err(CfbError::RootDirectoryError)? - } - Ok(directories) - } - - /// Loads the mini file allocation table for small files - fn load_mini_file_allocation_table(file_allocation_table: &Vec, sectors: &Sectors, header: &Header) -> Result, RustySheetError> { - Ok(if header.mini_file_allocation_table_sector_count > 0 { - let mini_file_allocation_table = Self::read_bytes(file_allocation_table, sectors, header.mini_file_allocation_table_sector_shift)?; - to_usize_iter(&mini_file_allocation_table).collect() - } else { - Vec::new() - }) - } - - /// Loads mini file allocation sectors for small files - fn load_mini_file_allocation_sectors(file_allocation_table: &Vec, sectors: &Sectors, mini: &Directory) -> Result { - let mut data = Self::read_bytes(file_allocation_table, sectors, mini.index)?; - data.truncate(mini.count); - Ok(Sectors { data, size: 64 }) // Mini sector size is fixed at 64 bytes + self.inner.exists(format!("/{name}")) } - /// Reads the complete content of a file by following the file allocation table chain - fn read_bytes(file_allocation_table: &Vec, sectors: &Sectors, index: usize) -> Result, RustySheetError> { - let mut content: Vec = Vec::new(); - let mut index = index; - while index < MAX_REG_SECT { - content.extend(sectors.get(index)); - index = file_allocation_table[index]; + pub(crate) fn copy_to( + &mut self, + name: &str, + writer: &mut W, + ) -> Result { + let path = format!("/{name}"); + if !self.inner.is_stream(&path) { + return Ok(false); } - Ok(content) - } -} - -/// Container for all sectors in the CFB file -#[derive(Debug)] -struct Sectors { - data: Vec, - // Size of individual sectors - size: usize, -} - -impl Sectors { - /// Gets the data for the sector at the specified index - fn get(&self, index: usize) -> &[u8] { - let source = (index + 1) * self.size; - let target = self.data.len().min((index + 2) * self.size); - &self.data[source..target] - } - - /// Gets a slice of data from the specified range - fn slice(&self, range: Range) -> &[u8] { - &self.data[range] - } -} - -/// CFB file header structure -#[derive(Debug)] -struct Header { - signature: u64, - major_version: u16, - sector_shift: u16, - file_allocation_table_count: usize, - directory_shift: usize, - mini_file_allocation_table_sector_shift: usize, - mini_file_allocation_table_sector_count: usize, - double_indirect_file_allocation_table_shift: usize, - double_indirect_file_allocation_table_count: usize, -} - -impl Header { - /// Parses the CFB header from the first 512 bytes of data - fn new(data: &[u8]) -> Result { - let header = Header { - signature: to_u64(&data[0..8]), - major_version: to_u16(&data[26..28]), - sector_shift: to_u16(&data[30..32]), - file_allocation_table_count: to_usize(&data[44..48]), - directory_shift: to_usize(&data[48..52]), - mini_file_allocation_table_sector_shift: to_usize(&data[60..64]), - mini_file_allocation_table_sector_count: to_usize(&data[64..68]), - double_indirect_file_allocation_table_shift: to_usize(&data[68..72]), - double_indirect_file_allocation_table_count: to_usize(&data[72..76]), - }; - - if header.signature != 0xE11A_B1A1_E011_CFD0 { - Err(CfbError::OleSignatureError)?; - } - - Ok(header) - } - - /// Calculates the sector size based on major version and sector shift - fn sector_size(&self) -> Result { - if self.major_version == 3 && self.sector_shift == 0x0009 { - Ok(512) // 2 ^ 9 - } else if self.major_version == 4 && self.sector_shift == 0x000C { - Ok(4096) // 2 ^ 12 - // For version 4 compound files, - // the header size (512 bytes) is less than the sector size (4,096 bytes), - // so the remaining part of the header (3,584 bytes) MUST be filled with all zeroes. - } else { - Err(CfbError::SectorSizeError(self.major_version, self.sector_shift))? - } - } -} - -/// Directory entry representing a file in the CFB structure -#[derive(Debug)] -struct Directory { - index: usize, - count: usize, -} - -impl Directory { - /// Creates a directory entry from raw bytes - fn new(bytes: &[u8]) -> (String, Directory) { - let size = to_u16(&bytes[64..66]) as usize; - let (name, _, _) = UTF_16LE.decode(&bytes[..size]); - let name = if let Some(position) = name.find('\0') { - name[..position].to_owned() - } else { - name.to_string() - }; - - let index = to_usize(&bytes[116..120]); - let count = to_u64(&bytes[120..128]) as usize; - (name, Directory { index, count }) + let mut stream = self.inner.open_stream(path)?; + std::io::copy(&mut stream, writer)?; + Ok(true) } } @@ -288,10 +42,10 @@ mod tests { use std::io::Cursor; #[test] - fn non_cfb_input_stops_after_header() { + fn non_cfb_input_stops_after_signature() { let mut reader = Cursor::new(vec![0_u8; 4096]); assert!(Cfb::new(&mut reader).is_err()); - assert_eq!(reader.stream_position().unwrap(), 512); + assert_eq!(reader.stream_position().unwrap(), 8); } } diff --git a/src/helpers/string.rs b/src/helpers/string.rs index e6a83f6..3d5c4ff 100644 --- a/src/helpers/string.rs +++ b/src/helpers/string.rs @@ -1,22 +1,6 @@ //! Binary data conversion utilities for reading various numeric types from byte slices. //! Provides efficient little-endian conversion functions optimized for spreadsheet parsing. -use std::usize; - -/// Converts a byte slice into an iterator of 32-bit unsigned integers. -/// Processes bytes in 4-byte chunks using little-endian byte order. -pub(crate) fn to_u32_iter(bytes: &[u8]) -> impl ExactSizeIterator + '_ { - bytes.chunks(4) - .map(|chunk| chunk.try_into().expect("[u8; 4]")) - .map(u32::from_le_bytes) -} - -/// Converts a byte slice into an iterator of usize values. -/// First converts to u32, then safely converts to usize for the current platform. -pub(crate) fn to_usize_iter(bytes: &[u8]) -> impl ExactSizeIterator + '_ { - to_u32_iter(bytes).map(|value| value.try_into().expect("usize")) -} - /// Converts the first 8 bytes of a slice to a 64-bit floating point number. /// Uses little-endian byte order for conversion. #[inline] diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index df30488..c5d3cdf 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -27,6 +27,7 @@ pub(crate) mod xlsb; pub(crate) mod xlsx; pub(crate) mod criteria; pub(crate) mod sheet; +pub(crate) mod shared_strings; pub(crate) fn resolve_number_format( number_formats: &[CellType], diff --git a/src/spreadsheet/shared_strings.rs b/src/spreadsheet/shared_strings.rs new file mode 100644 index 0000000..3aa61bb --- /dev/null +++ b/src/spreadsheet/shared_strings.rs @@ -0,0 +1,124 @@ +use crate::error::RustySheetError; +use std::fs::File; +use std::io::BufReader; +use std::io::BufWriter; +use std::io::ErrorKind; +use std::io::Read; +use std::io::Seek; +use std::io::SeekFrom; +use std::io::Write; + +pub(crate) struct SharedStringStore { + index: BufWriter, + values: BufWriter, + values_length: u64, + count: usize, + reader: Option, +} + +struct SharedStringReader { + index: BufReader, + values: BufReader, + index_position: u64, + values_position: u64, +} + +impl SharedStringStore { + const RECORD_SIZE: u64 = 16; + + pub(crate) fn new() -> Result { + Ok(Self { + index: BufWriter::with_capacity(64 * 1024, tempfile::tempfile()?), + values: BufWriter::with_capacity(64 * 1024, tempfile::tempfile()?), + values_length: 0, + count: 0, + reader: None, + }) + } + + pub(crate) fn push(&mut self, value: &str) -> Result<(), RustySheetError> { + if self.reader.is_some() { + Err(std::io::Error::new( + ErrorKind::InvalidData, + "cannot append after reading shared strings", + ))?; + } + let offset = self.values_length; + self.values.write_all(value.as_bytes())?; + self.index.write_all(&offset.to_le_bytes())?; + self.index.write_all(&(value.len() as u64).to_le_bytes())?; + self.values_length += value.len() as u64; + self.count += 1; + Ok(()) + } + + pub(crate) fn get(&mut self, id: usize) -> Result { + if id >= self.count { + Err(std::io::Error::new( + ErrorKind::InvalidData, + "shared string index out of bounds", + ))?; + } + let record_offset = (id as u64).checked_mul(Self::RECORD_SIZE).ok_or_else(|| { + std::io::Error::new(ErrorKind::InvalidData, "shared string index overflow") + })?; + if self.reader.is_none() { + self.index.flush()?; + self.values.flush()?; + self.reader = Some(SharedStringReader { + index: BufReader::with_capacity(64 * 1024, self.index.get_ref().try_clone()?), + values: BufReader::with_capacity(64 * 1024, self.values.get_ref().try_clone()?), + index_position: self.count as u64 * Self::RECORD_SIZE, + values_position: self.values_length, + }); + } + let reader = self + .reader + .as_mut() + .expect("shared string reader initialized"); + if reader.index_position != record_offset { + reader.index.seek(SeekFrom::Start(record_offset))?; + reader.index_position = record_offset; + } + let mut record = [0_u8; Self::RECORD_SIZE as usize]; + reader.index.read_exact(&mut record)?; + reader.index_position += Self::RECORD_SIZE; + + let offset = u64::from_le_bytes(record[..8].try_into().expect("shared string offset")); + let length = u64::from_le_bytes(record[8..].try_into().expect("shared string length")); + let length = usize::try_from(length).map_err(|_| { + std::io::Error::new(ErrorKind::InvalidData, "shared string is too large") + })?; + let mut bytes = vec![0_u8; length]; + if reader.values_position != offset { + reader.values.seek(SeekFrom::Start(offset))?; + reader.values_position = offset; + } + reader.values.read_exact(&mut bytes)?; + reader.values_position += length as u64; + String::from_utf8(bytes) + .map_err(|error| std::io::Error::new(ErrorKind::InvalidData, error).into()) + } + + pub(crate) fn len(&self) -> usize { + self.count + } +} + +#[cfg(test)] +mod tests { + use super::SharedStringStore; + + #[test] + fn shared_string_store_reads_sequential_and_random_values() { + let mut store = SharedStringStore::new().unwrap(); + store.push("alpha").unwrap(); + store.push("bravo").unwrap(); + store.push("charlie").unwrap(); + + assert_eq!(store.get(0).unwrap(), "alpha"); + assert_eq!(store.get(1).unwrap(), "bravo"); + assert_eq!(store.get(0).unwrap(), "alpha"); + assert_eq!(store.get(2).unwrap(), "charlie"); + } +} diff --git a/src/spreadsheet/xls.rs b/src/spreadsheet/xls.rs index 02119c4..7e9581d 100644 --- a/src/spreadsheet/xls.rs +++ b/src/spreadsheet/xls.rs @@ -1,4 +1,3 @@ -use crate::error::ResultOptionChain; use crate::error::RustySheetError; use crate::helpers::biff8::Biff8Reader; use crate::helpers::cfb::Cfb; @@ -12,11 +11,16 @@ use crate::spreadsheet::excel::load_number_formats; use crate::spreadsheet::reference::index_to_reference; use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; +use crate::spreadsheet::shared_strings::SharedStringStore; +use crate::spreadsheet::SheetBatch; use crate::spreadsheet::Spreadsheet; use crate::spreadsheet::SpreadsheetError; use either::Either; use std::collections::HashMap; use std::collections::HashSet; +use std::io::Seek; +use std::io::SeekFrom; +use std::sync::Arc; use thiserror::Error; // BIFF8 record type identifiers for Excel file parsing @@ -56,8 +60,8 @@ pub(crate) struct XlsSpreadsheet { pub(crate) name: String, /// BIFF8 reader for parsing Excel binary format records reader: Biff8Reader, - /// Shared string table containing repeated text values - shared_strings: Vec, + /// Disk-backed shared string table containing repeated text values + shared_strings: SharedStringStore, /// Number format mappings for cell type detection number_formats: Vec, /// List of worksheets with their names and stream positions @@ -74,14 +78,18 @@ impl XlsSpreadsheet { /// * `Result` - Initialized spreadsheet or error pub(crate) fn open(file_name: &str) -> Result { // Open file from local path or remote URL - let mut reader = UnifiedReader::new(file_name)?; - let cfb = Cfb::new(&mut reader)?; - let mut reader = cfb.read("Workbook") - .ok_none_else(|| cfb.read("Book"))? - .map(Biff8Reader::new) - .ok_or_else(|| SpreadsheetError::SpreadsheetEmptyError(file_name.to_owned()))?; + let reader = UnifiedReader::new(file_name)?; + let mut cfb = Cfb::new(reader)?; + let mut workbook = tempfile::tempfile()?; + if !cfb.copy_to("Workbook", &mut workbook)? && !cfb.copy_to("Book", &mut workbook)? { + Err(SpreadsheetError::SpreadsheetEmptyError(file_name.to_owned()))?; + } + let length = usize::try_from(workbook.stream_position()?) + .map_err(|_| std::io::Error::other("XLS workbook is too large"))?; + workbook.seek(SeekFrom::Start(0))?; + let mut reader = Biff8Reader::new(workbook, length)?; let mut is_1904 = false; - let mut shared_strings = Vec::new(); + let mut shared_strings = SharedStringStore::new()?; let mut custom_formats: HashMap = HashMap::new(); let mut format_indexes: Vec = Vec::new(); let mut sheets: Vec<(String, usize)> = Vec::new(); @@ -106,7 +114,7 @@ impl XlsSpreadsheet { let id = reader.read_u16()?; format_indexes.push(id.to_string()); } - SST => shared_strings = load_shared_strings(&mut reader)?, + SST => load_shared_strings(&mut reader, &mut shared_strings)?, BOUND_SHEET8 => { let pointer = reader.read_usize()?; reader.skip(2)?; @@ -128,6 +136,160 @@ impl XlsSpreadsheet { sheets, }) } + + fn stream_xls_sheets( + &mut self, + criteria: &Criteria, + consumer: &mut dyn FnMut(SheetBatch) -> bool, + ) -> Result<(), RustySheetError> { + let empty_shared_strings = Arc::new(Vec::new()); + let mut sheet_count = 0_usize; + for (sheet_name, pointer) in &self.sheets { + if criteria + .sheet_limit + .map(|limit| sheet_count >= limit) + .unwrap_or(false) + { + break; + } else if criteria.accept(sheet_name) { + sheet_count += 1; + } else { + continue; + } + + self.reader.goto(*pointer); + self.reader.next()?; + let mut sheet = Sheet::new( + &self.name, + sheet_name, + criteria.range, + criteria.rows_limit, + criteria.skip_empty_rows, + ); + let mut last_row = sheet.chunk_row_lower; + let mut has_data = false; + while let Some(tag) = self.reader.next()? { + match tag { + BOF | EOF => break, + MUL_RK => { + let row = self.reader.read_u16()? as usize; + let col_lower_bound = self.reader.read_u16()? as usize; + let col_upper_bound = self.reader.get_u16_back(2)? as usize; + for col in col_lower_bound..=col_upper_bound { + if sheet.contains(row, col) { + if let Some(last_row) = last_row { + if criteria.end_at_empty_row + && ((!has_data && last_row != row) + || (has_data && last_row + 1 < row)) + { + break; + } + } + last_row = Some(row); + let index = self.reader.read_u16()? as usize; + let kind = self.number_formats[index]; + let value = self.reader.read_rk_number()?; + if !criteria.nulls.contains(&value) { + has_data = true; + sheet.push(Cell { + row, + col, + kind, + value, + }); + while let Some(chunk) = sheet.take_ready_chunk() { + if !consumer(SheetBatch { + sheet: chunk, + shared_strings: Arc::clone(&empty_shared_strings), + }) { + return Ok(()); + } + } + } + } else { + self.reader.skip(6)?; + } + } + } + BOOL_ERR | NUMBER | RK | LABEL_SST | LABEL | FORMULA => { + let row = self.reader.read_u16()? as usize; + let col = self.reader.read_u16()? as usize; + if sheet.contains(row, col) { + if let Some(last_row) = last_row { + if criteria.end_at_empty_row + && ((!has_data && last_row != row) + || (has_data && last_row + 1 < row)) + { + break; + } + } + last_row = Some(row); + let (either, value) = match tag { + BOOL_ERR => read_bool_or_error_cell(&mut self.reader)?, + NUMBER => read_number_cell(&mut self.reader)?, + RK => read_rk_cell(&mut self.reader)?, + LABEL_SST => read_label_sst_cell(&mut self.reader)?, + LABEL => read_label_cell(&mut self.reader)?, + _ => read_formula_cell(&mut self.reader)?, + }; + let kind = match either { + Either::Left(kind) => kind, + Either::Right(index) => self.number_formats[index], + }; + if kind == CellType::Error { + if !criteria.error_as_null { + let reference = index_to_reference(row, col); + Err(SpreadsheetError::CellValueError( + sheet.file_name.clone(), + sheet.name.clone(), + reference, + value, + ))?; + } + continue; + } + let (kind, value) = if kind == CellType::SharedString { + ( + CellType::InlineString, + self.shared_strings.get(value.parse::()?)?, + ) + } else { + (kind, value) + }; + if !criteria.nulls.contains(&value) { + has_data = true; + sheet.push(Cell { + row, + col, + kind, + value, + }); + while let Some(chunk) = sheet.take_ready_chunk() { + if !consumer(SheetBatch { + sheet: chunk, + shared_strings: Arc::clone(&empty_shared_strings), + }) { + return Ok(()); + } + } + } + } + } + _ => (), + } + } + sheet.finish(criteria.end_at_empty_row); + while let Some(chunk) = sheet.take_ready_chunk() { + if !consumer(SheetBatch { + sheet: chunk, + shared_strings: Arc::clone(&empty_shared_strings), + }) { + return Ok(()); + } + } + } + Ok(()) + } } impl Spreadsheet for XlsSpreadsheet { @@ -138,8 +300,7 @@ impl Spreadsheet for XlsSpreadsheet { /// Loads shared strings with optional index filtering /// - /// XLS files are typically small enough to load all shared strings at once - /// and maintain them in memory for efficient access during parsing. + /// Selected values are loaded from the disk-backed shared string table. /// /// # Arguments /// * `indexes` - Optional set of string indexes to filter by @@ -150,12 +311,13 @@ impl Spreadsheet for XlsSpreadsheet { &mut self, indexes: Option>, ) -> Result<(Vec, HashMap), RustySheetError> { - let shared_strings = self.shared_strings.to_owned(); + let indexes = indexes.unwrap_or_else(|| (0..self.shared_strings.len()).collect()); + let mut shared_strings = Vec::with_capacity(indexes.len()); let mut mappings = HashMap::::new(); - if let Some(keys) = indexes { - for key in keys { - mappings.insert(key, key); - } + for id in indexes { + let index = shared_strings.len(); + shared_strings.push(self.shared_strings.get(id)?); + mappings.insert(id, index); } Ok((shared_strings, mappings)) } @@ -280,6 +442,14 @@ impl Spreadsheet for XlsSpreadsheet { Ok(sheets) } + + fn stream_sheets( + &mut self, + criteria: &Criteria, + consumer: &mut dyn FnMut(SheetBatch) -> bool, + ) -> Result<(), RustySheetError> { + self.stream_xls_sheets(criteria, consumer) + } } /// Loads the shared string table from BIFF8 SST record @@ -291,16 +461,18 @@ impl Spreadsheet for XlsSpreadsheet { /// * `reader` - BIFF8 reader positioned at SST record /// /// # Returns -/// * `Result>` - Vector of shared string values -fn load_shared_strings(reader: &mut Biff8Reader) -> Result, RustySheetError> { - let mut shared_strings: Vec = Vec::new(); +/// * `Result<()>` - Values are appended to the disk-backed store +fn load_shared_strings( + reader: &mut Biff8Reader, + shared_strings: &mut SharedStringStore, +) -> Result<(), RustySheetError> { reader.skip(4)?; let count = reader.read_usize()?; for _ in 0..count { let string = reader.read_xl_unicode_rich_extended_string()?; - shared_strings.push(string); + shared_strings.push(&string)?; } - Ok(shared_strings) + Ok(()) } /// Reads a BOOL_ERR record containing boolean or error cell values diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index c8cf4c2..32edd8d 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -19,18 +19,13 @@ use crate::spreadsheet::reference::index_to_reference; use crate::spreadsheet::reference::reference_to_index; use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; +use crate::spreadsheet::shared_strings::SharedStringStore; use quick_xml::events::Event; use quick_xml::name::QName; use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; -use std::fs::File; use std::io::BufReader; -use std::io::ErrorKind; -use std::io::Read; -use std::io::Seek; -use std::io::SeekFrom; -use std::io::Write; use std::sync::Arc; use zip::ZipArchive; use zip::read::ZipFile; @@ -64,67 +59,6 @@ pub(crate) struct XlsxSpreadsheet { sheets: Vec<(String, String)>, } -struct SharedStringStore { - index: File, - values: File, -} - -impl SharedStringStore { - const RECORD_SIZE: u64 = 16; - const NULL_OFFSET: u64 = u64::MAX; - - fn load( - zip: &mut ZipArchive, - nulls: &HashSet, - ) -> Result { - let mut index = tempfile::tempfile()?; - let mut values = tempfile::tempfile()?; - let Some(mut reader) = zip.xml_reader("xl/sharedStrings.xml")? else { - return Ok(Self { index, values }); - }; - - match_xml_events!(reader => { - Event::Start(event) if event.name() == TAG_SHARED_STRING_ITEM => { - let value = read_string_value(&mut reader, TAG_SHARED_STRING_ITEM, false)?; - let (offset, length) = if nulls.contains(&value) { - (Self::NULL_OFFSET, 0) - } else { - let offset = values.stream_position()?; - values.write_all(value.as_bytes())?; - (offset, value.len() as u64) - }; - index.write_all(&offset.to_le_bytes())?; - index.write_all(&length.to_le_bytes())?; - } - }); - - Ok(Self { index, values }) - } - - fn get(&mut self, id: usize) -> Result, RustySheetError> { - let record_offset = (id as u64) - .checked_mul(Self::RECORD_SIZE) - .ok_or_else(|| std::io::Error::new(ErrorKind::InvalidData, "shared string index overflow"))?; - self.index.seek(SeekFrom::Start(record_offset))?; - let mut record = [0_u8; Self::RECORD_SIZE as usize]; - self.index.read_exact(&mut record)?; - - let offset = u64::from_le_bytes(record[..8].try_into().expect("shared string offset")); - if offset == Self::NULL_OFFSET { - return Ok(None); - } - let length = u64::from_le_bytes(record[8..].try_into().expect("shared string length")); - let length = usize::try_from(length) - .map_err(|_| std::io::Error::new(ErrorKind::InvalidData, "shared string is too large"))?; - let mut bytes = vec![0_u8; length]; - self.values.seek(SeekFrom::Start(offset))?; - self.values.read_exact(&mut bytes)?; - String::from_utf8(bytes) - .map(Some) - .map_err(|error| std::io::Error::new(ErrorKind::InvalidData, error).into()) - } -} - impl XlsxSpreadsheet { /// Opens an XLSX spreadsheet file and parses its structure /// @@ -143,12 +77,26 @@ impl XlsxSpreadsheet { }) } + fn load_shared_string_store(&mut self) -> Result { + let mut store = SharedStringStore::new()?; + let Some(mut reader) = self.zip.xml_reader("xl/sharedStrings.xml")? else { + return Ok(store); + }; + match_xml_events!(reader => { + Event::Start(event) if event.name() == TAG_SHARED_STRING_ITEM => { + let value = read_string_value(&mut reader, TAG_SHARED_STRING_ITEM, false)?; + store.push(&value)?; + } + }); + Ok(store) + } + fn stream_xlsx_sheets( &mut self, criteria: &Criteria, consumer: &mut dyn FnMut(SheetBatch) -> bool, ) -> Result<(), RustySheetError> { - let mut shared_strings = SharedStringStore::load(&mut self.zip, &criteria.nulls)?; + let mut shared_strings = self.load_shared_string_store()?; let empty_shared_strings = Arc::new(Vec::new()); let mut sheet_count = 0_usize; @@ -233,10 +181,8 @@ impl XlsxSpreadsheet { } let (kind, resolved_value) = if kind == CellType::SharedString { - ( - CellType::InlineString, - shared_strings.get(value.parse::()?)?, - ) + let value = shared_strings.get(value.parse::()?)?; + (CellType::InlineString, (!criteria.nulls.contains(&value)).then_some(value)) } else if criteria.nulls.contains(&value) { (kind, None) } else { From 859f035ee626391d8c32f1e0b7822b96ff3dc86a Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Fri, 7 Aug 2026 19:31:46 -0400 Subject: [PATCH 09/12] Preserve XLS shared string order --- src/spreadsheet/xls.rs | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/spreadsheet/xls.rs b/src/spreadsheet/xls.rs index 7e9581d..663d01d 100644 --- a/src/spreadsheet/xls.rs +++ b/src/spreadsheet/xls.rs @@ -311,7 +311,13 @@ impl Spreadsheet for XlsSpreadsheet { &mut self, indexes: Option>, ) -> Result<(Vec, HashMap), RustySheetError> { - let indexes = indexes.unwrap_or_else(|| (0..self.shared_strings.len()).collect()); + let Some(indexes) = indexes else { + let mut shared_strings = Vec::with_capacity(self.shared_strings.len()); + for id in 0..self.shared_strings.len() { + shared_strings.push(self.shared_strings.get(id)?); + } + return Ok((shared_strings, HashMap::new())); + }; let mut shared_strings = Vec::with_capacity(indexes.len()); let mut mappings = HashMap::::new(); for id in indexes { @@ -610,3 +616,30 @@ fn read_formula_cell( Err(XlsError::FormulaValueError(formula))? } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn load_all_shared_strings_preserves_index_order() { + let file = tempfile::tempfile().unwrap(); + let reader = Biff8Reader::new(file, 0).unwrap(); + let mut shared_strings = SharedStringStore::new().unwrap(); + shared_strings.push("alpha").unwrap(); + shared_strings.push("bravo").unwrap(); + shared_strings.push("charlie").unwrap(); + let mut spreadsheet = XlsSpreadsheet { + name: String::new(), + reader, + shared_strings, + number_formats: Vec::new(), + sheets: Vec::new(), + }; + + let (values, mappings) = spreadsheet.load_shared_strings(None).unwrap(); + + assert_eq!(values, ["alpha", "bravo", "charlie"]); + assert!(mappings.is_empty()); + } +} From 313cf4dbb5bad7dc7a1b8aa147dea224ee4f3200 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Fri, 7 Aug 2026 19:53:34 -0400 Subject: [PATCH 10/12] Report read_sheet worker panics --- src/extension/read_sheet.rs | 76 ++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/src/extension/read_sheet.rs b/src/extension/read_sheet.rs index f8d73d7..fc356e9 100644 --- a/src/extension/read_sheet.rs +++ b/src/extension/read_sheet.rs @@ -32,12 +32,16 @@ use duckdb::vtab::InitInfo; use duckdb::vtab::TableFunctionInfo; use duckdb::vtab::VTab; use glob::Pattern; +use std::any::Any; use std::collections::HashSet; use std::error::Error; use std::sync::mpsc::sync_channel; use std::sync::mpsc::Receiver; +use std::sync::mpsc::SyncSender; use std::sync::Mutex; +type StreamResult = Result; + /// Parameters for reading a single sheet from a spreadsheet file. struct ReadSheetParameters { /// Path to the spreadsheet file @@ -192,11 +196,40 @@ impl TryFrom<&ReadSheetParameters> for ReadSheetBindData { /// This tracks the current processing state and column projections. pub(crate) struct ReadSheetInitData { /// Bounded queue fed by the spreadsheet parser. - receiver: Mutex>>, + receiver: Mutex>, /// Column indices that should be projected (output) from the source data projections: Vec, } +fn run_stream_worker(sender: SyncSender, stream: F) +where + F: FnOnce(&SyncSender) -> Result<(), RustySheetError>, +{ + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| stream(&sender))); + match result { + Ok(Ok(())) => (), + Ok(Err(error)) => { + let _ = sender.send(Err(error.to_string())); + } + Err(payload) => { + let _ = sender.send(Err(format!( + "spreadsheet stream panicked: {}", + panic_payload_message(payload.as_ref()) + ))); + } + } +} + +fn panic_payload_message(payload: &(dyn Any + Send)) -> String { + if let Some(message) = payload.downcast_ref::<&'static str>() { + (*message).to_string() + } else if let Some(message) = payload.downcast_ref::() { + message.clone() + } else { + "unknown panic".to_string() + } +} + /// Main table function implementation for reading single sheets from spreadsheets. /// This implements the DuckDB VTab trait to provide SQL table function capabilities. pub(crate) struct ReadSheetTableFunction; @@ -230,12 +263,11 @@ impl VTab for ReadSheetTableFunction { .collect::>(); let (sender, receiver) = sync_channel(2); std::thread::spawn(move || { - let result = open_spreadsheet(&file_name).and_then(|mut spreadsheet| { - spreadsheet.stream_sheets(&criteria, &mut |batch| sender.send(Ok(batch)).is_ok()) + run_stream_worker(sender, |sender| { + open_spreadsheet(&file_name).and_then(|mut spreadsheet| { + spreadsheet.stream_sheets(&criteria, &mut |batch| sender.send(Ok(batch)).is_ok()) + }) }); - if let Err(error) = result { - let _ = sender.send(Err(error.to_string())); - } }); Ok(ReadSheetInitData { receiver: Mutex::new(receiver), @@ -321,3 +353,35 @@ impl VTab for ReadSheetTableFunction { ]) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn stream_worker_sends_parser_errors() { + let (sender, receiver) = sync_channel(1); + + run_stream_worker(sender, |_| { + Err(RustySheetError::WithContextError("bad workbook".to_string())) + }); + + assert_eq!(receive_error(receiver), "bad workbook"); + } + + #[test] + fn stream_worker_sends_panic_errors() { + let (sender, receiver) = sync_channel(1); + + run_stream_worker(sender, |_| panic!("bad workbook")); + + assert_eq!(receive_error(receiver), "spreadsheet stream panicked: bad workbook"); + } + + fn receive_error(receiver: Receiver) -> String { + match receiver.recv().unwrap() { + Ok(_) => panic!("expected stream error"), + Err(error) => error, + } + } +} From f2524a129d373072de1659171f7d7973aad3fb46 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 13:05:06 -0400 Subject: [PATCH 11/12] Use checked workbook indexes while streaming --- src/spreadsheet/xls.rs | 18 +++++++++++-- src/spreadsheet/xlsx.rs | 59 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/src/spreadsheet/xls.rs b/src/spreadsheet/xls.rs index 663d01d..7f9026b 100644 --- a/src/spreadsheet/xls.rs +++ b/src/spreadsheet/xls.rs @@ -187,7 +187,14 @@ impl XlsSpreadsheet { } last_row = Some(row); let index = self.reader.read_u16()? as usize; - let kind = self.number_formats[index]; + let kind = resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?; let value = self.reader.read_rk_number()?; if !criteria.nulls.contains(&value) { has_data = true; @@ -234,7 +241,14 @@ impl XlsSpreadsheet { }; let kind = match either { Either::Left(kind) => kind, - Either::Right(index) => self.number_formats[index], + Either::Right(index) => resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?, }; if kind == CellType::Error { if !criteria.error_as_null { diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index 32edd8d..141f26f 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -136,9 +136,17 @@ impl XlsxSpreadsheet { col_count = 0; } Event::Start(event) if event.name() == TAG_CELL => { - (row, col) = event.get_attribute_value("r")? - .and_then(|reference| reference_to_index(&reference)) - .unwrap_or((row_count, col_count)); + if let Some(reference) = event.get_attribute_value("r")? { + (row, col) = reference_to_index(&reference).ok_or_else(|| { + SpreadsheetError::CellReferenceError( + sheet.file_name.to_owned(), + sheet.name.to_owned(), + reference.to_string(), + ) + })?; + } else { + (row, col) = (row_count, col_count); + } col_count += 1; if sheet.after_row_upper_bound(row) { break; @@ -156,7 +164,15 @@ impl XlsxSpreadsheet { }).unwrap_or(CellType::Number); if let Some(format_id) = event.get_attribute_value("s")? { if kind == CellType::Number && !format_id.is_empty() { - kind = self.number_formats[format_id.parse::()?]; + let index = format_id.parse::()?; + kind = resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?; } } } else { @@ -758,6 +774,24 @@ mod tests { assert!(error.contains("workbook defines 1 styles")); } + #[test] + fn stream_invalid_style_index_returns_error() { + let path = invalid_workbook_path("stream-style"); + write_invalid_style_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.stream_sheets(&default_criteria(), &mut |_| true); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid style index error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A11")); + assert!(error.contains("invalid style index 999")); + assert!(error.contains("workbook defines 1 styles")); + } + #[test] fn invalid_shared_string_index_returns_error() { let path = invalid_workbook_path("shared-string"); @@ -792,6 +826,23 @@ mod tests { assert!(error.contains("invalid cell reference 'XFE1'")); } + #[test] + fn stream_invalid_cell_reference_returns_error() { + let path = invalid_workbook_path("stream-cell-reference"); + write_invalid_cell_reference_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.stream_sheets(&default_criteria(), &mut |_| true); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid cell reference error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1")); + assert!(error.contains("invalid cell reference 'XFE1'")); + } + #[test] fn invalid_date_header_value_returns_error() { let path = invalid_workbook_path("date-header"); From 666cddddaba760c08be154815dbfde8f986fa258 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 13:37:22 -0400 Subject: [PATCH 12/12] Use required worksheet XML while streaming --- src/spreadsheet/xlsx.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index 141f26f..58b126a 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -128,7 +128,7 @@ impl XlsxSpreadsheet { let mut col = 0_usize; let mut kind = CellType::default(); let mut value = String::new(); - let mut reader = self.zip.xml_reader(zip_path)?.expect(sheet_name); + let mut reader = required_xml_reader(&mut self.zip, zip_path)?; match_xml_events!(reader => { Event::End(event) if event.name() == TAG_ROW => { @@ -877,6 +877,23 @@ mod tests { assert!(error.contains("missing or corrupted")); } + #[test] + fn missing_sheet_xml_returns_error_while_streaming() { + let path = invalid_workbook_path("missing-sheet-streaming"); + write_missing_sheet_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.stream_sheets(&default_criteria(), &mut |_| true); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected missing sheet XML error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("xl/worksheets/sheet1.xml")); + assert!(error.contains("missing or corrupted")); + } + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH)