diff --git a/Cargo.lock b/Cargo.lock index de93390b2..8f4f2b13c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3237,9 +3237,9 @@ dependencies = [ [[package]] name = "zlob" -version = "1.6.3" +version = "1.6.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f0f010d9b10495cc0037cb47cedc143c39c7fa2d59f752dca19036fd0234a210" +checksum = "7ed9cfbcfaf165adcf07d994f85a92effaef03029b53afc0872152671efcfbda" dependencies = [ "bindgen", "bitflags 2.11.0", diff --git a/Cargo.toml b/Cargo.toml index 99854c592..261e9b5ac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,7 +36,7 @@ ignore = "0.4.22" memmap2 = "0.9" mimalloc = "0.1.47" signal-hook-registry = "1.4" -zlob = { version = "=1.6.3" } +zlob = { version = "=1.6.5" } mlua = { version = "0.11.1", features = ["module", "luajit"] } neo_frizbee = { version = "0.11.0", features = ["match_end_col"] } diff --git a/crates/fff-core/src/file_picker.rs b/crates/fff-core/src/file_picker.rs index bbb62096b..346504074 100644 --- a/crates/fff-core/src/file_picker.rs +++ b/crates/fff-core/src/file_picker.rs @@ -506,9 +506,19 @@ impl FileItem { let is_binary = is_known_binary_extension(path); // SAFETY-ish: paths on macOS/Linux are bytes; lossy conversion mirrors // the existing `to_string_lossy()` behavior on non-UTF8 names. - let rel_str = String::from_utf8_lossy(relative_path).into_owned(); + let decoded = String::from_utf8_lossy(relative_path).into_owned(); + // The caller's offset indexes the raw bytes; re-measure it against the + // decoded string so it never lands inside a U+FFFD (#799). + let dir_bytes = relative_path + .get(..basename_offset as usize) + .unwrap_or(relative_path); + let basename_offset = if std::str::from_utf8(dir_bytes).is_ok() { + basename_offset + } else { + String::from_utf8_lossy(dir_bytes).len() as u16 + }; let item = Self::new_raw(basename_offset, size, modified, git_status, is_binary); - (item, rel_str) + (item, decoded) } pub(crate) fn update_frecency_scores( diff --git a/crates/fff-core/src/walk/zlob.rs b/crates/fff-core/src/walk/zlob.rs index 9dc2f3504..98eddf1fc 100644 --- a/crates/fff-core/src/walk/zlob.rs +++ b/crates/fff-core/src/walk/zlob.rs @@ -54,19 +54,17 @@ pub(crate) fn walk_collect_files( if !entry.is_file() { // unlike ripgrep walker zlob doesnt show .git files if entry.is_dir() { - let rel_bytes = entry.relative_path_bytes(); - if !rel_bytes.is_empty() { - let mut rel = String::from_utf8_lossy(rel_bytes).into_owned(); - rel.push('/'); - collected.lock().1.push(rel); + let relative_path = entry.relative_path_lossy(); + if !relative_path.is_empty() { + let mut relative_path = relative_path.into_owned(); + relative_path.push('/'); + collected.lock().1.push(relative_path); } } return WalkState::Continue; } - let rel_bytes = entry.relative_path_bytes(); - // `basename()` returns `&str` for files only. let basename = entry.basename().unwrap_or(""); let is_binary = is_known_binary_extension_basename(basename); @@ -78,14 +76,16 @@ pub(crate) fn walk_collect_files( .map(|ns| (ns / 1_000_000_000).max(0) as u64) .unwrap_or(0); - let basename_offset = entry.basename_offset_in_relative(); + // Lossy pair: the offset must index the decoded string, not the raw + // bytes, or it lands inside a U+FFFD on invalid-UTF-8 names (#799). + let basename_offset = entry.basename_offset_in_relative_lossy() as u16; // zlob emits '/'-separated relative paths, which is fff's canonical // internal form on every platform — store them verbatim. - let rel_str = String::from_utf8_lossy(rel_bytes).into_owned(); + let relative_path = entry.relative_path_lossy().into_owned(); let item = FileItem::new_raw(basename_offset, size, modified, None, is_binary); let mut guard = collected.lock(); - guard.0.push((item, rel_str)); + guard.0.push((item, relative_path)); let n = guard.0.len(); drop(guard); diff --git a/crates/fff-core/tests/invalid_utf8_paths.rs b/crates/fff-core/tests/invalid_utf8_paths.rs new file mode 100644 index 000000000..d758cc6d3 --- /dev/null +++ b/crates/fff-core/tests/invalid_utf8_paths.rs @@ -0,0 +1,261 @@ +// Regression test for https://github.com/dmtrKovalenko/fff/issues/799 +// Unix-only: Windows filenames are UTF-16 and cannot carry these bytes. +#![cfg(unix)] + +use std::path::{Path, PathBuf}; +use tempfile::TempDir; + +use fff_search::file_picker::FilePicker; +use fff_search::{FilePickerOptions, FuzzySearchOptions, PaginationArgs, QueryParser}; + +/// Byte-exact corpus from the reporter's tarball (fff-invalid-utf8-repro.tar.gz, +/// sha256 fc0011038d27bbd63a3ca67a929d4f342b4895f922062fe86bd958712c67de08): +/// four nested dirs plus one empty file, names carrying invalid UTF-8, +/// control bytes and truncated multibyte sequences. +const REPRO_COMPONENTS: &[(&str, bool)] = &[ + // (hex-encoded name, is_dir) + ("64617461322e6361622e657874726163746564", true), + ( + "2366711cf02a03386b398b0b95824b120cd90135e777c6356ab77ead67ed0be428e3c5a7c49084cd62468cef4f780b46a97ff5e2be8afefd0962a9cfae50f861a2f637fcfe62f44c32aa96392ce1873f392ce18785905893a6c8f7b9b80b39f9cacc576a22f42758c9bd1e3071bbf041ee629cf0bbd09771b2", + true, + ), + ( + "33516e5b0214f6bdf7056a22dd6886262defcb33811eb9d4bf84b321422cd4b2beb87e5db8c4aedf7fae5e45a5c5fae7e9ba39b1547f8cc3778738f9a94391b3875f20c4cb0a1b4b163938f39558e66ab0e81d0bcacd4139fd62cbf3bcfd9142a29ab511ad9c7766390abad8143428261982ba754133515066a9edf0d34c13e33e35858f65eee4adb6144fdb11546fc442574121adb24191fb95ae6ee3c40a9d787cde8cf1941c8ca7e4f903f2a5b5eaee4b5fe5b18ad62b3a48dc67431d94e0d6503b8e7bf48cd7c3be9c8569598e6f23db3222", + true, + ), + ( + "6248697c3afe892703961a9d668d9ac88f557d357768c5e665ac9abc505920a44c06a56fff", + true, + ), + ( + "07fa04794121fad9f17a52a373a636bec7d08d0fa68e1e7a61ce8751bbf68d06ab74fe07206dde4cadb44730f93b49f2987148fe1f28093980f5df7fcbac6b46d9837e31e97e1bae9f581a665209e43f783f06a65666d1853a9f36dea3b56aa3dcfdf7c08ccdbd7423b460de92c7157f506c1f6057fab76abb2bdae6056ccadc49dac6f5ee1e8af2c499f60dc350e3df6a5b9f38135b5dbd60c182e099af1d1f6d3c9ba26dda282e77ec59bde5afcf3e1204f7f8fd8d67bb6bdb37b65cee783f888f8454cf5d967b239ee8cd956dc3e616", + false, + ), +]; + +/// Hand-made corpus covering the shapes the tarball doesn't: invalid bytes in +/// the leading component, an invalid byte right before the separator, a name +/// that is invalid only in its basename, and a valid-UTF-8 multibyte dir whose +/// offset must stay untouched. Entries are '/'-joined hex components. +const EXTRA_CASES: &[(&str, bool)] = &[ + // "\xff\xffx/file.txt" — the minimal case from the triage report + ("ffff78/66696c652e747874", false), + // "a\xffb\xff/plain.md" — invalid byte immediately before the separator + ("61ff62ff/706c61696e2e6d64", false), + // valid dir, invalid basename: "ok/\xc3(bad" + ("6f6b/c328626164", false), + // "каталог/main.rs" — valid multibyte, offset must equal the raw one + ("d0bad0b0d182d0b0d0bbd0bed0b3/6d61696e2e7273", false), +]; + +#[test] +fn scans_invalid_utf8_corpus_without_panicking() { + let tmp = TempDir::new().unwrap(); + let Some(expected) = build_chain(tmp.path(), REPRO_COMPONENTS) else { + eprintln!("filesystem rejects invalid UTF-8 names; skipping"); + return; + }; + + // Pre-fix this aborts the process from the background scan thread. + let picker = scan(tmp.path()); + let indexed = all_paths(&picker); + + assert!( + indexed.contains(&expected), + "the reporter's file must be indexed under its lossy path" + ); + assert_offsets_are_char_boundaries(&picker); + assert_dirs_indexed(&picker, &expected); +} + +#[test] +fn scans_mixed_encoding_corpus_without_panicking() { + let tmp = TempDir::new().unwrap(); + let Some(()) = build_paths(tmp.path(), EXTRA_CASES) else { + eprintln!("filesystem rejects invalid UTF-8 names; skipping"); + return; + }; + + let picker = scan(tmp.path()); + let indexed = all_paths(&picker); + + assert!( + indexed + .iter() + .any(|p| p.ends_with("\u{FFFD}\u{FFFD}x/file.txt")), + "expected the \\xff\\xffx/file.txt case, got {indexed:?}" + ); + // A valid-UTF-8 multibyte dir must keep the raw offset verbatim. + let cyrillic = indexed + .iter() + .find(|p| p.contains("каталог")) + .expect("cyrillic path must be indexed"); + assert!(cyrillic.ends_with("каталог/main.rs"), "got {cyrillic:?}"); + assert_offsets_are_char_boundaries(&picker); +} + +/// Searching by a plain ASCII component of a path whose other components are +/// invalid UTF-8 must still work end to end. +#[test] +fn searches_across_invalid_utf8_parents() { + let tmp = TempDir::new().unwrap(); + if build_chain(tmp.path(), REPRO_COMPONENTS).is_none() { + eprintln!("filesystem rejects invalid UTF-8 names; skipping"); + return; + } + + let picker = scan(tmp.path()); + let parser = QueryParser::default(); + let parsed = parser.parse("data2.cab.extracted"); + let result = picker.fuzzy_search( + &parsed, + None, + FuzzySearchOptions { + max_threads: 1, + pagination: PaginationArgs { + offset: 0, + limit: 50, + }, + ..Default::default() + }, + ); + + assert_eq!(result.items.len(), 1, "the single corpus file must match"); + assert!( + result.items[0] + .relative_path(&picker) + .starts_with("data2.cab.extracted/") + ); +} + +fn scan(base: &Path) -> FilePicker { + let mut picker = FilePicker::new(FilePickerOptions { + base_path: base.to_string_lossy().into_owned(), + enable_mmap_cache: false, + watch: false, + ..Default::default() + }) + .expect("FilePicker::new"); + picker.collect_files().expect("collect_files"); + picker +} + +fn all_paths(picker: &FilePicker) -> Vec { + picker + .get_files() + .iter() + .map(|f| f.relative_path(picker)) + .collect() +} + +/// The exact invariant #799 violated: the stored basename offset must be a +/// char boundary of the decoded path and must point right past a separator. +fn assert_offsets_are_char_boundaries(picker: &FilePicker) { + for file in picker.get_files().iter() { + let path = file.relative_path(picker); + let offset = file.filename_offset_in_relative_path(); + assert!( + path.is_char_boundary(offset), + "offset {offset} is not a char boundary of {path:?}" + ); + assert!(offset <= path.len(), "offset {offset} past end of {path:?}"); + if offset > 0 { + assert_eq!( + &path[offset - 1..offset], + "/", + "offset must follow a separator in {path:?}" + ); + } + assert!( + !path[offset..].contains('/'), + "basename split wrong: {path:?}" + ); + } +} + +/// Every ancestor of the corpus file must land in the searchable dir table. +fn assert_dirs_indexed(picker: &FilePicker, file_path: &str) { + let dirs = picker + .get_dirs() + .iter() + .map(|d| d.relative_path(picker)) + .collect::>(); + + let mut prefix = String::new(); + let components: Vec<&str> = file_path.split('/').collect(); + for component in &components[..components.len() - 1] { + prefix.push_str(component); + prefix.push('/'); + assert!( + dirs.iter() + .any(|d| d.trim_end_matches('/') == prefix.trim_end_matches('/')), + "missing dir {prefix:?} in {dirs:?}" + ); + } +} + +/// Materializes the tarball chain under `root`: each entry nests inside the +/// previous one. Returns the '/'-joined lossy path of the leaf file, or `None` +/// when the filesystem refuses invalid UTF-8 names (APFS returns EILSEQ, ZFS +/// with utf8only likewise) so the test skips instead of failing. +fn build_chain(root: &Path, components: &[(&str, bool)]) -> Option { + let mut path = PathBuf::from(root); + let mut decoded = String::new(); + + for (hex, is_dir) in components { + let bytes = unhex(hex); + path.push(os_str(&bytes)); + if !decoded.is_empty() { + decoded.push('/'); + } + decoded.push_str(&String::from_utf8_lossy(&bytes)); + + if !create(&path, *is_dir) { + return None; + } + } + + Some(decoded) +} + +/// Materializes independent '/'-joined hex paths under `root`. +fn build_paths(root: &Path, specs: &[(&str, bool)]) -> Option<()> { + for (hex, is_dir) in specs { + let mut path = PathBuf::from(root); + let components: Vec> = hex.split('/').map(unhex).collect(); + let (last, parents) = components.split_last().unwrap(); + + for parent in parents { + path.push(os_str(parent)); + if !create(&path, true) { + return None; + } + } + + path.push(os_str(last)); + if !create(&path, *is_dir) { + return None; + } + } + Some(()) +} + +fn os_str(bytes: &[u8]) -> &std::ffi::OsStr { + use std::os::unix::ffi::OsStrExt; + std::ffi::OsStr::from_bytes(bytes) +} + +fn create(path: &Path, is_dir: bool) -> bool { + if is_dir { + std::fs::create_dir_all(path).is_ok() + } else { + std::fs::write(path, b"").is_ok() + } +} + +fn unhex(s: &str) -> Vec { + (0..s.len()) + .step_by(2) + .map(|i| u8::from_str_radix(&s[i..i + 2], 16).unwrap()) + .collect() +}