Add context to get_toml_path() error

[tmfink]

Handles #5188

Before Change

$ cargo b --bin rustfmt && HOME=/root ./target/debug/rustfmt --check /dev/null
Permission denied (os error 13)

After Change

$ cargo b --bin rustfmt && HOME=/root ./target/debug/rustfmt --check /dev/null
Failed to get metadata for config file "/root/.rustfmt.toml": Permission denied (os error 13)

[tmfink]

I avoided changing the signature of from_toml_path() by doing some gymnastics with std::io::Error.

Alternative approaches would be using a custom error enum (like thiserror) or Box<dyn Error> (like anyhow). But, these approaches would require modifying many other functions that transitively call from_toml_path() to return a different, compatible error type.

I leave it to the maintainers to decide what is preferred.

[ytmimi]

Thanks for submitting this PR!

The changes look good to me. I'm wondering if there's a way that we could write a test for the new error message. Any thoughts?

[tmfink]

The changes look good to me. I'm wondering if there's a way that we could write a test for the new error message. Any thoughts?

I considered writing a test, but I couldn't think of a way to test this in a portable way without overhauling more of the code.

We could just hope that a directory like /root exists with root permissions, but such tests would only work on some Unix-like systems (not all of them) that happen to have that directory with sufficient permissions. Such a test would only work on some systems with specific configurations. We could try to put logic in the test to only perform the actual test if there is a directory with appropriate owner/permissions, but that's kinda ugly.

We could also make the get_toml_path() generic so that we could provide a "fake" filesystem layer that returns an error of our choosing. But, I think the resulting code would be more difficult to maintain.

[ytmimi]

I considered writing a test, but I couldn't think of a way to test this in a portable way without overhauling more of the code.

I think portability is something we should strive for, but in this case I think it might be more practical to have platform specific implementations for these tests. Even if we only had a unix test (gated by #[cfg(unix)]) that would be a good starting point, and we could add tests for windows and mac later on.

Maybe we could create a temporary directory or file and then give ownership to root using something like nix::unistd::chown and then try to call rustfmt with --config-path to the temp rustfmt.toml file or directory?

[tmfink]

Maybe we could create a temporary directory or file and then give ownership to root using something like nix::unistd::chown and then try to call rustfmt with --config-path to the temp rustfmt.toml file or directory?

This wouldn't work. You can't set the owner to root if you are not root. If you could, then a user could accidentally change the owner of a directory to root but not be able to recover. Example with shell commands:

$ mkdir /tmp/test_a
$ chown root /tmp/test_a
chown: changing ownership of '/tmp/test_a': Operation not permitted

We essentially need a directory owned by root (or another user) that we don't have permissions to list. It would be fragile to assume /root is the root user's home directory, so we would need to use a function call like getpwuid_r() to get the path to the root user's home directory. As far as I can tell, there's no safe wrapper to getpwuid_r() available in the nix crate, so I would need to use a manual unsafe call to the function (setting up C-style NUL terminated strings and such).

Of course, there's no guarantee that the root user's home directory are such that we cannot list the contents. Instead of 700 (only owner can read/list) permissions, the permissions could be something like 750 (owner or group can read/list). On some systems, the root user may not have a home directory.

Due to these problems, I don't think it's worth writing a test. I could try to code some heuristics to exit the test early if the prerequisites are not there, but that seems fragile. If I don't test for the prerequisites correctly, the test could fail unexpectedly. It could seem like the requirements are met, but they are not and the get_toml_path() would not return an error.

[tmfink]

This is what a test could look like. I personally think adding this test is not worth the complexity/maintainability.
This test will fail on systems where none of the candidate directories exist with the necessary permissions.

diff --git a/src/config/mod.rs b/src/config/mod.rs
index 5041e1e3..dea534ad 100644
--- a/src/config/mod.rs
+++ b/src/config/mod.rs
@@ -25,6 +25,8 @@ pub(crate) mod file_lines;
 pub(crate) mod license;
 pub(crate) mod lists;
 
+const FAIL_METADATA_FOR_CONFIG_FILE: &str = "Failed to get metadata for config file";
+
 // This macro defines configuration options used in rustfmt. Each option
 // is defined as follows:
 //
@@ -364,7 +366,7 @@ fn get_toml_path(dir: &Path) -> Result<Option<PathBuf>, Error> {
             // find the project file yet, and continue searching.
             Err(e) => {
                 if e.kind() != ErrorKind::NotFound {
-                    let ctx = format!("Failed to get metadata for config file {:?}", &config_file);
+                    let ctx = format!("{} {:?}", FAIL_METADATA_FOR_CONFIG_FILE, &config_file);
                     let err = anyhow::Error::new(e).context(ctx);
                     return Err(Error::new(ErrorKind::Other, err));
                 }
@@ -663,6 +665,41 @@ make_backup = false
         assert_eq!(config.unstable_features(), true);
     }
 
+    #[cfg(target_os = "linux")]
+    #[test]
+    fn test_fail_access_config_dir() {
+        fn is_unlistable_directory<P: AsRef<Path>>(path: &P) -> bool {
+            let metadata = match fs::metadata(path) {
+                Err(err) => return err.kind() == ErrorKind::PermissionDenied,
+                Ok(metadata) => metadata,
+            };
+            if !metadata.is_dir() {
+                return false;
+            }
+            let mut child_path = path.as_ref().to_path_buf();
+            child_path.push("a");
+
+            // listing a child path should give us a permission error
+            match fs::metadata(child_path) {
+                Ok(_) => false,
+                Err(err) => err.kind() == ErrorKind::PermissionDenied,
+            }
+        }
+
+        let possible_root_dirs = ["/root", "/home/root", "/var/home/root"];
+        let root_dir = possible_root_dirs
+            .into_iter()
+            .find(is_unlistable_directory)
+            .expect("Unable to find root-owned, non-listable dir on this system; fix test");
+        let root_dir: PathBuf = root_dir
+            .parse()
+            .expect("Failed to convert dir path to PathBuf");
+
+        let err = get_toml_path(&root_dir).expect_err("Listing root-owned directory should fail");
+        let err_str = format!("{:#}", err);
+        assert!(err_str.contains(FAIL_METADATA_FOR_CONFIG_FILE))
+    }
+
     #[cfg(test)]
     mod deprecated_option_merge_imports {
         use super::*;

[ytmimi]

@tmfink I really appreciate your detailed explanation on all this. I don't do too much work on Linux day to day so that was very informative. I also appreciate you taking the time to come up with a test, even though as you mention it might not be reliable when run on all machines.

I think given your explanation and the complexity of setting up the test It's probably okay if we move forward on this without the test in place. @calebcartwright I'd also really like your opinion on this as well.

[calebcartwright]

Haven't had a chance to look at this yet (promise to do so soon) but wanted to add that I don't view a test as necessary for being able to move ahead.

Wouldn't hate having a test for things like error messages one day, but feel like it'd be more appropriate to test that specific piece via unit tests with appropriate mocks/stubs on the actual fs touch points. However, we're not really setup for that level of unit testing and I don't think the gymnastics that would be required to generate the message via more of an integration or functional style test are worth it

[calebcartwright]

Much better, thank you!