From b05c05eb4592533a35b21f79c522b8c87878cbf7 Mon Sep 17 00:00:00 2001 From: John Shin Date: Thu, 18 May 2023 18:25:41 -0700 Subject: [PATCH 1/5] mkdir: chmod of parent directories created by -p --- src/uu/mkdir/src/mkdir.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 9f490ecf9..f48f53bee 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -20,7 +20,7 @@ use uucore::mode; use uucore::{display::Quotable, fs::dir_strip_dot_for_creation}; use uucore::{format_usage, help_about, help_section, help_usage, show, show_if_err}; -static DEFAULT_PERM: u32 = 0o755; +static DEFAULT_PERM: u32 = 0o777; const ABOUT: &str = help_about!("mkdir.md"); const USAGE: &str = help_usage!("mkdir.md"); @@ -41,7 +41,7 @@ fn get_mode(_matches: &ArgMatches, _mode_had_minus_prefix: bool) -> Result Result { let digits: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; - // Translate a ~str in octal form to u16, default to 755 + // Translate a ~str in octal form to u16, default to 777 // Not tested on Windows let mut new_mode = DEFAULT_PERM; match matches.get_one::(options::MODE) { @@ -158,7 +158,7 @@ fn exec(dirs: ValuesRef, recursive: bool, mode: u32, verbose: bool) -> } fn mkdir(path: &Path, recursive: bool, mode: u32, verbose: bool) -> UResult<()> { - create_dir(path, recursive, verbose)?; + create_dir(path, recursive, verbose, false)?; chmod(path, mode) } @@ -179,7 +179,7 @@ fn chmod(_path: &Path, _mode: u32) -> UResult<()> { Ok(()) } -fn create_dir(path: &Path, recursive: bool, verbose: bool) -> UResult<()> { +fn create_dir(path: &Path, recursive: bool, verbose: bool, is_parent: bool) -> UResult<()> { if path.exists() && !recursive { return Err(USimpleError::new( 1, @@ -192,7 +192,7 @@ fn create_dir(path: &Path, recursive: bool, verbose: bool) -> UResult<()> { if recursive { match path.parent() { - Some(p) => create_dir(p, recursive, verbose)?, + Some(p) => create_dir(p, recursive, verbose, true)?, None => { USimpleError::new(1, "failed to create whole tree"); } @@ -207,6 +207,11 @@ fn create_dir(path: &Path, recursive: bool, verbose: bool) -> UResult<()> { path.quote() ); } + if is_parent { + // directories created by -p have permission bits set to '=rwx,u+wx', + // which is umask modified by 'u+wx' + chmod(path, (!mode::get_umask() & 0o0777) | 0o0300)?; + } Ok(()) } Err(_) if path.is_dir() => Ok(()), From 5e1a6c1f137f986a3fd089e5d08ce7696aceba33 Mon Sep 17 00:00:00 2001 From: John Shin Date: Thu, 18 May 2023 19:12:38 -0700 Subject: [PATCH 2/5] mkdir: add test for checking mode of parent dirs with -p --- tests/by-util/test_mkdir.rs | 68 ++++++++++++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index e2ac763b4..08b3cb1ed 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -71,6 +71,72 @@ fn test_mkdir_dup_dir_parent() { scene.ucmd().arg("-p").arg(TEST_DIR6).succeeds(); } +#[cfg(not(windows))] +#[test] +fn test_mkdir_parent_mode() { + let (at, mut ucmd) = at_and_ucmd!(); + + let default_umask: mode_t = 0o022; + let original_umask = unsafe { umask(default_umask) }; + + ucmd.arg("-p").arg("a/b").succeeds().no_stderr().no_stdout(); + + assert!(at.dir_exists("a")); + // parents created by -p have permissions set to "=rwx,u+wx" + assert_eq!( + at.metadata("a").permissions().mode(), + ((!default_umask & 0o777) | 0o300) + 0o40000 + ); + assert!(at.dir_exists("a/b")); + // sub directory's permission is determined only by the umask + assert_eq!( + at.metadata("a/b").permissions().mode(), + (!default_umask & 0o777) + 0o40000 + ); + + unsafe { + umask(original_umask); + } +} + +#[cfg(not(windows))] +#[test] +fn test_mkdir_parent_mode_check_existing_parent() { + let (at, mut ucmd) = at_and_ucmd!(); + + let default_umask: mode_t = 0o022; + let original_umask = unsafe { umask(default_umask) }; + + at.mkdir("a"); + + ucmd.arg("-p") + .arg("a/b/c") + .succeeds() + .no_stderr() + .no_stdout(); + + assert!(at.dir_exists("a")); + // parent dirs that already exist do not get their permissions modified + assert_eq!( + at.metadata("a").permissions().mode(), + (!default_umask & 0o777) + 0o40000 + ); + assert!(at.dir_exists("a/b")); + assert_eq!( + at.metadata("a/b").permissions().mode(), + ((!default_umask & 0o777) | 0o300) + 0o40000 + ); + assert!(at.dir_exists("a/b/c")); + assert_eq!( + at.metadata("a/b/c").permissions().mode(), + (!default_umask & 0o777) + 0o40000 + ); + + unsafe { + umask(original_umask); + } +} + #[test] fn test_mkdir_dup_file() { let scene = TestScenario::new(util_name!()); @@ -98,7 +164,7 @@ fn test_symbolic_alteration() { ucmd.arg("-m").arg("-w").arg(TEST_DIR1).succeeds(); let perms = at.metadata(TEST_DIR1).permissions().mode(); - assert_eq!(perms, 0o40555); + assert_eq!(perms, 0o40577); } #[test] From f592dff46a1e226480a19635abdadb18287ce7a1 Mon Sep 17 00:00:00 2001 From: John Shin Date: Thu, 18 May 2023 19:23:08 -0700 Subject: [PATCH 3/5] mkdir: skip setting dir permissions on windows --- src/uu/mkdir/src/mkdir.rs | 2 ++ tests/by-util/test_mkdir.rs | 10 +++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index f48f53bee..04281a455 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -179,6 +179,7 @@ fn chmod(_path: &Path, _mode: u32) -> UResult<()> { Ok(()) } +#[allow(unused_variables)] fn create_dir(path: &Path, recursive: bool, verbose: bool, is_parent: bool) -> UResult<()> { if path.exists() && !recursive { return Err(USimpleError::new( @@ -207,6 +208,7 @@ fn create_dir(path: &Path, recursive: bool, verbose: bool, is_parent: bool) -> U path.quote() ); } + #[cfg(not(windows))] if is_parent { // directories created by -p have permission bits set to '=rwx,u+wx', // which is umask modified by 'u+wx' diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index 08b3cb1ed..409334c2e 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -84,13 +84,13 @@ fn test_mkdir_parent_mode() { assert!(at.dir_exists("a")); // parents created by -p have permissions set to "=rwx,u+wx" assert_eq!( - at.metadata("a").permissions().mode(), + at.metadata("a").permissions().mode() as mode_t, ((!default_umask & 0o777) | 0o300) + 0o40000 ); assert!(at.dir_exists("a/b")); // sub directory's permission is determined only by the umask assert_eq!( - at.metadata("a/b").permissions().mode(), + at.metadata("a/b").permissions().mode() as mode_t, (!default_umask & 0o777) + 0o40000 ); @@ -118,17 +118,17 @@ fn test_mkdir_parent_mode_check_existing_parent() { assert!(at.dir_exists("a")); // parent dirs that already exist do not get their permissions modified assert_eq!( - at.metadata("a").permissions().mode(), + at.metadata("a").permissions().mode() as mode_t, (!default_umask & 0o777) + 0o40000 ); assert!(at.dir_exists("a/b")); assert_eq!( - at.metadata("a/b").permissions().mode(), + at.metadata("a/b").permissions().mode() as mode_t, ((!default_umask & 0o777) | 0o300) + 0o40000 ); assert!(at.dir_exists("a/b/c")); assert_eq!( - at.metadata("a/b/c").permissions().mode(), + at.metadata("a/b/c").permissions().mode() as mode_t, (!default_umask & 0o777) + 0o40000 ); From 9e8575dadd6501117147499728711211c1626a03 Mon Sep 17 00:00:00 2001 From: John Shin Date: Fri, 19 May 2023 08:13:33 -0700 Subject: [PATCH 4/5] mkdir: add documentation for the use of allow attribute --- src/uu/mkdir/src/mkdir.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 04281a455..a94439af5 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -179,6 +179,7 @@ fn chmod(_path: &Path, _mode: u32) -> UResult<()> { Ok(()) } +// `is_parent` argument is not used on windows #[allow(unused_variables)] fn create_dir(path: &Path, recursive: bool, verbose: bool, is_parent: bool) -> UResult<()> { if path.exists() && !recursive { From 33ac653ce54790e2a245bfaae3b05ed09835c0e2 Mon Sep 17 00:00:00 2001 From: John Shin Date: Sun, 21 May 2023 19:31:04 -0700 Subject: [PATCH 5/5] mkdir: correctly set umask so that tests fail without the fix --- tests/by-util/test_mkdir.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/by-util/test_mkdir.rs b/tests/by-util/test_mkdir.rs index 409334c2e..efa7c3248 100644 --- a/tests/by-util/test_mkdir.rs +++ b/tests/by-util/test_mkdir.rs @@ -76,7 +76,7 @@ fn test_mkdir_dup_dir_parent() { fn test_mkdir_parent_mode() { let (at, mut ucmd) = at_and_ucmd!(); - let default_umask: mode_t = 0o022; + let default_umask: mode_t = 0o160; let original_umask = unsafe { umask(default_umask) }; ucmd.arg("-p").arg("a/b").succeeds().no_stderr().no_stdout(); @@ -104,11 +104,11 @@ fn test_mkdir_parent_mode() { fn test_mkdir_parent_mode_check_existing_parent() { let (at, mut ucmd) = at_and_ucmd!(); - let default_umask: mode_t = 0o022; - let original_umask = unsafe { umask(default_umask) }; - at.mkdir("a"); + let default_umask: mode_t = 0o160; + let original_umask = unsafe { umask(default_umask) }; + ucmd.arg("-p") .arg("a/b/c") .succeeds() @@ -119,7 +119,7 @@ fn test_mkdir_parent_mode_check_existing_parent() { // parent dirs that already exist do not get their permissions modified assert_eq!( at.metadata("a").permissions().mode() as mode_t, - (!default_umask & 0o777) + 0o40000 + (!original_umask & 0o777) + 0o40000 ); assert!(at.dir_exists("a/b")); assert_eq!(