diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 6341ff8b1..018910119 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -428,8 +428,9 @@ fn sparse_patterns_from_proto( /// Creates intermediate directories from the `working_copy_path` to the /// `repo_path` parent. Returns disk path for the `repo_path` file. /// -/// If an intermediate directory exists and if it is a symlink, this function -/// will return an error. The `working_copy_path` directory may be a symlink. +/// If an intermediate directory exists and if it is a file or symlink, this +/// function returns `Ok(None)` to signal that the path should be skipped. +/// The `working_copy_path` directory may be a symlink. /// /// Note that this does not prevent TOCTOU bugs caused by concurrent checkouts. /// Another process may remove the directory created by this function and put a @@ -443,24 +444,22 @@ fn create_parent_dirs( for c in parent_path.components() { dir_path.push(c.to_fs_name().map_err(|err| err.with_path(repo_path))?); match fs::create_dir(&dir_path) { - Ok(()) => {} - Err(_) - if dir_path - .symlink_metadata() - .map(|m| m.is_dir()) - .unwrap_or(false) => {} - Err(err) => { - if dir_path.is_file() { - return Ok(None); + Ok(()) => {} // New directory + Err(err) => match dir_path.symlink_metadata() { + Ok(m) if m.is_dir() => {} // Existing directory + Ok(_) => { + return Ok(None); // Skip existing file or symlink } - return Err(CheckoutError::Other { - message: format!( - "Failed to create parent directories for {}", - repo_path.to_fs_path_unchecked(working_copy_path).display(), - ), - err: err.into(), - }); - } + Err(_) => { + return Err(CheckoutError::Other { + message: format!( + "Failed to create parent directories for {}", + repo_path.to_fs_path_unchecked(working_copy_path).display(), + ), + err: err.into(), + }) + } + }, } } @@ -485,6 +484,23 @@ fn remove_old_file(disk_path: &Path) -> Result<(), CheckoutError> { } } +/// Checks if new file or symlink named `disk_path` can be created. +/// +/// If the file already exists, this function return `Ok(false)` to signal +/// that the path should be skipped. +/// +/// This function can fail if `disk_path.parent()` isn't a directory. +fn can_create_new_file(disk_path: &Path) -> Result { + match disk_path.symlink_metadata() { + Ok(_) => Ok(false), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(true), + Err(err) => Err(CheckoutError::Other { + message: format!("Failed to stat {}", disk_path.display()), + err: err.into(), + }), + } +} + fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { let time = metadata .modified() @@ -1441,11 +1457,12 @@ impl TreeState { }; if present_before { remove_old_file(&disk_path)?; - } else if disk_path.exists() { + } else if !can_create_new_file(&disk_path)? { changed_file_states.push((path, FileState::placeholder())); stats.skipped_files += 1; continue; } + // TODO: Check that the file has not changed before overwriting/removing it. let file_state = match after { MaterializedTreeValue::Absent | MaterializedTreeValue::AccessDenied(_) => { diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index f55f598fe..19ed954e8 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -57,6 +57,17 @@ use testutils::write_random_commit; use testutils::TestRepoBackend; use testutils::TestWorkspace; +fn check_icase_fs(dir: &Path) -> bool { + let test_file = tempfile::Builder::new() + .prefix("icase-") + .tempfile_in(dir) + .unwrap(); + let orig_name = test_file.path().file_name().unwrap().to_str().unwrap(); + let upper_name = orig_name.to_ascii_uppercase(); + assert_ne!(orig_name, upper_name); + dir.join(upper_name).try_exists().unwrap() +} + fn to_owned_path_vec(paths: &[&RepoPath]) -> Vec { paths.iter().map(|&path| path.to_owned()).collect() } @@ -1212,32 +1223,112 @@ fn test_check_out_existing_file_cannot_be_removed() { } #[test] -fn test_existing_directory_symlink() { +fn test_check_out_existing_directory_symlink() { + if !check_symlink_support().unwrap() { + eprintln!("Skipping test because symlink isn't supported"); + return; + } + let settings = testutils::user_settings(); let mut test_workspace = TestWorkspace::init(&settings); let repo = &test_workspace.repo; let workspace_root = test_workspace.workspace.workspace_root().to_owned(); - // Creates a symlink in working directory, and a tree that will add a file under - // the symlinked directory. - if check_symlink_support().unwrap_or(false) { - try_symlink("..", workspace_root.join("parent")).unwrap(); - } else { - return; - } + // Creates a symlink in working directory, and a tree that will add a file + // under the symlinked directory. + try_symlink("..", workspace_root.join("parent")).unwrap(); let file_path = RepoPath::from_internal_string("parent/escaped"); let tree = create_tree(repo, &[(file_path, "contents")]); let commit = commit_with_tree(repo.store(), tree.id()); - // Checkout should fail because "parent" already exists and is a symlink. + // Checkout doesn't fail, but the file should be skipped. let ws = &mut test_workspace.workspace; - assert!(ws.check_out(repo.op_id().clone(), None, &commit).is_err()); + let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); + assert_eq!(stats.skipped_files, 1); // Therefore, "../escaped" shouldn't be created. assert!(!workspace_root.parent().unwrap().join("escaped").exists()); } +#[test] +fn test_check_out_existing_directory_symlink_icase_fs() { + if !check_symlink_support().unwrap() { + eprintln!("Skipping test because symlink isn't supported"); + return; + } + + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + let is_icase_fs = check_icase_fs(&workspace_root); + + // Creates a symlink in working directory, and a tree that will add a file + // under the symlinked directory. + try_symlink("..", workspace_root.join("parent")).unwrap(); + + let file_path = RepoPath::from_internal_string("PARENT/escaped"); + let tree = create_tree(repo, &[(file_path, "contents")]); + let commit = commit_with_tree(repo.store(), tree.id()); + + // Checkout doesn't fail, but the file should be skipped on icase fs. + let ws = &mut test_workspace.workspace; + let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); + if is_icase_fs { + assert_eq!(stats.skipped_files, 1); + } else { + assert_eq!(stats.skipped_files, 0); + } + + // Therefore, "../escaped" shouldn't be created. + assert!(!workspace_root.parent().unwrap().join("escaped").exists()); +} + +#[test_case(false; "symlink target does not exist")] +#[test_case(true; "symlink target exists")] +fn test_check_out_existing_file_symlink_icase_fs(victim_exists: bool) { + if !check_symlink_support().unwrap() { + eprintln!("Skipping test because symlink isn't supported"); + return; + } + + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + let is_icase_fs = check_icase_fs(&workspace_root); + + // Creates a symlink in working directory, and a tree that will overwrite + // the symlink content. + try_symlink("../pwned", workspace_root.join("parent")).unwrap(); + let victim_file_path = workspace_root.parent().unwrap().join("pwned"); + if victim_exists { + std::fs::write(&victim_file_path, "old").unwrap(); + } + assert_eq!(workspace_root.join("parent").exists(), victim_exists); + + let file_path = RepoPath::from_internal_string("PARENT"); + let tree = create_tree(repo, &[(file_path, "bad")]); + let commit = commit_with_tree(repo.store(), tree.id()); + + // Checkout doesn't fail, but the file should be skipped on icase fs. + let ws = &mut test_workspace.workspace; + let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); + if is_icase_fs { + assert_eq!(stats.skipped_files, 1); + } else { + assert_eq!(stats.skipped_files, 0); + } + + // Therefore, "../pwned" shouldn't be updated. + if victim_exists { + assert_eq!(std::fs::read(&victim_file_path).unwrap(), b"old"); + } else { + assert!(!victim_file_path.exists()); + } +} + #[test] fn test_check_out_file_removal_over_existing_directory_symlink() { if !check_symlink_support().unwrap() { @@ -1271,11 +1362,8 @@ fn test_check_out_file_removal_over_existing_directory_symlink() { assert!(file_path.to_fs_path_unchecked(&workspace_root).exists()); // Check out empty tree, which tries to remove "parent/escaped". - let result = ws.check_out(repo.op_id().clone(), None, &commit2); - assert_matches!( - result, - Err(CheckoutError::Other { message, .. }) if message.contains("create parent dir") - ); + let stats = ws.check_out(repo.op_id().clone(), None, &commit2).unwrap(); + assert_eq!(stats.skipped_files, 1); // "../escaped" shouldn't be removed. assert!(victim_file_path.exists());