Skip to content

Commit 54877ac

Browse files
committed
uucore/mv: fd-based xattr copy for cross-device moves (#10014)
Add fsxattr::copy_xattrs_fd; mv's EXDEV fallback opens src/dst with O_NOFOLLOW via safe_copy and copies content + xattrs on the live fds, pinning both inodes against a concurrent renamer.
1 parent 515d74b commit 54877ac

5 files changed

Lines changed: 172 additions & 32 deletions

File tree

src/uu/mv/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ uucore = { workspace = true, features = [
3131
"fs",
3232
"fsxattr",
3333
"perms",
34+
"safe-copy",
3435
"update-control",
3536
] }
3637
fluent = { workspace = true }

src/uu/mv/src/mv.rs

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -921,7 +921,7 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
921921
}
922922
#[cfg(not(any(target_os = "macos", target_os = "redox")))]
923923
{
924-
let _ = copy_xattrs_if_supported(from, to);
924+
let _ = fsxattr::copy_xattrs(from, to);
925925
}
926926
let _ = preserve_ownership(from, to);
927927
fs::remove_file(from)
@@ -1250,7 +1250,7 @@ fn copy_file_with_hardlinks_helper(
12501250
// Copy xattrs, ignoring ENOTSUP errors (filesystem doesn't support xattrs)
12511251
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
12521252
{
1253-
let _ = copy_xattrs_if_supported(from, to);
1253+
let _ = fsxattr::copy_xattrs(from, to);
12541254
}
12551255
// Preserve ownership (uid/gid) from the source
12561256
let _ = preserve_ownership(from, to);
@@ -1293,20 +1293,41 @@ fn rename_file_fallback(
12931293
}
12941294
}
12951295

1296-
// Regular file copy
1297-
fs::copy(from, to)
1298-
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
1299-
1300-
// Copy xattrs, ignoring ENOTSUP errors (filesystem doesn't support xattrs)
1301-
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
1296+
// Open src/dst with O_NOFOLLOW and keep the fds alive across copy,
1297+
// chown, xattr, and chmod so a concurrent path-swap can't redirect any
1298+
// step to a different inode.
1299+
#[cfg(unix)]
13021300
{
1303-
let _ = copy_xattrs_if_supported(from, to);
1301+
use std::fs::Permissions;
1302+
use std::os::unix::fs::{MetadataExt, PermissionsExt};
1303+
use uucore::safe_copy::{create_dest_restrictive, open_source};
1304+
let src_file = open_source(from, /* nofollow */ true)
1305+
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
1306+
let src_mode = src_file
1307+
.metadata()
1308+
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?
1309+
.mode()
1310+
& 0o7777;
1311+
let mut dst_file = create_dest_restrictive(to, /* nofollow */ true)
1312+
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
1313+
io::copy(&mut &src_file, &mut dst_file)
1314+
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
1315+
1316+
#[cfg(not(any(target_os = "macos", target_os = "redox")))]
1317+
{
1318+
let _ = fsxattr::copy_xattrs_fd(&src_file, &dst_file);
1319+
}
1320+
1321+
// chown before chmod: chown(2) clears setuid/setgid for non-root,
1322+
// so the final mode must be applied last to preserve those bits.
1323+
let _ = preserve_ownership(from, to);
1324+
let _ = dst_file.set_permissions(Permissions::from_mode(src_mode));
13041325
}
13051326

1306-
// Preserve ownership (uid/gid) from the source file
1307-
#[cfg(unix)]
1327+
#[cfg(not(unix))]
13081328
{
1309-
let _ = preserve_ownership(from, to);
1329+
fs::copy(from, to)
1330+
.map_err(|err| io::Error::new(err.kind(), translate!("mv-error-permission-denied")))?;
13101331
}
13111332

13121333
fs::remove_file(from)
@@ -1351,18 +1372,6 @@ fn preserve_ownership(from: &Path, to: &Path) -> io::Result<()> {
13511372
Ok(())
13521373
}
13531374

1354-
/// Copy xattrs from source to destination, ignoring ENOTSUP/EOPNOTSUPP errors.
1355-
/// These errors indicate the filesystem doesn't support extended attributes,
1356-
/// which is acceptable when moving files across filesystems.
1357-
#[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
1358-
fn copy_xattrs_if_supported(from: &Path, to: &Path) -> io::Result<()> {
1359-
match fsxattr::copy_xattrs(from, to) {
1360-
Ok(()) => Ok(()),
1361-
Err(e) if e.raw_os_error() == Some(libc::EOPNOTSUPP) => Ok(()),
1362-
Err(e) => Err(e),
1363-
}
1364-
}
1365-
13661375
fn is_empty_dir(path: &Path) -> bool {
13671376
fs::read_dir(path).is_ok_and(|mut contents| contents.next().is_none())
13681377
}

