From 3787695ba66d1c9501ece213427df01916d2d1cd Mon Sep 17 00:00:00 2001 From: TapTap Date: Tue, 22 Sep 2026 16:05:18 +0200 Subject: [PATCH] fix(xattr): apply --dirs directory xattrs fd-relative (#286) --- src/shared/file_save.c | 92 ++++++++++++++++++++++++++++-- tests/integration/test_features.py | 25 ++++++++ tests/test_xattr.c | 50 ++++++++++++++++ 3 files changed, 161 insertions(+), 6 deletions(-) diff --git a/src/shared/file_save.c b/src/shared/file_save.c index 98000ae..d207968 100644 --- a/src/shared/file_save.c +++ b/src/shared/file_save.c @@ -714,27 +714,107 @@ static FileSaveResult file_save_directory_to_disk(const FileSavePlan* plan, bool return FILE_SAVE_ERROR; bool dir_existed = file_path_exists_secure(dir_path); bool ok = file_ensure_directory_secure(dir_path); + /* One confined, no-follow descriptor drives ownership/mode/xattr/timestamp + application so none of them can follow a same-named symlink planted after + the mkdir. This mirrors the O_DIRECTORY|O_NOFOLLOW fd that + dir_metadata_list_apply() opens for the recursive path; the fd is reached + through the already-confined parent. */ + char* leaf = NULL; + int parent_fd = -1; + int dir_fd = -1; + if (ok) { + parent_fd = file_open_secure_parent(dir_path, &leaf, false); + if (parent_fd >= 0) + dir_fd = openat(parent_fd, leaf, O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC); + } /* P7 Wave E: apply the negotiated ownership to the directory ITSELF (not just the files inside it). --copy-as and every explicit identity policy own every entry, so a directory must not keep the receiver's owner while its children get the policy owner. Applied no-follow on the confined - parent fd after the mkdir; identity_apply_ownership_link() is itself a - no-op unless an identity policy is active. */ + parent fd; identity_apply_ownership_link() is itself a no-op unless an + identity policy is active. Ownership runs before the mode because a chown + clears setuid/setgid. A failed REQUIRED --copy-as ownership fails the + entry; every other policy stays best-effort. */ if (ok && file->metadata && identity_active_enabled()) { - char* leaf = NULL; - int parent_fd = file_open_secure_parent(dir_path, &leaf, false); if (parent_fd >= 0) { if (!identity_apply_ownership_link(parent_fd, leaf, (int32_t)file->metadata->uid, (int32_t)file->metadata->gid)) ok = false; - close(parent_fd); } else if (identity_copy_as_active()) { /* The directory exists (ok) but its required --copy-as ownership could not be applied because the confined parent could not be opened. */ ok = false; } - free(leaf); + } else if (ok && identity_copy_as_active()) { + ok = false; } + /* Mode next: fchmod also rewrites the ACL mask, so the xattrs/ACLs below must + follow it. The --chmod/permission-bits handling matches the recursive + dir_metadata_list_apply() path exactly. */ + if (ok && file->metadata && plan->config && plan->config->preserve_perms) { + const Config* config = plan->config; + if (dir_fd < 0) { + char* escaped_path = output_escape(dir_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_WARNING, "Failed to open directory %s to set its mode: %s", + escaped_path ? escaped_path : "", strerror(errno)); + free(escaped_path); + } else { + mode_t dir_mode = file->metadata->mode; + bool mode_ready = true; + if (config->chmod_spec && *config->chmod_spec && + !chmod_apply(dir_mode, config->chmod_spec, &dir_mode)) { + char* escaped_path = output_escape(dir_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_WARNING, "Failed to apply --chmod to directory %s", + escaped_path ? escaped_path : ""); + free(escaped_path); + mode_ready = false; + } + if (mode_ready) { + /* rsync -p copies the source directory mode exactly, including + group/other write and the setgid/sticky bits. Setuid/setgid/sticky + are super-user activities: when the connection forbade them + (SUPER_MODE_OFF / --no-super), strip them even under -p. */ + mode_t safe_mode = dir_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777); + if (!privilege_super_mode_permitted(config->super_mode)) + safe_mode &= ~(mode_t)(S_ISUID | S_ISGID | S_ISVTX); + if (fchmod(dir_fd, safe_mode) != 0) { + char* escaped_path = output_escape(dir_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_WARNING, "Failed to set directory mode on %s: %s", + escaped_path ? escaped_path : "", strerror(errno)); + free(escaped_path); + } + } + } + } + /* xattrs/ACLs after fchmod (the mode change can rewrite the ACL mask; the + ACL xattrs must be (re)applied last). Best-effort: a per-attribute failure + is logged and skipped by xattr_apply_fd(), never fatal. */ + if (ok && plan->config && plan->config->use_xattrs && dir_fd >= 0 && file->xattrs) + xattr_apply_fd(dir_fd, file->xattrs); + /* Timestamps last so no later chmod/xattr is mistaken for a content update. + -J/--omit-dir-times suppresses the directory mtime; --atimes/-U applies + only when the source atime is valid, exactly as the recursive path. */ + if (ok && file->metadata && plan->config && plan->config->preserve_times && + !plan->config->omit_dir_times) { + struct timespec times[2] = { + {.tv_sec = 0, .tv_nsec = UTIME_OMIT}, + {.tv_sec = file->metadata->mtime_sec, .tv_nsec = file->metadata->mtime_nsec}}; + if (plan->config->preserve_atimes && file->metadata->atime_valid) { + times[0].tv_sec = file->metadata->atime_sec; + times[0].tv_nsec = file->metadata->atime_nsec; + } + if (parent_fd >= 0 && utimensat(parent_fd, leaf, times, AT_SYMLINK_NOFOLLOW) != 0) { + char* escaped_path = output_escape(dir_path, log_get_8_bit_output()); + log_message(LOG_LEVEL_WARNING, "Failed to set directory timestamps on %s: %s", + escaped_path ? escaped_path : "", strerror(errno)); + free(escaped_path); + } + } + if (dir_fd >= 0) + close(dir_fd); + if (parent_fd >= 0) + close(parent_fd); + free(leaf); free(dir_path); if (ok && created && !dir_existed) *created = true; diff --git a/tests/integration/test_features.py b/tests/integration/test_features.py index e60d8c7..be64da9 100644 --- a/tests/integration/test_features.py +++ b/tests/integration/test_features.py @@ -6750,6 +6750,31 @@ class TestExtendedAttributes: assert os.getxattr(received, "user.rootdir") == b"r" assert os.getxattr(os.path.join(received, "sub"), "user.subdir") == b"s" + @pytest.mark.ci + @pytest.mark.parametrize("mt", [False, True]) + def test_dirs_directory_xattr_applied(self, shared_server, mt): + """#286.3: -d/-X must apply a transferred directory's user.* xattr at the + destination through the --dirs STATUS_MKDIR path (both the + single-threaded and -m/--threads receiver paths).""" + source, dest = self._source_and_dest("dirsxattr") + sub = os.path.join(source, "sub") + os.makedirs(sub) + if not _xattr_supported(sub): + pytest.skip("filesystem does not support user xattrs") + os.setxattr(sub, "user.dirsdir", b"dirs-value") + lst = os.path.join(TEST_DATA_DIR, "dirs_xattr_list.txt") + with open(lst, "wb") as fh: + fh.write(b"sub\n") + + flags = ["--files-from", lst, "--dirs", "-R", "-X"] + (["--threads"] if mt else []) + result, _ = run_client(source, dest, flags=flags, port=shared_server.port) + assert result.returncode == 0, \ + f"--dirs -X sync failed: {(result.stderr or result.stdout)[:300]}" + received = os.path.join(dest, "sub") + assert os.path.isdir(received), "--dirs directory entry was not created" + assert os.getxattr(received, "user.dirsdir") == b"dirs-value", \ + "the --dirs directory's user.* xattr was not applied at the destination" + @pytest.mark.ci def test_directory_default_acl_preserved(self, shared_server): """#286.3: -aA must preserve a directory's default POSIX ACL (the diff --git a/tests/test_xattr.c b/tests/test_xattr.c index 10447a3..96802f7 100644 --- a/tests/test_xattr.c +++ b/tests/test_xattr.c @@ -2,6 +2,7 @@ #include "xattr.h" #include "config.h" #include "file.h" +#include "file_save.h" #include "identity.h" #include "protocol.h" #include "test_utils.h" @@ -540,6 +541,54 @@ static void test_xattr_list_clone() { xattr_list_free(clone); } +/* #286.3: an explicit directory entry (--dirs, STATUS_MKDIR) that carries a + * captured user.* xattr must have it applied fd-relative by the directory + * install path itself -- not only by the receiver's deferred DirTimeList, which + * a direct file_save_to_disk_full() caller does not use. */ +static void test_file_save_directory_applies_xattrs() { + const char* root = "test_save_dir_xattr_tmp"; + const char* leaf = "subdir"; + const char* path = "test_save_dir_xattr_tmp/subdir"; + rmdir(path); + rmdir(root); + EXPECT_EQ_INT(mkdir(root, 0700), 0); + /* The working directory may be a filesystem without user xattrs (e.g. some + tmpfs mounts): skip cleanly rather than fail the suite. */ + if (setxattr(root, "user.fastsync-dirprobe", "p", 1, 0) != 0) { + rmdir(root); + return; + } + removexattr(root, "user.fastsync-dirprobe"); + + File* dir = file_create(leaf); + EXPECT_NOT_NULL(dir); + dir->is_dir = true; + FileXattrList* xattrs = xattr_list_new(); + EXPECT_NOT_NULL(xattrs); + EXPECT_TRUE(xattr_list_append(xattrs, "user.dirxattr", "dirvalue", 8)); + dir->xattrs = xattrs; + + Config* config = config_create(); + EXPECT_NOT_NULL(config); + config->use_metadata = true; + config->use_xattrs = true; + config->preserve_xattrs = true; + + EXPECT_EQ_INT(file_save_to_disk_full(root, dir, config), FILE_SAVE_WRITTEN); + EXPECT_EQ_INT(access(path, F_OK), 0); + + char value[32]; + ssize_t got = getxattr(path, "user.dirxattr", value, sizeof(value)); + EXPECT_EQ_INT((int)got, 8); + EXPECT_TRUE(got == 8 && memcmp(value, "dirvalue", 8) == 0); + + file_destroy(dir); + config_delete(config); + removexattr(path, "user.dirxattr"); + rmdir(path); + rmdir(root); +} + void test_xattr() { test_xattr_list_clone(); test_xattr_wire_roundtrip(); @@ -553,4 +602,5 @@ void test_xattr() { test_fake_super_restore(); test_fake_super_no_real_chown(); test_fake_super_storage_resolution(); + test_file_save_directory_applies_xattrs(); } \ No newline at end of file