Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shebang recipes no longer works when HOME is read-only #2123

Closed
vlaci opened this issue Jun 3, 2024 · 5 comments · Fixed by #2128
Closed

Shebang recipes no longer works when HOME is read-only #2123

vlaci opened this issue Jun 3, 2024 · 5 comments · Fixed by #2128

Comments

@vlaci
Copy link

vlaci commented Jun 3, 2024

As the temporary directories are now created in ~/.cache on Linux by default.

Figured this out while trying to update Just in nixpkgs1.

IMO, it would be nice to fall back gracefully to the platform's temporary directory
when we could not use the cache directory. I am willing to open a PR with something like the following diff, if you find this acceptable.

diff --git a/src/error.rs b/src/error.rs
index 06b0edf..1cc6ab6 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -20,10 +20,6 @@ pub(crate) enum Error<'src> {
     token: Token<'src>,
     output_error: OutputError,
   },
-  CacheDirIo {
-    io_error: io::Error,
-    path: PathBuf,
-  },
   ChooserInvoke {
     shell_binary: String,
     shell_arguments: String,
@@ -287,9 +283,6 @@ impl<'src> ColorDisplay for Error<'src> {
           }?,
         OutputError::Utf8(utf8_error) => write!(f, "Backtick succeeded but stdout was not utf8: {utf8_error}")?,
       }
-      CacheDirIo { io_error, path } => {
-        write!(f, "I/O error in cache dir `{}`: {io_error}", path.display())?;
-      }
       ChooserInvoke { shell_binary, shell_arguments, chooser, io_error} => {
         let chooser = chooser.to_string_lossy();
         write!(f, "Chooser `{shell_binary} {shell_arguments} {chooser}` invocation failed: {io_error}")?;
diff --git a/src/recipe.rs b/src/recipe.rs
index 97dc847..c19910f 100644
--- a/src/recipe.rs
+++ b/src/recipe.rs
@@ -355,11 +355,10 @@ impl<'src, D> Recipe<'src, D> {
       None => {
         if let Some(cache_dir) = dirs::cache_dir() {
           let path = cache_dir.join("just");
-          fs::create_dir_all(&path).map_err(|io_error| Error::CacheDirIo {
-            io_error,
-            path: path.clone(),
-          })?;
-          tempdir_builder.tempdir_in(path)
+          fs::create_dir_all(&path).map_or_else(
+            |_| tempdir_builder.tempdir(),
+            |_| tempdir_builder.tempdir_in(path),
+          )
         } else {
           tempdir_builder.tempdir()
         }
diff --git a/tests/tempdir.rs b/tests/tempdir.rs
index a7d2a5f..867aa3d 100644
--- a/tests/tempdir.rs
+++ b/tests/tempdir.rs
@@ -7,8 +7,7 @@ pub(crate) fn tempdir() -> TempDir {
 
   if let Some(cache_dir) = dirs::cache_dir() {
     let path = cache_dir.join("just");
-    fs::create_dir_all(&path).unwrap();
-    builder.tempdir_in(path)
+    fs::create_dir_all(&path).map_or_else(|_| builder.tempdir(), |_| builder.tempdir_in(path))
   } else {
     builder.tempdir()
   }

Footnotes

  1. https://github.com/NixOS/nixpkgs/pull/316156

@casey
Copy link
Owner

casey commented Jun 4, 2024

Thank you for opening this issue!

I changed the location shebang scripts are written to in #2067. The precipitating issue was that someone's /tmp directory was mounted noexec, so shebang scripts written there weren't runnable.

I sort of had a hunch that #2067 would break something else and I didn't know what, but it makes sense that read-only home would be the culprit.

Thinking out loud here, I'm wondering which is more common, having a read-only homedir or a noexec tmpdir? #2067 wasn't a particularly principled choice, I got a report of an issue with a particular configuration and made a change which fixed things for that configuration, but there is now a new issue with a different configuration, so it would be perfectly reasonable to just change it back.

Also, I'm not entirely sure that the cache dir is the right way to put shebang scripts anyways. Shebang scripts aren't reused once they're written, so there's no benefit to persisting them in the cache dir, vs having them be cleared on restart, as they would be in the tempdir.

Maybe the way to fix this is:

  1. Allow users to set, via environment variable and flag, where just puts its tempfiles. This would let users with weird configurations put them wherever they want.
  2. Probably default to the tmpdir instead of the cache dir, because that might be the better default.
  3. Check if /tmp is noexec, and if so, fall back to the cache dir.

@vlaci
Copy link
Author

vlaci commented Jun 4, 2024

What about using dirs.runtime_dir()? It is only returning something on Linux:

XDG_RUNTIME_DIR: the base directory relative to which user-specific non-essential runtime files and other file objects (such as sockets, named pipes, ...) should be stored. The directory MUST be owned by the user, who MUST be the only one having read and write access to it; its permissions MUST be 0700.

See also systemd/systemd#4081 (comment)

It'd make sense to not use world writable temporary locations from a security perspective anyway, so maybe cache_dir could be used as a fallback for Windows and Mac.


Tested, that using the below diff is sufficient to fix build in nixpkgs
diff --git a/src/recipe.rs b/src/recipe.rs
index 97dc847..230f685 100644
--- a/src/recipe.rs
+++ b/src/recipe.rs
@@ -353,7 +353,7 @@ impl<'src, D> Recipe<'src, D> {
     let tempdir = match &context.settings.tempdir {
       Some(tempdir) => tempdir_builder.tempdir_in(context.search.working_directory.join(tempdir)),
       None => {
-        if let Some(cache_dir) = dirs::cache_dir() {
+        if let Some(cache_dir) = dirs::runtime_dir() {
           let path = cache_dir.join("just");
           fs::create_dir_all(&path).map_err(|io_error| Error::CacheDirIo {
             io_error,
diff --git a/tests/tempdir.rs b/tests/tempdir.rs
index a7d2a5f..17a6f63 100644
--- a/tests/tempdir.rs
+++ b/tests/tempdir.rs
@@ -5,7 +5,7 @@ pub(crate) fn tempdir() -> TempDir {
 
   builder.prefix("just-test-tempdir");
 
-  if let Some(cache_dir) = dirs::cache_dir() {
+  if let Some(cache_dir) = dirs::runtime_dir() {
     let path = cache_dir.join("just");
     fs::create_dir_all(&path).unwrap();
     builder.tempdir_in(path)

@casey
Copy link
Owner

casey commented Jun 5, 2024

I just released 1.28, which uses the runtime dir, which seems like the correct choice anyways, since it should be cleared between restarts. Let me know if that works!

@vlaci
Copy link
Author

vlaci commented Jun 7, 2024

I just released 1.28, which uses the runtime dir, which seems like the correct choice anyways, since it should be cleared between restarts. Let me know if that works!

Everything looks good so far! 🚀

@casey
Copy link
Owner

casey commented Jun 7, 2024

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants