From c3c73c7d9d5b96958564b332435ee5a2ae0490a6 Mon Sep 17 00:00:00 2001 From: Abhinav Pangaria <127080854+18Abhinav07@users.noreply.github.com> Date: Mon, 26 Oct 2026 12:41:20 +0530 Subject: [PATCH 1/3] ls: only show "+" for a real ACL, and show it before "." `ls -l` marked a file with "." when it had a security context and "+" only when has_acl() was true. has_acl() treated any extended attribute as an ACL, so a security.selinux or security.SMACK64 label alone made it return true; and the label check ran first, so a file with both a label and an ACL showed ".". On Linux and Android has_acl() now counts only system.posix_acl_access, system.posix_acl_default and a non-trivial system.nfs4_acl (one naming a principal other than OWNER@, GROUP@ or EVERYONE@). ls checks the ACL first, then the label. Fixes #15809 --- src/uu/ls/src/display.rs | 33 ++++- src/uucore/src/lib/features/fsxattr.rs | 166 +++++++++++++++++++++++-- tests/by-util/test_ls.rs | 113 +++++++++++++++++ 3 files changed, 295 insertions(+), 17 deletions(-) diff --git a/src/uu/ls/src/display.rs b/src/uu/ls/src/display.rs index 5bb1e77344..b021db6ed9 100644 --- a/src/uu/ls/src/display.rs +++ b/src/uu/ls/src/display.rs @@ -314,14 +314,22 @@ fn pad_left(string: &str, count: usize) -> String { } /// Returns the alternate-access indicator that follows the 9-bit permission -/// string in long-format output: `.` for a non-trivial security context, `+` -/// for an ACL, otherwise a space (which acts as a placeholder so columns line -/// up across items that don't carry an indicator themselves). +/// string in long-format output: `+` for an ACL (even when the file also has a +/// security context), `.` for a non-trivial security context alone, otherwise a +/// space (which acts as a placeholder so columns line up across items that +/// don't carry an indicator themselves). fn alt_access_indicator(item: &PathData, config: &Config, is_acl_set: bool) -> u8 { - if item.security_context(config).len() > 1 { - b'.' - } else if is_acl_set { + alt_access_indicator_for(is_acl_set, item.security_context(config).len() > 1) +} + +/// The choice behind [`alt_access_indicator`]: an ACL wins over a security +/// context, because the label is itself an extended attribute and says nothing +/// about the ACL. +fn alt_access_indicator_for(has_acl: bool, has_context: bool) -> u8 { + if has_acl { b'+' + } else if has_context { + b'.' } else { b' ' } @@ -1423,3 +1431,16 @@ fn os_str_starts_with(haystack: &OsStr, needle: &[u8]) -> bool { fn write_os_str(writer: &mut W, string: &OsStr) -> std::io::Result<()> { writer.write_all(&os_str_as_bytes_lossy(string)) } + +#[cfg(test)] +mod tests { + use super::alt_access_indicator_for; + + #[test] + fn alt_access_indicator_prefers_acl_over_context() { + assert_eq!(alt_access_indicator_for(false, false), b' '); + assert_eq!(alt_access_indicator_for(false, true), b'.'); + assert_eq!(alt_access_indicator_for(true, false), b'+'); + assert_eq!(alt_access_indicator_for(true, true), b'+'); + } +} diff --git a/src/uucore/src/lib/features/fsxattr.rs b/src/uucore/src/lib/features/fsxattr.rs index a962718f4e..ad46744c6e 100644 --- a/src/uucore/src/lib/features/fsxattr.rs +++ b/src/uucore/src/lib/features/fsxattr.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore getxattr posix_acl_default posix_acl_access ENOTSUP EOPNOTSUPP renamer +// spell-checker:ignore getxattr posix_acl_default posix_acl_access nfs4_acl ENOTSUP EOPNOTSUPP renamer //! Set of functions to manage xattr on files and dirs @@ -249,8 +249,11 @@ pub fn apply_xattrs_fd_ignore_unsupported( /// Checks if a file has an Access Control List (ACL). /// -/// On macOS this asks the ACL API; elsewhere it looks at the file's extended -/// attributes. +/// On macOS this asks the ACL API. On Linux and Android it looks for the +/// attributes that store an ACL (for `NFSv4`, one that names someone besides the +/// owner, the group and everyone), so a security label (`security.selinux`) or a +/// `user.*` attribute alone is not an ACL. Elsewhere it looks at the file's +/// extended attributes. /// /// # Arguments /// @@ -259,14 +262,17 @@ pub fn apply_xattrs_fd_ignore_unsupported( /// /// # Returns /// -/// `true` if the file has an extended ACL (on macOS) or any extended attributes -/// (elsewhere), `false` otherwise. +/// `true` if the file has an extended ACL (on macOS, Linux and Android) or any +/// extended attributes (elsewhere), `false` otherwise. pub fn has_acl>(file: P, dereference: bool) -> bool { // On macOS most files carry some xattr (e.g. com.apple.provenance) and the // ACL is not one of them, so ask the ACL API instead. #[cfg(target_os = "macos")] return macos_acl::has_extended_acl(file.as_ref(), dereference); + #[cfg(not(target_os = "macos"))] + let file = file.as_ref(); + // don't use exacl here, it is doing more getxattr call then needed #[cfg(not(target_os = "macos"))] let attrs = if dereference { @@ -275,13 +281,75 @@ pub fn has_acl>(file: P, dereference: bool) -> bool { xattr::list(file) }; - #[cfg(not(target_os = "macos"))] - attrs.is_ok_and(|acl| { + // The attributes the kernel stores an ACL in. Other attributes, such as the + // SELinux or SMACK label, say nothing about an ACL. + #[cfg(any(target_os = "linux", target_os = "android"))] + return attrs.is_ok_and(|mut names| { + names.any(|name| { + if name == OsStr::new("system.posix_acl_access") + || name == OsStr::new("system.posix_acl_default") + { + return true; + } + if name != OsStr::new("system.nfs4_acl") { + return false; + } + // NFSv4 lists this attribute for every file, so look at its content. One that + // cannot be read is no evidence of an ACL. + let value = if dereference { + xattr::get_deref(file, &name) + } else { + xattr::get(file, &name) + }; + value + .ok() + .flatten() + .is_some_and(|raw| nfs4_acl_is_nontrivial(&raw)) + }) + }); + + #[cfg(not(any(target_os = "macos", target_os = "linux", target_os = "android")))] + attrs.is_ok_and(|mut acl| { // if we have extra attributes, we have an acl - acl.count() > 0 + acl.next().is_some() }) } +/// Whether an `NFSv4` ACL (the XDR value of `system.nfs4_acl`) names anyone besides +/// the owner, the group and everyone, which is all the mode bits can express. +/// A value that cannot be parsed counts as non-trivial. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn nfs4_acl_is_nontrivial(raw: &[u8]) -> bool { + // Big-endian 32-bit words; strings are length-prefixed and padded to 4 bytes. + fn word(raw: &mut &[u8]) -> Option { + let (head, rest) = raw.split_first_chunk::<4>()?; + *raw = rest; + usize::try_from(u32::from_be_bytes(*head)).ok() + } + + fn named_principal(mut raw: &[u8]) -> Option { + let count = word(&mut raw)?; + for _ in 0..count { + // ACE type, flags and access mask, then the principal. + let [_, _, _, len] = [ + word(&mut raw)?, + word(&mut raw)?, + word(&mut raw)?, + word(&mut raw)?, + ]; + let padded = len.checked_add(3)? & !3; + let who = raw.get(..len)?; + raw = raw.get(padded..)?; + if !matches!(who, b"OWNER@" | b"GROUP@" | b"EVERYONE@") { + return Some(true); + } + } + Some(false) + } + + named_principal(raw).unwrap_or(true) +} + #[cfg(target_os = "macos")] mod macos_acl { use std::ffi::{CString, c_char, c_int, c_void}; @@ -555,15 +623,22 @@ mod tests { File::create(&file_path).unwrap(); - // FIXME: this fails on a system that uses SELinux assert!(!has_acl(&file_path, true)); let test_attr = "user.test_acl"; let test_value = b"test value"; xattr::set(&file_path, test_attr, test_value).unwrap(); - // On macOS an ACL isn't an xattr, so an xattr alone is not an ACL. - assert_eq!(has_acl(&file_path, true), cfg!(not(target_os = "macos"))); + // An ACL isn't any xattr: on macOS it is not an xattr at all, and on + // Linux only the posix_acl/nfs4_acl attributes are ACLs. + assert_eq!( + has_acl(&file_path, true), + cfg!(not(any( + target_os = "macos", + target_os = "linux", + target_os = "android" + ))) + ); assert!(!has_security_cap_acl(&file_path, true)); // FreeBSD/NetBSD's xattr library does not support the "security" namespace @@ -580,6 +655,75 @@ mod tests { } } + #[test] + #[cfg(any(target_os = "linux", target_os = "android"))] + fn test_nfs4_acl_is_nontrivial() { + // count, then per entry: type, flags, access mask, principal (length, padded). + fn acl(principals: &[&str]) -> Vec { + let mut raw = u32::try_from(principals.len()) + .unwrap() + .to_be_bytes() + .to_vec(); + for who in principals { + for word in [0u32, 0, 0x1f, u32::try_from(who.len()).unwrap()] { + raw.extend(word.to_be_bytes()); + } + raw.extend(who.as_bytes()); + raw.resize(raw.len().next_multiple_of(4), 0); + } + raw + } + + assert!(!nfs4_acl_is_nontrivial(&acl(&[]))); + assert!(!nfs4_acl_is_nontrivial(&acl(&[ + "OWNER@", + "GROUP@", + "EVERYONE@" + ]))); + assert!(nfs4_acl_is_nontrivial(&acl(&[ + "OWNER@", + "alice@example.com", + "EVERYONE@" + ]))); + assert!(nfs4_acl_is_nontrivial(&acl(&["1000"]))); + + // Truncated or garbage values are not trusted to be trivial. + let full = acl(&["OWNER@", "GROUP@"]); + assert!(nfs4_acl_is_nontrivial(&full[..full.len() - 1])); + assert!(nfs4_acl_is_nontrivial(&[0, 0])); + assert!(nfs4_acl_is_nontrivial(&[0xff; 8])); + } + + #[test] + #[cfg(any(target_os = "linux", target_os = "android"))] + fn test_file_has_acl_posix_attribute() { + let temp_dir = tempdir().unwrap(); + let file_path = temp_dir.path().join("test_file.txt"); + File::create(&file_path).unwrap(); + + // An extended POSIX ACL in its xattr form: version 2, then (tag, perm, id) + // entries. The named user entry is what makes it more than the mode bits, + // which the kernel would not store as an ACL. + let mut acl = 2u32.to_le_bytes().to_vec(); + for (tag, perm, id) in [ + (0x01u16, 6u16, u32::MAX), // user::rw- + (0x02, 4, 1000), // user:1000:r-- + (0x04, 4, u32::MAX), // group::r-- + (0x10, 4, u32::MAX), // mask::r-- + (0x20, 4, u32::MAX), // other::r-- + ] { + acl.extend(tag.to_le_bytes()); + acl.extend(perm.to_le_bytes()); + acl.extend(id.to_le_bytes()); + } + // Not every file system stores ACLs (or lets the test set them). + if xattr::set(&file_path, "system.posix_acl_access", &acl).is_err() { + return; + } + assert!(has_acl(&file_path, true)); + assert!(has_acl(&file_path, false)); + } + #[test] fn test_apply_and_retrieve_xattrs_fd() { use std::fs::OpenOptions; diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 6b02b890d3..ce23fbbb52 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -5431,6 +5431,21 @@ fn test_ls_selinux_context_indicator() { ); } + // A file with an ACL as well as a context is listed with `+`: the context is + // an extended attribute too, so it must not hide the ACL. + at.touch("file_acl"); + if set_posix_acl(&at.plus_as_string("file_acl")) { + let result = scene.ucmd().args(&["-l", "file_acl"]).succeeds(); + let first_line = result.stdout_str().lines().next().unwrap().to_string(); + assert_eq!( + first_line.chars().nth(10), + Some('+'), + "Expected '+' indicator for an ACL plus SELinux context, got: {first_line}", + ); + } else { + println!("ACL plus context check skipped: cannot set a POSIX ACL here"); + } + // Test that ls -lnZ properly shows the context for file in ["file", "link"] { let result = scene.ucmd().args(&["-lnZ", file]).succeeds(); @@ -7182,6 +7197,104 @@ fn test_acl_display_macos() { .stdout_matches(&re_without_acl); } +/// Gives `path` a POSIX ACL that is more than its mode bits (a named user entry), +/// through the raw attribute so that only `setfattr` is needed. Returns false when +/// that is not possible here (no `setfattr`, or a file system without ACLs). +#[cfg(any(target_os = "linux", target_os = "android"))] +fn set_posix_acl(path: &str) -> bool { + use std::process::Command; + + // Version 2, then (tag, perm, id) entries: user::rw-, user:1000:r--, group::r--, + // mask::r--, other::r--. + let acl = "0x02000000\ + 01000600ffffffff\ + 02000400e8030000\ + 04000400ffffffff\ + 10000400ffffffff\ + 20000400ffffffff"; + Command::new("setfattr") + .args(["-n", "system.posix_acl_access", "-v", acl, path]) + .status() + .is_ok_and(|status| status.success()) +} + +// A real ACL is listed with `+`, so the check below that other attributes are not +// is not just passing because `+` never shows up. +#[cfg(any(target_os = "linux", target_os = "android"))] +#[test] +fn test_acl_display_raw_posix_acl() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("with_acl"); + at.touch("without_acl"); + + if !set_posix_acl(&at.plus_as_string("with_acl")) { + println!("test skipped: cannot set a POSIX ACL here"); + return; + } + + let result = scene + .ucmd() + .args(&["-l", "with_acl", "without_acl"]) + .succeeds(); + let out = result.stdout_str(); + assert!( + Regex::new(r"(?m)^-[rwx-]{9}\+ .*with_acl$") + .unwrap() + .is_match(out), + "{out}" + ); + assert!( + Regex::new(r"(?m)^-[rwx-]{9}[^+].*without_acl$") + .unwrap() + .is_match(out), + "{out}" + ); +} + +// An extended attribute that is not an ACL (a security label is one too) must not +// be listed with the ACL `+`. +#[cfg(any(target_os = "linux", target_os = "android"))] +#[test] +fn test_acl_display_ignores_other_xattrs() { + use std::process::Command; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.touch("with_xattr"); + + match Command::new("setfattr") + .args([ + "-n", + "user.test", + "-v", + "1", + &at.plus_as_string("with_xattr"), + ]) + .status() + .map(|status| status.code()) + { + Ok(Some(0)) => {} + Ok(_) => { + println!("test skipped: setfattr failed"); + return; + } + Err(e) => { + println!("test skipped: setfattr failed with {e}"); + return; + } + } + + // Whatever follows the mode bits (nothing, or `.` for a security context on + // an SELinux host), it is not the ACL `+`. + let re_without_acl = Regex::new(r"(?m)^-[rwx-]{9}[^+].*with_xattr$").unwrap(); + scene + .ucmd() + .args(&["-l", "with_xattr"]) + .succeeds() + .stdout_matches(&re_without_acl); +} + // Regression test for https://github.com/uutils/coreutils/issues/10980 // Each file with an ACL must not inflate the link-count column width. #[cfg(all(unix, not(target_vendor = "apple")))] From 034383cfdecec6b2340c9b5ae2121a59b2e89fc2 Mon Sep 17 00:00:00 2001 From: Abhinav Pangaria <127080854+18Abhinav07@users.noreply.github.com> Date: Tue, 27 Oct 2026 16:26:28 +0530 Subject: [PATCH 2/3] ls: simplify the ACL indicator and its tests per review Inline the precedence in alt_access_indicator and drop the extra function and its unit test, set ACLs with setfacl like the other tests, and share the command handling in one helper. --- src/uu/ls/src/display.rs | 30 +++-------------- tests/by-util/test_ls.rs | 69 ++++++++++++++++++---------------------- 2 files changed, 36 insertions(+), 63 deletions(-) diff --git a/src/uu/ls/src/display.rs b/src/uu/ls/src/display.rs index b021db6ed9..a30cd0279f 100644 --- a/src/uu/ls/src/display.rs +++ b/src/uu/ls/src/display.rs @@ -315,20 +315,13 @@ fn pad_left(string: &str, count: usize) -> String { /// Returns the alternate-access indicator that follows the 9-bit permission /// string in long-format output: `+` for an ACL (even when the file also has a -/// security context), `.` for a non-trivial security context alone, otherwise a -/// space (which acts as a placeholder so columns line up across items that -/// don't carry an indicator themselves). +/// security context, which is an extended attribute too), `.` for a non-trivial +/// security context alone, otherwise a space (which acts as a placeholder so +/// columns line up across items that don't carry an indicator themselves). fn alt_access_indicator(item: &PathData, config: &Config, is_acl_set: bool) -> u8 { - alt_access_indicator_for(is_acl_set, item.security_context(config).len() > 1) -} - -/// The choice behind [`alt_access_indicator`]: an ACL wins over a security -/// context, because the label is itself an extended attribute and says nothing -/// about the ACL. -fn alt_access_indicator_for(has_acl: bool, has_context: bool) -> u8 { - if has_acl { + if is_acl_set { b'+' - } else if has_context { + } else if item.security_context(config).len() > 1 { b'.' } else { b' ' @@ -1431,16 +1424,3 @@ fn os_str_starts_with(haystack: &OsStr, needle: &[u8]) -> bool { fn write_os_str(writer: &mut W, string: &OsStr) -> std::io::Result<()> { writer.write_all(&os_str_as_bytes_lossy(string)) } - -#[cfg(test)] -mod tests { - use super::alt_access_indicator_for; - - #[test] - fn alt_access_indicator_prefers_acl_over_context() { - assert_eq!(alt_access_indicator_for(false, false), b' '); - assert_eq!(alt_access_indicator_for(false, true), b'.'); - assert_eq!(alt_access_indicator_for(true, false), b'+'); - assert_eq!(alt_access_indicator_for(true, true), b'+'); - } -} diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index ce23fbbb52..6522649076 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -5434,7 +5434,10 @@ fn test_ls_selinux_context_indicator() { // A file with an ACL as well as a context is listed with `+`: the context is // an extended attribute too, so it must not hide the ACL. at.touch("file_acl"); - if set_posix_acl(&at.plus_as_string("file_acl")) { + if run_attr_tool( + "setfacl", + &["-m", "u:bin:r--", &at.plus_as_string("file_acl")], + ) { let result = scene.ucmd().args(&["-l", "file_acl"]).succeeds(); let first_line = result.stdout_str().lines().next().unwrap().to_string(); assert_eq!( @@ -5443,7 +5446,6 @@ fn test_ls_selinux_context_indicator() { "Expected '+' indicator for an ACL plus SELinux context, got: {first_line}", ); } else { - println!("ACL plus context check skipped: cannot set a POSIX ACL here"); } // Test that ls -lnZ properly shows the context @@ -7197,39 +7199,41 @@ fn test_acl_display_macos() { .stdout_matches(&re_without_acl); } -/// Gives `path` a POSIX ACL that is more than its mode bits (a named user entry), -/// through the raw attribute so that only `setfattr` is needed. Returns false when -/// that is not possible here (no `setfattr`, or a file system without ACLs). +/// Runs an attribute tool (`setfacl`, `setfattr`) on a test file. Returns false, after +/// saying why, when it cannot be used here, so that the caller can skip its test. #[cfg(any(target_os = "linux", target_os = "android"))] -fn set_posix_acl(path: &str) -> bool { +fn run_attr_tool(tool: &str, args: &[&str]) -> bool { use std::process::Command; - // Version 2, then (tag, perm, id) entries: user::rw-, user:1000:r--, group::r--, - // mask::r--, other::r--. - let acl = "0x02000000\ - 01000600ffffffff\ - 02000400e8030000\ - 04000400ffffffff\ - 10000400ffffffff\ - 20000400ffffffff"; - Command::new("setfattr") - .args(["-n", "system.posix_acl_access", "-v", acl, path]) - .status() - .is_ok_and(|status| status.success()) + // calling the command directly. xattr requires some dev packages to be installed + // and it adds a complex dependency just for a test + match Command::new(tool).args(args).status().map(|s| s.code()) { + Ok(Some(0)) => true, + Ok(_) => { + println!("test skipped: {tool} failed"); + false + } + Err(e) => { + println!("test skipped: {tool} failed with {e}"); + false + } + } } // A real ACL is listed with `+`, so the check below that other attributes are not // is not just passing because `+` never shows up. #[cfg(any(target_os = "linux", target_os = "android"))] #[test] -fn test_acl_display_raw_posix_acl() { +fn test_acl_display_access_acl_on_file() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.touch("with_acl"); at.touch("without_acl"); - if !set_posix_acl(&at.plus_as_string("with_acl")) { - println!("test skipped: cannot set a POSIX ACL here"); + if !run_attr_tool( + "setfacl", + &["-m", "u:bin:r--", &at.plus_as_string("with_acl")], + ) { return; } @@ -7257,32 +7261,21 @@ fn test_acl_display_raw_posix_acl() { #[cfg(any(target_os = "linux", target_os = "android"))] #[test] fn test_acl_display_ignores_other_xattrs() { - use std::process::Command; - let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.touch("with_xattr"); - match Command::new("setfattr") - .args([ + if !run_attr_tool( + "setfattr", + &[ "-n", "user.test", "-v", "1", &at.plus_as_string("with_xattr"), - ]) - .status() - .map(|status| status.code()) - { - Ok(Some(0)) => {} - Ok(_) => { - println!("test skipped: setfattr failed"); - return; - } - Err(e) => { - println!("test skipped: setfattr failed with {e}"); - return; - } + ], + ) { + return; } // Whatever follows the mode bits (nothing, or `.` for a security context on From b2c415e5cdc70466bab9f09ab2dd3dd7662478a8 Mon Sep 17 00:00:00 2001 From: Abhinav Pangaria <127080854+18Abhinav07@users.noreply.github.com> Date: Fri, 30 Oct 2026 10:44:07 +0530 Subject: [PATCH 3/3] ls: drop an empty else left in the SELinux indicator test --- tests/by-util/test_ls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 6522649076..8f3f7b0c2e 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -5445,7 +5445,6 @@ fn test_ls_selinux_context_indicator() { Some('+'), "Expected '+' indicator for an ACL plus SELinux context, got: {first_line}", ); - } else { } // Test that ls -lnZ properly shows the context