diff --git a/crates/ruff_charon/src/store.rs b/crates/ruff_charon/src/store.rs index e3e8f80710440..757c1ea1b2f92 100644 --- a/crates/ruff_charon/src/store.rs +++ b/crates/ruff_charon/src/store.rs @@ -363,6 +363,13 @@ impl Store { Self::replace(&self.path(crate_name, "rows.bin"), &bin)?; if self.tsv_export { Self::replace(&self.path(crate_name, "rows.tsv"), &out)?; + } else { + // A TSV left by an earlier import would describe the previous + // revision while `rows.bin` describes this one. + match fs::remove_file(self.path(crate_name, "rows.tsv")) { + Err(e) if e.kind() != io::ErrorKind::NotFound => return Err(e), + _ => {} + } } Self::replace(&self.path(crate_name, "residuals.tsv"), &res)?; Self::replace(&self.path(crate_name, "files.tsv"), &files)?; diff --git a/crates/ruff_charon/src/store_bin.rs b/crates/ruff_charon/src/store_bin.rs index 7a4f22059d937..c1d914eb45f9a 100644 --- a/crates/ruff_charon/src/store_bin.rs +++ b/crates/ruff_charon/src/store_bin.rs @@ -248,6 +248,12 @@ pub enum WriteError { /// The offending index. value: u32, }, + /// The laid-out file is too large for its u32 section offsets and + /// lengths (a section ends past 4 GiB). + FileTooLarge { + /// The byte position that does not fit. + value: usize, + }, } /// Why [`read`] refused a file; one variant per refusal. @@ -328,6 +334,9 @@ impl fmt::Display for WriteError { write!(f, "{section} is too large to address ({len})") } Self::NoProv => write!(f, "no provenance record"), + Self::FileTooLarge { value } => { + write!(f, "section position {value} does not fit a u32") + } Self::FunDeclTooLarge { value } => { write!( f, @@ -699,24 +708,34 @@ pub fn write(rows: &[BinRow], partial: bool, prov: &[Prov]) -> Result, W data: prov_section(prov)?, }); - Ok(assemble( + assemble( [n_rows, n_symbols, n_files], u32::from(partial) * FLAG_PARTIAL, sections, - )) + ) } /// Lay out header, directory and sections (each starting on a multiple of 8). -fn assemble(counts: [u32; 3], flags: u32, mut sections: Vec
) -> Vec { +/// Every offset, length and the section count must fit their u32 directory +/// fields; a value that does not is an error, never a saturated `u32::MAX` +/// that would publish a file the reader then refuses. +fn assemble( + counts: [u32; 3], + flags: u32, + mut sections: Vec
, +) -> Result, WriteError> { sections.sort_by_key(|s| s.tag); - let n_sections = u32::try_from(sections.len()).unwrap_or(u32::MAX); + let n_sections = dir_field(sections.len())?; let mut dir = Vec::with_capacity(sections.len() * DIR_ENTRY_LEN); let mut body: Vec = Vec::new(); let base = HEADER_LEN + sections.len() * DIR_ENTRY_LEN; for s in §ions { pad8(&mut body); - let offset = u32::try_from(base + body.len()).unwrap_or(u32::MAX); - let len = u32::try_from(s.data.len()).unwrap_or(u32::MAX); + let offset = dir_field(base + body.len())?; + let len = dir_field(s.data.len())?; + // The section's end must be addressable too: a reader checks + // `offset + len` against the file length. + dir_field(base + body.len() + s.data.len())?; dir.extend_from_slice(&s.tag.to_le_bytes()); dir.extend_from_slice(&s.width.to_le_bytes()); dir.extend_from_slice(&s.enc.to_le_bytes()); @@ -740,7 +759,12 @@ fn assemble(counts: [u32; 3], flags: u32, mut sections: Vec
) -> Vec file.extend_from_slice(&body); let d = digest(&file[32..]); file[16..24].copy_from_slice(&d.to_le_bytes()); - file + Ok(file) +} + +/// A directory field: a section count, offset or length as a u32. +fn dir_field(value: usize) -> Result { + u32::try_from(value).map_err(|_| WriteError::FileTooLarge { value }) } // --------------------------------------------------------------------------- @@ -1223,3 +1247,20 @@ pub fn read(bytes: &[u8]) -> Result { digest: dig, }) } + +#[cfg(test)] +mod tests { + use super::{WriteError, dir_field}; + + /// Red if a directory field saturates instead of failing: the last + /// addressable byte converts, the first one past 4 GiB is refused. + #[test] + fn a_directory_field_past_u32_is_an_error() { + let max = usize::try_from(u32::MAX).expect("u32 fits usize on supported targets"); + assert_eq!(dir_field(max), Ok(u32::MAX)); + assert_eq!( + dir_field(max + 1), + Err(WriteError::FileTooLarge { value: max + 1 }) + ); + } +} diff --git a/crates/ruff_charon/tests/store_bin_store.rs b/crates/ruff_charon/tests/store_bin_store.rs index 25fb9f1f18152..bf3ffc10c6bb0 100644 --- a/crates/ruff_charon/tests/store_bin_store.rs +++ b/crates/ruff_charon/tests/store_bin_store.rs @@ -83,6 +83,33 @@ fn tsv_export_off_writes_no_tsv() { assert!(!store.rows("cg_lib").expect("rows").is_empty()); } +/// Red if turning the export off leaves an older `rows.tsv` behind: it would +/// describe the previous revision while `rows.bin` describes the new one. +#[test] +fn turning_the_export_off_removes_a_stale_tsv() { + let dir = temp("tsv_stale"); + Store::new(&dir) + .import(&llbc("cg_lib.llbc"), "cg_lib") + .expect("import rev1"); + assert!(dir.join("cg_lib.rows.tsv").exists(), "rev1 wrote a TSV"); + let store = Store::new(&dir).with_tsv_export(false); + store + .import(&llbc("cg_lib_rev2.llbc"), "cg_lib") + .expect("import rev2"); + assert!( + !dir.join("cg_lib.rows.tsv").exists(), + "the rev1 TSV survived a rev2 import with export off" + ); + let rev2 = store.rows("cg_lib").expect("rows"); + let rev1 = Store::new(temp("tsv_stale_ref")); + rev1.import(&llbc("cg_lib.llbc"), "cg_lib").expect("ref"); + assert_ne!( + rev2, + rev1.rows("cg_lib").expect("ref rows"), + "the two revisions must differ, or this test proves nothing" + ); +} + /// Red when a function row loses its `FunDeclId`, the CRATE row gains one, or /// a DEFINES row names a different function than the one it was harvested from. #[test]