src/uucore/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ extendedbigdecimal = ["bigdecimal", "num-traits"]
138138
fast-inc = []
139139
fs = ["dunce", "libc", "winapi-util", "windows-sys"]
140140
fsext = ["libc", "windows-sys", "bstr"]
141-
fsxattr = ["xattr", "itertools"]
141+
fsxattr = ["xattr", "itertools", "libc"]
142142
hardware = []
143143
lines = []
144144
feat_systemd_logind = ["utmpx", "libc"]

src/uucore/src/lib/features/fsxattr.rs

Lines changed: 90 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
// For the full copyright and license information, please view the LICENSE
44
// file that was distributed with this source code.
55

6-
// spell-checker:ignore getxattr posix_acl_default posix_acl_access
6+
// spell-checker:ignore getxattr posix_acl_default posix_acl_access ENOTSUP EOPNOTSUPP renamer
77

88
//! Set of functions to manage xattr on files and dirs
99
use itertools::Itertools;
@@ -13,20 +13,74 @@ use std::ffi::{OsStr, OsString};
1313
use std::os::unix::ffi::OsStrExt;
1414
use std::path::Path;
1515

16+
/// Returns true if the error is the kernel/filesystem signaling that
17+
/// extended attributes are not supported (`ENOTSUP` / `EOPNOTSUPP`).
18+
/// On Linux these are the same errno; on the BSDs they differ, so we
19+
/// match on either.
20+
#[cfg(unix)]
21+
fn is_xattr_unsupported(err: &std::io::Error) -> bool {
22+
matches!(
23+
err.raw_os_error(),
24+
Some(e) if e == libc::ENOTSUP || e == libc::EOPNOTSUPP
25+
)
26+
}
27+
28+
#[cfg(not(unix))]
29+
fn is_xattr_unsupported(_err: &std::io::Error) -> bool {
30+
false
31+
}
32+
1633
/// Copies extended attributes (xattrs) from one file or directory to another.
1734
///
35+
/// Returns `Ok(())` if the destination filesystem signals that xattrs are
36+
/// not supported (`ENOTSUP` / `EOPNOTSUPP`), since cross-filesystem moves
37+
/// onto e.g. tmpfs without xattr support are a legitimate scenario. All
38+
/// other errors propagate so the caller can surface (for example)
39+
/// permission failures on `security.*` namespaces.
40+
///
1841
/// # Arguments
1942
///
2043
/// * `source` - A reference to the source path.
2144
/// * `dest` - A reference to the destination path.
22-
///
23-
/// # Returns
24-
///
25-
/// A result indicating success or failure.
2645
pub fn copy_xattrs<P: AsRef<Path>>(source: P, dest: P) -> std::io::Result<()> {
27-
for attr_name in xattr::list(&source)? {
46+
let attrs = match xattr::list(&source) {
47+
Ok(a) => a,
48+
Err(e) if is_xattr_unsupported(&e) => return Ok(()),
49+
Err(e) => return Err(e),
50+
};
51+
for attr_name in attrs {
2852
if let Some(value) = xattr::get(&source, &attr_name)? {
29-
xattr::set(&dest, &attr_name, &value)?;
53+
if let Err(e) = xattr::set(&dest, &attr_name, &value) {
54+
if is_xattr_unsupported(&e) {
55+
return Ok(());
56+
}
57+
return Err(e);
58+
}
59+
}
60+
}
61+
Ok(())
62+
}
63+
64+
/// Copies xattrs between two open file descriptors. Pins both inodes so
65+
/// list/get/set calls cannot be redirected by a concurrent renamer, unlike
66+
/// the path-based [`copy_xattrs`]. `ENOTSUP` / `EOPNOTSUPP` is treated as
67+
/// success.
68+
#[cfg(unix)]
69+
pub fn copy_xattrs_fd(source: &std::fs::File, dest: &std::fs::File) -> std::io::Result<()> {
70+
use xattr::FileExt;
71+
let attrs = match source.list_xattr() {
72+
Ok(a) => a,
73+
Err(e) if is_xattr_unsupported(&e) => return Ok(()),
74+
Err(e) => return Err(e),
75+
};
76+
for attr_name in attrs {
77+
if let Some(value) = source.get_xattr(&attr_name)? {
78+
if let Err(e) = dest.set_xattr(&attr_name, &value) {
79+
if is_xattr_unsupported(&e) {
80+
return Ok(());
81+
}
82+
return Err(e);
83+
}
3084
}
3185
}
3286
Ok(())
@@ -222,6 +276,35 @@ mod tests {
222276
assert_eq!(copied_value, test_value);
223277
}
224278

279+
#[test]
280+
#[cfg(unix)]
281+
fn test_copy_xattrs_fd() {
282+
let temp_dir = tempdir().unwrap();
283+
let source_path = temp_dir.path().join("source.txt");
284+
let dest_path = temp_dir.path().join("dest.txt");
285+
286+
File::create(&source_path).unwrap();
287+
File::create(&dest_path).unwrap();
288+
289+
let test_attr = "user.fd_test";
290+
let test_value = b"fd value";
291+
// Skip silently if the test fs doesn't support user xattrs.
292+
if xattr::set(&source_path, test_attr, test_value).is_err() {
293+
return;
294+
}
295+
296+
let src = File::open(&source_path).unwrap();
297+
let dst = std::fs::OpenOptions::new()
298+
.write(true)
299+
.open(&dest_path)
300+
.unwrap();
301+
302+
copy_xattrs_fd(&src, &dst).unwrap();
303+
304+
let copied = xattr::get(&dest_path, test_attr).unwrap().unwrap();
305+
assert_eq!(copied, test_value);
306+
}
307+
225308
#[test]
226309
fn test_apply_and_retrieve_xattrs() {
227310
let temp_dir = tempdir().unwrap();

util/check-safe-traversal.sh

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,53 @@ if echo "$AVAILABLE_UTILS" | grep -q "cp"; then
269269
rm -f test_cp_src test_cp_dst
270270
fi
271271

272+
# mv cross-device (EXDEV) must use fd-based *xattr ops (issue #10014).
273+
if echo "$AVAILABLE_UTILS" | grep -q "mv" && [ -d /dev/shm ]; then
274+
# Need different filesystems for the EXDEV fallback to fire.
275+
temp_fs_id=$(stat -f -c %i "$TEMP_DIR" 2>/dev/null || echo "")
276+
shm_fs_id=$(stat -f -c %i /dev/shm 2>/dev/null || echo "")
277+
if [ -z "$temp_fs_id" ] || [ -z "$shm_fs_id" ] || [ "$temp_fs_id" = "$shm_fs_id" ]; then
278+
echo "WARN: mv cross-device xattr check: TMPDIR and /dev/shm are on the same filesystem; skipped"
279+
else
280+
shm_probe=$(mktemp -p /dev/shm mv_xattr_probe.XXXXXX)
281+
if setfattr -n user.probe -v ok "$shm_probe" 2>/dev/null; then
282+
rm -f "$shm_probe"
283+
cross_src=$(mktemp -p "$TEMP_DIR" cross_src.XXXXXX)
284+
cross_dst=$(mktemp -u -p /dev/shm cross_dst.XXXXXX)
285+
echo "cross-device payload" > "$cross_src"
286+
if setfattr -n user.tag -v pinned "$cross_src" 2>/dev/null; then
287+
if [ "$USE_MULTICALL" -eq 1 ]; then
288+
mv_cmd="$COREUTILS_BIN mv"
289+
else
290+
mv_cmd="$PROJECT_ROOT/target/${PROFILE}/mv"
291+
fi
292+
strace -f -e trace='%file,fgetxattr,fsetxattr,flistxattr,getxattr,setxattr,listxattr' \
293+
-o strace_mv_xattr.log \
294+
$mv_cmd "$cross_src" "$cross_dst" 2>/dev/null || true
295+
296+
# Path-based xattr calls on src/dst basenames = the TOCTOU pattern.
297+
cross_src_base=$(basename "$cross_src")
298+
cross_dst_base=$(basename "$cross_dst")
299+
if grep -qE "(listxattr|getxattr|setxattr)\([^,]*($cross_src_base|$cross_dst_base)" strace_mv_xattr.log; then
300+
cat strace_mv_xattr.log
301+
fail_immediately "mv cross-device must use fd-based xattr ops (issue #10014)"
302+
fi
303+
if grep -qE 'flistxattr|fgetxattr|fsetxattr' strace_mv_xattr.log; then
304+
echo "OK: mv cross-device uses fd-based xattr syscalls"
305+
else
306+
echo "WARN: mv cross-device xattr check: no xattr syscalls observed (xattr may have been filtered - check filesystem support)"
307+
fi
308+
rm -f "$cross_dst"
309+
else
310+
echo "WARN: mv cross-device xattr check: TMPDIR does not support user xattrs; skipped"
311+
fi
312+
else
313+
rm -f "$shm_probe"
314+
echo "WARN: mv cross-device xattr check: /dev/shm does not support user xattrs; skipped"
315+
fi
316+
fi
317+
fi
318+
272319
# mv cross-device symlink replacement must use *at syscalls against a
273320
# pinned parent fd (matches GNU's force_symlinkat) so a concurrent rename
274321
# of the parent directory cannot redirect the temp-and-rename dance, and

0 commit comments

Comments
 (0)