fix(xattr): apply --dirs directory xattrs fd-relative (#286)

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 : "<allocation failed>", 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 : "<allocation failed>");
+        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 : "<allocation failed>", 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 : "<allocation failed>", 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;

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

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();
 }