diff --git a/Cargo.lock b/Cargo.lock index b2d88dda..85d94e53 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2060,9 +2060,9 @@ checksum = "92620684d99f750bae383ecb3be3748142d6095760afd5cbcf2261e9a279d780" [[package]] name = "h2" -version = "0.4.15" +version = "0.4.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6cb093c84e8bd9b188d4c4a8cb6579fc016968d14c99882163cd3ff402a4f155" +checksum = "a9f37a958b41b3b19ee2707c06439c0e9e547e847223eb791ecb0cb821c65e27" dependencies = [ "atomic-waker", "bytes", diff --git a/src/compiler/cc.rs b/src/compiler/cc.rs index 1f187b85..fddc7766 100644 --- a/src/compiler/cc.rs +++ b/src/compiler/cc.rs @@ -999,10 +999,10 @@ impl CcArgs { /// Whether an existing output needs the selected compiler's own path /// handling instead of cache materialization. /// - /// GCC and clang can differ in how they handle an existing output, and - /// filesystem permissions/ACLs can make truncate and replace semantics - /// observably different. Kache cannot safely infer those semantics from a - /// pathname, so every existing output uses the selected compiler directly. + /// Ordinary compiler-owned outputs are private, owner-writable regular + /// files. Kache may replace those on a hit and let the compiler overwrite + /// them on a miss. Symlinks, hardlinks, read-only files and non-regular + /// paths still need the selected compiler's exact pathname semantics. pub(crate) fn requires_compiler_output_semantics(&self) -> bool { self.compiler_output_paths() .into_iter() @@ -1073,13 +1073,37 @@ impl CcArgs { } } -fn output_path_requires_compiler_semantics(path: &Path) -> bool { +pub(crate) fn output_path_requires_compiler_semantics(path: &Path) -> bool { match std::fs::symlink_metadata(path) { - Ok(_) => true, + Ok(meta) => !meta.file_type().is_file() || !regular_output_is_replaceable(path, &meta), Err(err) => err.kind() != std::io::ErrorKind::NotFound, } } +fn regular_output_is_replaceable(path: &Path, meta: &std::fs::Metadata) -> bool { + regular_output_is_independent(path, meta) && regular_output_is_owner_writable(meta) +} + +fn regular_output_is_owner_writable(meta: &std::fs::Metadata) -> bool { + if meta.permissions().readonly() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + // SAFETY: geteuid has no arguments, pointers, or preconditions. + let current_uid = unsafe { libc::geteuid() }; + meta.uid() == current_uid && meta.permissions().mode() & 0o200 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + #[cfg(unix)] fn regular_output_is_independent(_path: &Path, meta: &std::fs::Metadata) -> bool { use std::os::unix::fs::MetadataExt; @@ -8499,7 +8523,7 @@ mod tests { } #[test] - fn cc_output_safety_refuses_existing_writable_regular_file() { + fn cc_output_safety_allows_existing_writable_private_regular_file() { let dir = tempfile::tempdir().unwrap(); let output = dir.path().join("plain.o"); std::fs::write(&output, b"ordinary compiler output").unwrap(); @@ -8507,7 +8531,12 @@ mod tests { let output_str = output.to_string_lossy().into_owned(); let parsed = CcArgs::parse(&s(&["cc", "-c", "foo.c", "-o", &output_str])).unwrap(); - assert!(parsed.requires_compiler_output_semantics()); + assert!(!parsed.requires_compiler_output_semantics()); + assert!(!parsed.refuse_reasons(&[]).iter().any(|reason| { + reason + .description() + .contains("requires compiler write semantics") + })); assert_eq!( discover_cc_output_artifacts(&parsed).outputs().len(), 1, @@ -8515,6 +8544,27 @@ mod tests { ); } + #[test] + fn regular_output_writability_distinguishes_readonly_metadata() { + let dir = tempfile::tempdir().unwrap(); + let output = dir.path().join("permissions.o"); + std::fs::write(&output, b"ordinary compiler output").unwrap(); + + let writable = std::fs::metadata(&output).unwrap(); + let original_permissions = writable.permissions(); + assert!(regular_output_is_owner_writable(&writable)); + + let mut readonly_permissions = original_permissions.clone(); + readonly_permissions.set_readonly(true); + std::fs::set_permissions(&output, readonly_permissions).unwrap(); + let readonly = std::fs::metadata(&output).unwrap(); + assert!(!regular_output_is_owner_writable(&readonly)); + + // Windows refuses to remove a read-only file, so restore the exact + // original permissions before the temporary directory is dropped. + std::fs::set_permissions(&output, original_permissions).unwrap(); + } + /// A user-owned read-only output must reach the selected compiler intact. #[cfg(unix)] #[test] diff --git a/src/link.rs b/src/link.rs index 6d9e7f56..b9bcbc3e 100644 --- a/src/link.rs +++ b/src/link.rs @@ -779,7 +779,7 @@ fn copy_file(src: &Path, dst: &Path, executable: bool) -> Result<()> { Ok(()) } -/// A fully-written C/C++ cache artifact awaiting no-clobber publication. +/// A fully-written C/C++ cache artifact awaiting publication. /// /// The staging file is created in the target directory with the same requested /// mode as a compiler output (`0666`). The kernel therefore applies the current @@ -810,6 +810,21 @@ impl PreparedWritableTarget { crate::opcounts::record_copied(self.bytes); Ok(()) } + + /// Atomically replace an existing ordinary compiler output. + /// + /// The caller must first establish that the target is a private, writable + /// regular file. Special paths retain the no-clobber path above and are + /// handled by the selected compiler instead. + pub(crate) fn publish_replacing(self) -> Result<()> { + let target = self.target; + self.staged + .persist(&target) + .map_err(|error| error.error) + .with_context(|| format!("publishing cc output over {}", target.display()))?; + crate::opcounts::record_copied(self.bytes); + Ok(()) + } } fn new_writable_staging_file(target: &Path) -> Result { diff --git a/src/wrapper.rs b/src/wrapper.rs index e4b007e2..06c85b9e 100644 --- a/src/wrapper.rs +++ b/src/wrapper.rs @@ -509,10 +509,7 @@ fn should_store_cc_result(exit_code: i32, has_artifacts: bool) -> bool { } fn cc_output_path_requires_passthrough(path: &Path) -> bool { - match std::fs::symlink_metadata(path) { - Ok(_) => true, - Err(error) => error.kind() != std::io::ErrorKind::NotFound, - } + crate::compiler::cc::output_path_requires_compiler_semantics(path) } /// Forward a `cc`-crate compiler-family probe (`kache -E `) to @@ -1210,15 +1207,19 @@ fn publish_prepared_cc_artifacts_with( mut before_publish: impl FnMut(usize, &Path) -> Result<()>, ) -> Result<()> { // Validate the whole set before making any final pathname visible. + let mut replace_existing = Vec::with_capacity(prepared.len()); for artifact in &prepared { if cc_output_path_requires_passthrough(artifact.target()) { anyhow::bail!( "cc restore: output path changed and now requires compiler passthrough semantics" ); } + replace_existing.push(std::fs::symlink_metadata(artifact.target()).is_ok()); } - for (index, artifact) in prepared.into_iter().enumerate() { + for (index, (artifact, replace_existing)) in + prepared.into_iter().zip(replace_existing).enumerate() + { if let Err(error) = before_publish(index, artifact.target()) { return if index == 0 { Err(error) @@ -1226,7 +1227,18 @@ fn publish_prepared_cc_artifacts_with( Err(error.context(PartialCcRestore)) }; } - if let Err(error) = artifact.publish() { + let publish = if replace_existing { + if cc_output_path_requires_passthrough(artifact.target()) { + Err(anyhow::anyhow!( + "cc restore: output path changed and now requires compiler passthrough semantics" + )) + } else { + artifact.publish_replacing() + } + } else { + artifact.publish() + }; + if let Err(error) = publish { return if index == 0 { Err(error) } else { @@ -1239,9 +1251,10 @@ fn publish_prepared_cc_artifacts_with( /// Restore cached cc artifacts to this invocation's output paths. /// -/// Every artifact is staged first, then the set is published absent-only. If a -/// race wins after publication starts, the caller receives `PartialCcRestore` -/// and must not run the compiler over the partially restored output set. +/// Every artifact is staged first. Absent paths use no-clobber publication; +/// validated ordinary existing outputs are atomically replaced. If a race wins +/// after publication starts, the caller receives `PartialCcRestore` and must +/// not run the compiler over the partially restored output set. fn restore_cc_from_cache( store: &Store, parsed: &crate::compiler::cc::CcArgs, @@ -5853,6 +5866,60 @@ mod tests { assert!(blob_meta.permissions().readonly()); } + #[test] + fn restore_cc_from_cache_replaces_existing_plain_object() { + let dir = tempfile::tempdir().unwrap(); + let config = test_config(dir.path().join("cache")); + let store = Store::open(&config).unwrap(); + let hash = "abababababababababababababababababababababababababababababababab"; + create_blob(&store, hash, b"cached object"); + + let output = dir.path().join("output.o"); + std::fs::write(&output, b"stale object").unwrap(); + let output_str = output.to_string_lossy().into_owned(); + let parsed = CcCompiler::new() + .parse(&s(&["cc", "-c", "foo.c", "-o", &output_str])) + .unwrap(); + let meta = entry_meta("cc-replace-key", vec![cached_file("foo.o", hash)], &[]); + + restore_cc_from_cache(&store, &parsed, &meta).unwrap(); + + assert_eq!(std::fs::read(&output).unwrap(), b"cached object"); + assert_eq!( + std::fs::read(store.blob_path(hash)).unwrap(), + b"cached object" + ); + } + + #[test] + fn cc_restore_revalidates_existing_target_before_replacing_it() { + let dir = tempfile::tempdir().unwrap(); + let output = dir.path().join("output.o"); + std::fs::write(&output, b"race winner").unwrap(); + let prepared = + vec![link::prepare_writable_target_from_bytes(&output, b"cached object").unwrap()]; + + let error = publish_prepared_cc_artifacts_with(prepared, |_, target| { + let mut permissions = std::fs::metadata(target)?.permissions(); + permissions.set_readonly(true); + std::fs::set_permissions(target, permissions)?; + Ok(()) + }) + .unwrap_err(); + + assert!(error.to_string().contains("requires compiler passthrough")); + assert_eq!(std::fs::read(&output).unwrap(), b"race winner"); + + // TempDir cleanup cannot remove a read-only Windows file. + #[cfg(windows)] + { + let mut permissions = std::fs::metadata(&output).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + permissions.set_readonly(false); + std::fs::set_permissions(&output, permissions).unwrap(); + } + } + #[test] fn cc_restore_marks_partial_publication_and_preserves_race_winner() { let dir = tempfile::tempdir().unwrap(); @@ -6800,13 +6867,13 @@ exit 0 } #[test] - fn cc_output_path_passthrough_distinguishes_absent_and_existing_paths() { + fn cc_output_path_passthrough_allows_plain_files_but_refuses_symlinks() { let dir = tempfile::tempdir().unwrap(); let output = dir.path().join("output.o"); assert!(!cc_output_path_requires_passthrough(&output)); std::fs::write(&output, b"existing").unwrap(); - assert!(cc_output_path_requires_passthrough(&output)); + assert!(!cc_output_path_requires_passthrough(&output)); #[cfg(unix)] { @@ -6846,6 +6913,7 @@ exit 0 .unwrap(); std::fs::set_permissions(&fallback, std::fs::Permissions::from_mode(0o755)).unwrap(); std::fs::write(&output, b"existing output").unwrap(); + std::fs::set_permissions(&output, std::fs::Permissions::from_mode(0o444)).unwrap(); let capture_arg = capture.to_string_lossy().into_owned(); let output_arg = output.to_string_lossy().into_owned(); diff --git a/tests/integration_test.rs b/tests/integration_test.rs index c68d2a48..761b3168 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -1406,8 +1406,8 @@ fn test_cc_dev_null_output_is_passthrough_and_preserved() { } /// C/C++ cache materialization must never recreate the read-only/shared shape -/// that forced unsafe pre-cleaning. An existing warm output is passed to the -/// selected compiler unchanged rather than replaced by Kache. +/// that forced unsafe pre-cleaning. A private writable output remains cacheable, +/// while every restored output remains independently compiler-writable. #[cfg(unix)] #[test] fn test_cc_cache_output_stays_writable_across_miss_hit_and_recompile() { @@ -1439,12 +1439,56 @@ fn test_cc_cache_output_stays_writable_across_miss_hit_and_recompile() { assert_ne!( std::fs::metadata(&output).unwrap().permissions().mode() & 0o200, 0, - "passthrough compiler must overwrite the warm output without pre-clean" + "compiler must overwrite the warm output without unsafe pre-clean" ); let report = kache_report(cache_dir.path()); - assert_cc_report_counts(&report, 1, 1); - assert_eq!(report["summary"]["passthroughs"].as_u64(), Some(1)); + assert_cc_report_counts(&report, 2, 1); + assert_eq!(report["summary"]["passthroughs"].as_u64(), Some(0)); +} + +/// Regression for #744: an ordinary compiler-owned object remains cacheable +/// when the build system leaves it in place. This covers both halves of the +/// regression: a matching key may replace the stale object from cache, and a +/// configuration change must compile and store rather than pass through. +#[test] +fn test_cc_existing_plain_output_hits_and_reconfigured_miss_is_stored() { + build_kache(); + let project = TempDir::new().unwrap(); + let cache_dir = TempDir::new().unwrap(); + let source = project.path().join("foo.c"); + let output = project.path().join("foo.o"); + std::fs::write(&source, "int f(void) { return 42; }\n").unwrap(); + + let source_str = source.to_string_lossy().into_owned(); + let output_str = output.to_string_lossy().into_owned(); + let o0 = ["cc", "-c", &source_str, "-o", &output_str, "-O0", "-g0"]; + let o2 = ["cc", "-c", &source_str, "-o", &output_str, "-O2", "-g0"]; + + run_kache_cc(project.path(), cache_dir.path(), &o0); + let cached_o0 = std::fs::read(&output).unwrap(); + + std::fs::write(&output, b"stale ordinary object").unwrap(); + run_kache_cc(project.path(), cache_dir.path(), &o0); + assert_eq!( + std::fs::read(&output).unwrap(), + cached_o0, + "an existing private regular object must be replaceable on a cache hit" + ); + + run_kache_cc(project.path(), cache_dir.path(), &o2); + std::fs::remove_file(&output).unwrap(); + run_kache_cc(project.path(), cache_dir.path(), &o2); + + let report = kache_report(cache_dir.path()); + let summary = &report["summary"]; + assert_eq!(summary["local_hits"].as_u64(), Some(2)); + assert_eq!(summary["passthroughs"].as_u64(), Some(0)); + assert_eq!( + summary["misses"].as_u64().unwrap_or(0) + summary["dups"].as_u64().unwrap_or(0), + 2, + "both cold configurations must be admitted to the cache" + ); } /// A cache hit must not create an output directory that the selected compiler diff --git a/tests/unknown_compiler_probe_test.rs b/tests/unknown_compiler_probe_test.rs index 029cad4b..85915b50 100644 --- a/tests/unknown_compiler_probe_test.rs +++ b/tests/unknown_compiler_probe_test.rs @@ -3,7 +3,7 @@ use std::fs; use std::os::unix::fs::PermissionsExt; use std::process::Command; -use std::time::Instant; +use std::time::{Duration, Instant}; fn kache_binary() -> &'static str { env!("CARGO_BIN_EXE_kache") @@ -101,6 +101,14 @@ fn probe_recovers_when_wrapper_leaves_descendant_on_stdout() { if let Ok(pid_str) = fs::read_to_string(&pid_file) && let Ok(pid) = pid_str.trim().parse::() { + // The process-group SIGKILL has been sent before kache returns, but an + // orphaned descendant can remain visible as a zombie until init reaps + // it. Give that asynchronous reap a small bounded window before + // treating the process as still alive. + let deadline = Instant::now() + Duration::from_secs(2); + while unsafe { libc::kill(pid, 0) == 0 } && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(10)); + } let still_alive = unsafe { libc::kill(pid, 0) == 0 }; assert!( !still_alive,