From 247ebf4f0192c9f7206bd54a1b19c2b2e8cea4ce Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Mar 2026 18:50:58 +0000 Subject: [PATCH] fix(fs): enforce write limits on chmod copy-on-write https://claude.ai/code/session_01WZjYqxm5xMPAEe7FSHJkDy --- crates/bashkit/src/fs/overlay.rs | 278 ++++++++++++++++++++++++++++--- 1 file changed, 259 insertions(+), 19 deletions(-) diff --git a/crates/bashkit/src/fs/overlay.rs b/crates/bashkit/src/fs/overlay.rs index 312e0117..d0a36636 100644 --- a/crates/bashkit/src/fs/overlay.rs +++ b/crates/bashkit/src/fs/overlay.rs @@ -148,6 +148,9 @@ pub struct OverlayFs { whiteouts: RwLock>, /// Combined limits for the overlay view limits: FsLimits, + // Tracks lower-layer usage that is hidden by upper overrides or whiteouts. + // Updated incrementally in async methods so compute_usage (sync) stays accurate. + lower_hidden: RwLock, } impl OverlayFs { @@ -218,6 +221,7 @@ impl OverlayFs { upper: InMemoryFs::with_limits(FsLimits::unlimited()), whiteouts: RwLock::new(HashSet::new()), limits, + lower_hidden: RwLock::new(FsUsage::default()), } } @@ -243,18 +247,27 @@ impl OverlayFs { } /// Compute combined usage (upper + visible lower). + /// + /// Deducts lower-layer entries that are hidden by upper overrides or whiteouts + /// to avoid double-counting. The `lower_hidden` accumulator is maintained + /// incrementally by write_file, remove, chmod, and related async methods. fn compute_usage(&self) -> FsUsage { - // Get upper layer usage let upper_usage = self.upper.usage(); - - // Lower layer usage is counted but we don't double-count - // files that are overwritten in upper or whited out let lower_usage = self.lower.usage(); - - // Combine both layers - let total_bytes = upper_usage.total_bytes + lower_usage.total_bytes; - let file_count = upper_usage.file_count + lower_usage.file_count; - let dir_count = upper_usage.dir_count + lower_usage.dir_count; + let hidden = self.lower_hidden.read().unwrap(); + + let total_bytes = upper_usage + .total_bytes + .saturating_add(lower_usage.total_bytes) + .saturating_sub(hidden.total_bytes); + let file_count = upper_usage + .file_count + .saturating_add(lower_usage.file_count) + .saturating_sub(hidden.file_count); + let dir_count = upper_usage + .dir_count + .saturating_add(lower_usage.dir_count) + .saturating_sub(hidden.dir_count); FsUsage::new(total_bytes, file_count, dir_count) } @@ -340,6 +353,19 @@ impl OverlayFs { let mut whiteouts = self.whiteouts.write().unwrap(); whiteouts.remove(&path); } + + /// Record that a lower-layer file is now hidden (overridden or whited out). + fn hide_lower_file(&self, size: u64) { + let mut hidden = self.lower_hidden.write().unwrap(); + hidden.total_bytes = hidden.total_bytes.saturating_add(size); + hidden.file_count = hidden.file_count.saturating_add(1); + } + + /// Record that a lower-layer directory is now hidden. + fn hide_lower_dir(&self) { + let mut hidden = self.lower_hidden.write().unwrap(); + hidden.dir_count = hidden.dir_count.saturating_add(1); + } } #[async_trait] @@ -372,7 +398,14 @@ impl FileSystem for OverlayFs { // Check limits before writing self.check_write_limits(content.len())?; - // Remove any whiteout for this path + // Track whether lower file becomes newly hidden by this write. + // If the path is not already in upper AND not already whited out, + // then writing to upper will newly shadow the lower entry. + let already_in_upper = self.upper.exists(&path).await.unwrap_or(false); + let already_whited = self.is_whiteout(&path); + let lower_exists = self.lower.exists(&path).await.unwrap_or(false); + + // Remove any whiteout for this path (upper override takes over hiding) self.remove_whiteout(&path); // Ensure parent directory exists in upper @@ -390,7 +423,21 @@ impl FileSystem for OverlayFs { } // Write to upper - self.upper.write_file(&path, content).await + self.upper.write_file(&path, content).await?; + + // If this write newly hides a lower file (not previously hidden by + // upper override or whiteout), record the hidden lower contribution. + if lower_exists && !already_in_upper && !already_whited { + if let Ok(meta) = self.lower.stat(&path).await { + match meta.file_type { + FileType::File => self.hide_lower_file(meta.size), + FileType::Directory => self.hide_lower_dir(), + _ => {} + } + } + } + + Ok(()) } async fn append_file(&self, path: &Path, content: &[u8]) -> Result<()> { @@ -415,6 +462,7 @@ impl FileSystem for OverlayFs { // If file exists in lower, copy-on-write if self.lower.exists(&path).await.unwrap_or(false) { + let lower_meta = self.lower.stat(&path).await?; let existing = self.lower.read_file(&path).await?; // Check limits for combined content @@ -430,7 +478,11 @@ impl FileSystem for OverlayFs { // Copy existing content and append new content let mut combined = existing; combined.extend_from_slice(content); - return self.upper.write_file(&path, &combined).await; + self.upper.write_file(&path, &combined).await?; + + // Lower file is now hidden by the upper copy + self.hide_lower_file(lower_meta.size); + return Ok(()); } // Create new file in upper @@ -469,15 +521,22 @@ impl FileSystem for OverlayFs { self.upper.remove(&path, recursive).await?; } - // If was in lower, add whiteout + // If was in lower, add whiteout and track hiding. + // If in_upper was also true, the lower was already hidden (by the upper + // override). The whiteout replaces the override as the hiding mechanism, + // so no additional deduction needed. if in_lower { - if recursive { - // Add whiteouts for all paths under this directory - // This is a simplification - real overlayfs uses opaque dirs - self.add_whiteout(&path); - } else { - self.add_whiteout(&path); + // Newly hiding the lower entry only if there was no upper override + if !in_upper { + if let Ok(meta) = self.lower.stat(&path).await { + match meta.file_type { + FileType::File => self.hide_lower_file(meta.size), + FileType::Directory => self.hide_lower_dir(), + _ => {} + } + } } + self.add_whiteout(&path); } Ok(()) @@ -627,9 +686,20 @@ impl FileSystem for OverlayFs { // Create in upper with same content (for files) if stat.file_type == FileType::File { let content = self.lower.read_file(&path).await?; + self.check_write_limits(content.len())?; + + // Ensure parent dir exists in upper before write + if let Some(parent) = path.parent() { + if !self.upper.exists(parent).await.unwrap_or(false) { + self.upper.mkdir(parent, true).await?; + } + } + self.upper.write_file(&path, &content).await?; + self.hide_lower_file(stat.size); } else if stat.file_type == FileType::Directory { self.upper.mkdir(&path, true).await?; + self.hide_lower_dir(); } return self.upper.chmod(&path, mode).await; @@ -760,6 +830,176 @@ mod tests { assert_eq!(content, b"new content"); } + #[tokio::test] + async fn test_chmod_cow_enforces_write_limits() { + // Issue #417: chmod copy-on-write must check limits before writing to upper + let lower = Arc::new(InMemoryFs::new()); + lower + .write_file(Path::new("/tmp/big.txt"), &vec![b'x'; 5000]) + .await + .unwrap(); + + // Limit upper layer to 1000 bytes total - the 5000 byte file shouldn't fit + let limits = FsLimits::new().max_total_bytes(1000); + let overlay = OverlayFs::with_limits(lower, limits); + + // chmod triggers CoW from lower -> upper; must be rejected + let result = overlay.chmod(Path::new("/tmp/big.txt"), 0o755).await; + assert!( + result.is_err(), + "chmod CoW should fail when content exceeds write limits" + ); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("filesystem full"), + "expected 'filesystem full' error, got: {err}" + ); + + // File should NOT exist in upper layer + assert!( + !overlay + .upper + .exists(Path::new("/tmp/big.txt")) + .await + .unwrap(), + "file should not have been copied to upper layer" + ); + } + + #[tokio::test] + async fn test_usage_no_double_count_override() { + // Issue #418: overwriting a lower file in upper should not double-count + let lower = Arc::new(InMemoryFs::new()); + lower + .write_file(Path::new("/tmp/file.txt"), b"lower data") // 10 bytes + .await + .unwrap(); + + let overlay = OverlayFs::new(lower); + + // Snapshot before override + let usage_before = overlay.usage(); + + // Override in upper with smaller content + overlay + .write_file(Path::new("/tmp/file.txt"), b"upper!") // 6 bytes + .await + .unwrap(); + + let usage_after = overlay.usage(); + // File count should not change: same file, just overridden + assert_eq!( + usage_after.file_count, usage_before.file_count, + "overridden file should not increase file_count" + ); + // Bytes should decrease by (10 - 6) = 4 because lower's 10 bytes are + // replaced by upper's 6 bytes + assert_eq!( + usage_after.total_bytes, + usage_before.total_bytes - 4, + "overridden file bytes should reflect upper size, not sum" + ); + } + + #[tokio::test] + async fn test_usage_no_double_count_whiteout() { + // Issue #418: deleting a lower file should deduct it from usage + let lower = Arc::new(InMemoryFs::new()); + lower + .write_file(Path::new("/tmp/gone.txt"), b"12345") // 5 bytes + .await + .unwrap(); + + let overlay = OverlayFs::new(lower.clone()); + let usage_before = overlay.usage(); + + // Delete through overlay (creates whiteout) + overlay + .remove(Path::new("/tmp/gone.txt"), false) + .await + .unwrap(); + + let usage_after = overlay.usage(); + assert_eq!( + usage_after.file_count, + usage_before.file_count - 1, + "whited-out file should not be counted" + ); + assert_eq!( + usage_after.total_bytes, + usage_before.total_bytes - 5, + "whited-out file bytes should be deducted" + ); + } + + #[tokio::test] + async fn test_usage_unique_files_both_layers() { + // Files unique to each layer should each count once + let lower = Arc::new(InMemoryFs::new()); + lower + .write_file(Path::new("/tmp/lower.txt"), b"aaa") // 3 bytes + .await + .unwrap(); + + let overlay = OverlayFs::new(lower); + let usage_before = overlay.usage(); + + overlay + .write_file(Path::new("/tmp/upper.txt"), b"bbbbb") // 5 bytes + .await + .unwrap(); + + let usage_after = overlay.usage(); + // Adding a unique upper file: +1 file, +5 bytes + assert_eq!( + usage_after.file_count, + usage_before.file_count + 1, + "unique upper file adds one to count" + ); + assert_eq!( + usage_after.total_bytes, + usage_before.total_bytes + 5, + "unique upper file adds its bytes" + ); + } + + #[tokio::test] + async fn test_usage_recreate_after_whiteout() { + // Delete then recreate: file should count once with new size + let lower = Arc::new(InMemoryFs::new()); + lower + .write_file(Path::new("/tmp/file.txt"), b"old data 10") // 11 bytes + .await + .unwrap(); + + let overlay = OverlayFs::new(lower); + let usage_before = overlay.usage(); + + // Delete + overlay + .remove(Path::new("/tmp/file.txt"), false) + .await + .unwrap(); + + // Recreate with different size + overlay + .write_file(Path::new("/tmp/file.txt"), b"new") // 3 bytes + .await + .unwrap(); + + let usage_after = overlay.usage(); + // Net effect: replaced 11-byte file with 3-byte file => -8 bytes, same count + assert_eq!( + usage_after.file_count, usage_before.file_count, + "recreated file counted once" + ); + assert_eq!( + usage_after.total_bytes, + usage_before.total_bytes - 8, + "recreated file uses new size" + ); + } + #[tokio::test] async fn test_read_dir_merged() { let lower = Arc::new(InMemoryFs::new());