Fix permission issues during layer handling

Cargo.toml

@@ -9,4 +9,5 @@ members = [
     "examples/basics",
     "examples/execd",
     "examples/ruby-sample",
+    "test-buildpacks/readonly-layer-files"
 ]

libcnb/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## [Unreleased]
 
+- Fix permission issues during layer handling when the layer contains read-only directories. ([#488](https://github.com/heroku/libcnb.rs/pull/488)).
+- Add explicit `DeleteLayerError` to provide more context when debugging layer handling problems. ([#488](https://github.com/heroku/libcnb.rs/pull/488)).
+
 ## [0.9.0] 2022-07-14
 
 - Use the crate's `README.md` as the root module's rustdocs, so that all of the crate's documentation can be seen in one place on `docs.rs`. ([#460](https://github.com/heroku/libcnb.rs/pull/460))

libcnb/Cargo.toml

@@ -16,6 +16,7 @@ anyhow = { version = "1.0.58", optional = true }
 libcnb-data = { path = "../libcnb-data", version = "0.8.0" }
 libcnb-proc-macros = { version = "0.3.0", path = "../libcnb-proc-macros" }
 serde = { version = "1.0.139", features = ["derive"] }
+stacker = "0.1.15"
 thiserror = "1.0.31"
 toml = "0.5.9"
 

libcnb/src/layer/handling.rs

@@ -7,7 +7,7 @@ use crate::data::layer_content_metadata::LayerContentMetadata;
 use crate::generic::GenericMetadata;
 use crate::layer::{ExistingLayerStrategy, Layer, LayerData, MetadataMigration};
 use crate::layer_env::LayerEnv;
-use crate::util::default_on_not_found;
+use crate::util::{default_on_not_found, remove_recursively};
 use crate::Buildpack;
 use crate::{write_toml_file, TomlFileError};
 use serde::de::DeserializeOwned;
@@ -167,6 +167,12 @@ impl<E> From<HandleLayerError> for HandleLayerErrorOrBuildpackError<E> {
     }
 }
 
+impl<E> From<DeleteLayerError> for HandleLayerErrorOrBuildpackError<E> {
+    fn from(e: DeleteLayerError) -> Self {
+        HandleLayerErrorOrBuildpackError::HandleLayerError(HandleLayerError::DeleteLayerError(e))
+    }
+}
+
 impl<E> From<ReadLayerError> for HandleLayerErrorOrBuildpackError<E> {
     fn from(e: ReadLayerError) -> Self {
         HandleLayerErrorOrBuildpackError::HandleLayerError(HandleLayerError::ReadLayerError(e))
@@ -190,6 +196,9 @@ pub enum HandleLayerError {
     #[error("Unexpected IoError while handling layer: {0}")]
     IoError(#[from] std::io::Error),
 
+    #[error("Unexpected DeleteLayerError while handling layer: {0}")]
+    DeleteLayerError(#[from] DeleteLayerError),
+
     #[error("Unexpected ReadLayerError while handling layer: {0}")]
     ReadLayerError(#[from] ReadLayerError),
 
@@ -200,6 +209,12 @@ pub enum HandleLayerError {
     UnexpectedMissingLayer,
 }
 
+#[derive(thiserror::Error, Debug)]
+pub enum DeleteLayerError {
+    #[error("IOError while deleting existing layer: {0}")]
+    IoError(#[from] std::io::Error),
+}
+
 #[derive(thiserror::Error, Debug)]
 pub enum ReadLayerError {
     #[error("Layer content metadata could not be parsed!")]
@@ -231,11 +246,11 @@ enum ExecDPrograms {
 fn delete_layer<P: AsRef<Path>>(
     layers_dir: P,
     layer_name: &LayerName,
-) -> Result<(), std::io::Error> {
+) -> Result<(), DeleteLayerError> {
     let layer_dir = layers_dir.as_ref().join(layer_name.as_str());
     let layer_toml = layers_dir.as_ref().join(format!("{layer_name}.toml"));
 
-    default_on_not_found(fs::remove_dir_all(&layer_dir))?;
+    default_on_not_found(remove_recursively(&layer_dir))?;
     default_on_not_found(fs::remove_file(&layer_toml))?;
 
     Ok(())

libcnb/src/util.rs

@@ -1,3 +1,7 @@
+use std::fs;
+use std::fs::Permissions;
+use std::path::Path;
+
 /// Removes [`std::io::Error`] values from a [`Result`] that have the
 /// [`std::io::ErrorKind::NotFound`] error kind by replacing them with the default value for `T`.
 pub(crate) fn default_on_not_found<T: Default>(
@@ -12,10 +16,54 @@ pub(crate) fn default_on_not_found<T: Default>(
     }
 }
 
+/// Recursively removes the given path, similar to [`std::fs::remove_dir_all`].
+///
+/// Compared to `remove_dir_all`, this function behaves more like `rm -rf` on UNIX systems. It will
+/// delete files and directories even if their permissions would normally prevent deletion as long
+/// as the current user is the owner of these files (or root).
+pub(crate) fn remove_recursively<P: AsRef<Path>>(path: P) -> std::io::Result<()> {
+    if path.as_ref().symlink_metadata()?.is_dir() {
+        // To delete a directory, the current user must have the permission to write and list the
+        // directory (to empty it before deleting). To reduce the possibility of permission errors,
+        // we try to set the correct permissions before attempting to delete the directory and the
+        // files within it.
+        let permissions = if cfg!(target_family = "unix") {
+            use std::os::unix::fs::PermissionsExt;
+            Permissions::from_mode(0o777)
+        } else {
+            let mut permissions = path.as_ref().metadata()?.permissions();
+            permissions.set_readonly(false);
+            permissions
+        };
+
+        fs::set_permissions(&path, permissions)?;
+
+        for entry in fs::read_dir(&path)? {
+            let path = entry?.path();
+
+            // Since the directory structure could be a deep, blowing the stack is a real danger
+            // here. We use the `stacker` crate to allocate stack on the heap if we're running out
+            // of stack space.
+            //
+            // Neither the minimum stack size nor the amount of bytes allocated when we run out of
+            // stack space are backed by data/science. They're "best guesses", if you have reason
+            // to believe they need to change, you're probably right.
+            stacker::maybe_grow(4096, 32768, || remove_recursively(&path))?;
+        }
+
+        fs::remove_dir(&path)
+    } else {
+        fs::remove_file(&path)
+    }
+}
+
 #[cfg(test)]
 mod tests {
-    use crate::util::default_on_not_found;
+    use crate::util::{default_on_not_found, remove_recursively};
+    use std::fs;
+    use std::fs::Permissions;
     use std::io::ErrorKind;
+    use tempfile::tempdir;
 
     #[test]
     fn default_on_not_found_with_notfound() {
@@ -38,4 +86,32 @@ mod tests {
     fn default_on_not_found_with_ok() {
         assert_eq!(default_on_not_found(Ok("Hello!")).unwrap(), "Hello!");
     }
+
+    #[test]
+    #[cfg(target_family = "unix")]
+    fn remove_recursively_readonly_directory() {
+        use std::os::unix::fs::PermissionsExt;
+
+        let temp_dir = tempdir().unwrap();
+        let directory = temp_dir.path().join("sub_dir");
+        fs::create_dir_all(&directory).unwrap();
+        fs::write(directory.join("destination.txt"), "LV-426").unwrap();
+        fs::set_permissions(&directory, Permissions::from_mode(0o555)).unwrap();
+
+        remove_recursively(temp_dir.path()).unwrap();
+    }
+
+    #[test]
+    #[cfg(target_family = "unix")]
+    fn remove_recursively_no_executable_bit_directory() {
+        use std::os::unix::fs::PermissionsExt;
+
+        let temp_dir = tempdir().unwrap();
+        let directory = temp_dir.path().join("sub_dir");
+        fs::create_dir_all(&directory).unwrap();
+        fs::write(directory.join("destination.txt"), "LV-426").unwrap();
+        fs::set_permissions(&directory, Permissions::from_mode(0o666)).unwrap();
+
+        remove_recursively(temp_dir.path()).unwrap();
+    }
 }

test-buildpacks/README.md

@@ -0,0 +1,5 @@
+# Test Buildpacks
+
+Buildpacks in this directory are meant as integration tests for libcnb.rs. In some cases, unit tests are not enough to
+verify the correctness of the framework. Don't use these buildpacks as examples on how to implement a buildpack, see
+the [`examples` directory](../examples) instead.

test-buildpacks/readonly-layer-files/Cargo.toml

@@ -0,0 +1,12 @@
+[package]
+name = "readonly-layer-files"
+version = "0.0.0"
+edition = "2021"
+rust-version = "1.59"
+publish = false
+
+[dependencies]
+libcnb = { path = "../../libcnb" }
+
+[dev-dependencies]
+libcnb-test = { path = "../../libcnb-test" }

test-buildpacks/readonly-layer-files/buildpack.toml

@@ -0,0 +1,9 @@
+api = "0.6"
+
+[buildpack]
+id = "libcnb-test-buildpacks/readonly-layer-files"
+version = "0.1.0"
+name = "libcnb test buildpack: readonly-layer-files"
+
+[[stacks]]
+id = "*"

test-buildpacks/readonly-layer-files/src/layer.rs

@@ -0,0 +1,53 @@
+use crate::TestBuildpack;
+use libcnb::build::BuildContext;
+use libcnb::data::layer_content_metadata::LayerTypes;
+use libcnb::generic::GenericMetadata;
+use libcnb::layer::{ExistingLayerStrategy, Layer, LayerData, LayerResult, LayerResultBuilder};
+use libcnb::Buildpack;
+use std::fs;
+use std::fs::Permissions;
+use std::os::unix::fs::PermissionsExt;
+use std::path::Path;
+
+pub struct TestLayer;
+
+impl Layer for TestLayer {
+    type Buildpack = TestBuildpack;
+    type Metadata = GenericMetadata;
+
+    fn types(&self) -> LayerTypes {
+        LayerTypes {
+            launch: true,
+            build: true,
+            cache: true,
+        }
+    }
+
+    fn create(
+        &self,
+        _context: &BuildContext<Self::Buildpack>,
+        layer_path: &Path,
+    ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
+        let directory = layer_path.join("sub_directory");
+        fs::create_dir_all(&directory)?;
+
+        fs::write(directory.join("foo.txt"), "hello world!")?;
+
+        // By making the sub-directory read-only, files inside it cannot be deleted. This would
+        // cause issues when libcnb.rs tries to delete a cached layer directory unless libcnb.rs
+        // handles this case explicitly.
+        fs::set_permissions(&directory, Permissions::from_mode(0o555))?;
+
+        LayerResultBuilder::new(GenericMetadata::default()).build()
+    }
+
+    fn existing_layer_strategy(
+        &self,
+        _context: &BuildContext<Self::Buildpack>,
+        _layer_data: &LayerData<Self::Metadata>,
+    ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::Error> {
+        // Even though this is (currently) the default, we explicitly declare it here to make sure
+        // the layer will be recreated, even if the default in libcnb changes.
+        Ok(ExistingLayerStrategy::Recreate)
+    }
+}

test-buildpacks/readonly-layer-files/src/main.rs

@@ -0,0 +1,45 @@
+// Enable Clippy lints that are disabled by default.
+// https://rust-lang.github.io/rust-clippy/stable/index.html
+#![warn(clippy::pedantic)]
+// This lint is too noisy and enforces a style that reduces readability in many cases.
+#![allow(clippy::module_name_repetitions)]
+
+mod layer;
+
+use crate::layer::TestLayer;
+use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder};
+use libcnb::data::layer_name;
+use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder};
+use libcnb::generic::{GenericMetadata, GenericPlatform};
+use libcnb::{buildpack_main, Buildpack};
+use std::io::Error;
+
+pub struct TestBuildpack;
+
+impl Buildpack for TestBuildpack {
+    type Platform = GenericPlatform;
+    type Metadata = GenericMetadata;
+    type Error = TestBuildpackError;
+
+    fn detect(&self, _context: DetectContext<Self>) -> libcnb::Result<DetectResult, Self::Error> {
+        DetectResultBuilder::pass().build()
+    }
+
+    fn build(&self, context: BuildContext<Self>) -> libcnb::Result<BuildResult, Self::Error> {
+        context.handle_layer(layer_name!("test"), TestLayer)?;
+        BuildResultBuilder::new().build()
+    }
+}
+
+#[derive(Debug)]
+pub enum TestBuildpackError {
+    IOError(std::io::Error),
+}
+
+impl From<std::io::Error> for TestBuildpackError {
+    fn from(io_error: Error) -> Self {
+        Self::IOError(io_error)
+    }
+}
+
+buildpack_main!(TestBuildpack);

test-buildpacks/readonly-layer-files/tests/integration_test.rs

@@ -0,0 +1,28 @@
+//! Integration tests using libcnb-test.
+//!
+//! All integration tests are skipped by default (using the `ignore` attribute),
+//! since performing builds is slow. To run the tests use: `cargo test -- --ignored`
+
+// Enable Clippy lints that are disabled by default.
+// https://rust-lang.github.io/rust-clippy/stable/index.html
+#![warn(clippy::pedantic)]
+
+use libcnb_test::{BuildConfig, TestRunner};
+use std::env::temp_dir;
+
+use std::fs;
+
+#[test]
+#[ignore = "integration test"]
+fn test() {
+    let empty_app_dir = temp_dir().join("empty-app-dir");
+    fs::create_dir_all(&empty_app_dir).unwrap();
+
+    let build_config = BuildConfig::new("heroku/builder:22", &empty_app_dir);
+
+    // The test succeeds if we're able to build with a cached layer that has directories with
+    // problematic permissions inside it (See the buildpack for details).
+    TestRunner::default().build(&build_config, |context| {
+        context.rebuild(&build_config, |_| {});
+    });
+}