chmod: match rsync 3.4.1 --chmod and remove mode masking (#293)

- --chmod no longer implies --preserve-perms; repeated --chmod options
  accumulate, and D/F/X selectors plus s/t special bits are supported with
  rsync's exact parse_chmod/tweak_mode semantics.
- Stop masking group/other write and setuid/setgid/sticky: -p copies the
  source mode exactly, no-p new entries use source&~umask, directories keep
  setgid/sticky, and special nodes follow the same rules.
- Apply ownership before mode on the fd path so a chown cannot clear the
  setuid/setgid bits -p just restored (rsync order).
- Update unit and integration tests, including differential checks against
  rsync 3.4.1.

src/client/client_cli.c

@@ -117,6 +117,27 @@ static int set_string_option(char** dest, const char* value, const char* option_
   return 0;
 }
 
+/* Append a --chmod clause list to the accumulated spec with a comma.  rsync
+ * 3.2.4+ makes repeated --chmod options cumulative, so they must not replace
+ * the previous ones.  Returns 0 on success, -1 on failure. */
+static int append_chmod_spec(char** dest, const char* value) {
+  if (!*dest)
+    return set_string_option(dest, value, "--chmod");
+  size_t old_len = strlen(*dest);
+  size_t add_len = strlen(value);
+  char* merged = malloc(old_len + add_len + 2);
+  if (!merged) {
+    log_message(LOG_LEVEL_ERROR, "memory allocation failed for --chmod");
+    return -1;
+  }
+  memcpy(merged, *dest, old_len);
+  merged[old_len] = ',';
+  memcpy(merged + old_len + 1, value, add_len + 1);
+  free(*dest);
+  *dest = merged;
+  return 0;
+}
+
 /* Parse a string as a positive integer into *dest. Returns 0 on success, -1 on error. */
 static int set_positive_int_option(int* dest, const char* value, const char* option_name) {
   if (!parse_positive_int(value, dest)) {
@@ -962,6 +983,8 @@ static int apply_table_option(Config* config, const OptionEntry* entry, const ch
   case OPT_NOOP:
     return 0;
   case OPT_STRING:
+    if (entry->offset == offsetof(Config, chmod_spec))
+      return append_chmod_spec((char**)field, value);
     return set_string_option((char**)field, value, entry->name);
   case OPT_POS_INT:
     return set_positive_int_option((int*)field, value, entry->name);
@@ -1220,7 +1243,6 @@ static bool cli_handle_table_option(CliParseCtx* ctx) {
           ctx->exit_code = -1;
           return true;
         }
-        config->preserve_perms = true;
       }
       /* Remember that --server-host was explicitly given (the field itself
          defaults to 127.0.0.1, so a value check cannot distinguish it).  Used
@@ -1267,7 +1289,7 @@ static bool cli_handle_inline_chmod(CliParseCtx* ctx) {
   const char* arg = ctx->argv[ctx->i];
   if (strncmp(arg, "--chmod=", 8) != 0)
     return false;
-  if (set_string_option(&config->chmod_spec, arg + 8, "--chmod") != 0) {
+  if (append_chmod_spec(&config->chmod_spec, arg + 8) != 0) {
     ctx->exit_code = -1;
     return true;
   }
@@ -1277,7 +1299,6 @@ static bool cli_handle_inline_chmod(CliParseCtx* ctx) {
     ctx->exit_code = -1;
     return true;
   }
-  config->preserve_perms = true;
   return true;
 }
 

src/client/usage.c

@@ -202,7 +202,8 @@ void print_usage(void) {
   printf("                      --copy-as); --numeric-ids only changes how ids map\n");
   printf("  --no-super          Forbid those super-user activities even when the\n");
   printf("                      receiver is running as root\n");
-  printf("  --chmod <changes>   Modify transferred permissions (rsync syntax)\n");
+  printf(
+      "  --chmod <changes>   Modify new/transferred permissions (rsync syntax; implies no -p)\n");
   printf("  --numeric-ids       Map uid/gid by id instead of by name (a modifier, not\n");
   printf("                      an ownership request: combine with -o/-g or a map)\n");
   printf("  --usermap=MAP       Map usernames when applying ownership: comma-separated\n");

src/shared/chmod.c

@@ -1,90 +1,183 @@
 #include "chmod.h"
+#include "file.h"
 #include <stddef.h>
 #include <string.h>
 
-static bool parse_clause(mode_t* mode, const char* begin, const char* end) {
-  const char* p = begin;
-  unsigned who = 0;
-  while (p < end && strchr("ugoa", *p)) {
-    if (*p == 'a')
-      who = 7;
-    else
-      who |= *p == 'u' ? 1U : (*p == 'g' ? 2U : 4U);
-    p++;
-  }
-  if (who == 0)
-    who = 7;
-  if (p == end || (*p != '+' && *p != '-' && *p != '='))
-    return false;
-  char operation = *p++;
-  mode_t bits = 0;
-  while (p < end) {
-    mode_t bit;
-    switch (*p++) {
-    case 'r':
-      bit = 4;
-      break;
-    case 'w':
-      bit = 2;
-      break;
-    case 'x':
-      bit = 1;
-      break;
-    default:
-      return false;
-    }
-    bits |= bit;
-  }
-  for (unsigned class_index = 0; class_index < 3; class_index++) {
-    unsigned class_bit = 1U << class_index;
-    if (!(who & class_bit))
-      continue;
-    mode_t shift = (mode_t)((2U - class_index) * 3U);
-    mode_t mask = (mode_t)(7U << shift);
-    mode_t class_bits = (mode_t)(bits << shift);
-    if (operation == '+')
-      *mode |= class_bits;
-    else if (operation == '-')
-      *mode &= ~class_bits;
-    else
-      *mode = (*mode & ~mask) | class_bits;
-  }
-  return true;
-}
+/* rsync's --chmod parser (parse_chmod + tweak_mode).  A single clause is
+ * applied as it is completed, so repeated clauses and repeated --chmod options
+ * (joined with commas by the CLI) accumulate exactly like rsync.  The D/F
+ * selectors restrict a clause to directories/files; X adds execute only to
+ * directories or files that were already executable. */
+
+#define CHMOD_BITS 07777
+#define CHMOD_FLAG_X_KEEP (1U << 0)
+#define CHMOD_FLAG_DIRS_ONLY (1U << 1)
+#define CHMOD_FLAG_FILES_ONLY (1U << 2)
+
+enum chmod_op { CHMOD_OP_ADD = 1, CHMOD_OP_SUB, CHMOD_OP_EQ, CHMOD_OP_SET };
+enum chmod_state {
+  CHMOD_STATE_ERROR,
+  CHMOD_STATE_1ST_HALF,
+  CHMOD_STATE_2ND_HALF,
+  CHMOD_STATE_OCTAL
+};
 
 bool chmod_apply(mode_t mode, const char* spec, mode_t* result) {
   if (!spec || !*spec || !result)
     return false;
-  bool numeric = true;
-  size_t length = strlen(spec);
-  if (length > 4)
-    numeric = false;
-  for (size_t i = 0; i < length && numeric; i++)
-    numeric = spec[i] >= '0' && spec[i] <= '7';
-  if (numeric) {
-    if (length == 0 || length > 4)
-      return false;
-    mode_t parsed = 0;
-    for (size_t i = 0; i < length; i++)
-      parsed = (mode_t)((parsed << 3) | (spec[i] - '0'));
-    *result = parsed;
-    return true;
-  }
-
+  const mode_t nonperm = mode & ~(mode_t)CHMOD_BITS;
+  const bool initially_executable = (mode & 0111) != 0;
   mode_t changed = mode;
-  const char* begin = spec;
-  while (*begin) {
-    const char* end = strchr(begin, ',');
-    if (!end)
-      end = begin + strlen(begin);
-    if (!parse_clause(&changed, begin, end))
-      return false;
-    if (*end == '\0')
+  int state = CHMOD_STATE_1ST_HALF;
+  unsigned where = 0;
+  int what = 0, op = 0, topbits = 0, topoct = 0, flags = 0;
+  const char* p = spec;
+  while (state != CHMOD_STATE_ERROR) {
+    if (*p == '\0' || *p == ',') {
+      int bits;
+      if (!op) {
+        state = CHMOD_STATE_ERROR;
+        break;
+      }
+      if (where)
+        bits = (int)(where * (unsigned)what);
+      else {
+        where = 0111;
+        bits = (int)((where * (unsigned)what) & ~(unsigned)file_process_umask());
+      }
+      int mode_and, mode_or;
+      switch (op) {
+      case CHMOD_OP_ADD:
+        mode_and = CHMOD_BITS;
+        mode_or = bits + topoct;
+        break;
+      case CHMOD_OP_SUB:
+        mode_and = CHMOD_BITS - bits - topoct;
+        mode_or = 0;
+        break;
+      case CHMOD_OP_EQ:
+        mode_and = CHMOD_BITS - (int)(where * 7U) - (topoct ? topbits : 0);
+        mode_or = bits + topoct;
+        break;
+      default:
+        mode_and = 0;
+        mode_or = bits;
+        break;
+      }
+      bool is_dir = S_ISDIR(nonperm);
+      if (!((flags & CHMOD_FLAG_DIRS_ONLY) && !is_dir) &&
+          !((flags & CHMOD_FLAG_FILES_ONLY) && is_dir)) {
+        changed &= (mode_t)mode_and;
+        if ((flags & CHMOD_FLAG_X_KEEP) && !initially_executable && !is_dir)
+          changed |= (mode_t)(mode_or & ~0111);
+        else
+          changed |= (mode_t)mode_or;
+      }
+      if (*p == '\0')
+        break;
+      p++;
+      state = CHMOD_STATE_1ST_HALF;
+      where = 0;
+      what = op = topoct = topbits = flags = 0;
+      continue;
+    }
+    switch (state) {
+    case CHMOD_STATE_1ST_HALF:
+      switch (*p) {
+      case 'D':
+        if (flags & CHMOD_FLAG_FILES_ONLY) {
+          state = CHMOD_STATE_ERROR;
+          break;
+        }
+        flags |= CHMOD_FLAG_DIRS_ONLY;
+        break;
+      case 'F':
+        if (flags & CHMOD_FLAG_DIRS_ONLY) {
+          state = CHMOD_STATE_ERROR;
+          break;
+        }
+        flags |= CHMOD_FLAG_FILES_ONLY;
+        break;
+      case 'u':
+        where |= 0100;
+        topbits |= 04000;
+        break;
+      case 'g':
+        where |= 0010;
+        topbits |= 02000;
+        break;
+      case 'o':
+        where |= 0001;
+        break;
+      case 'a':
+        where |= 0111;
+        break;
+      case '+':
+        op = CHMOD_OP_ADD;
+        state = CHMOD_STATE_2ND_HALF;
+        break;
+      case '-':
+        op = CHMOD_OP_SUB;
+        state = CHMOD_STATE_2ND_HALF;
+        break;
+      case '=':
+        op = CHMOD_OP_EQ;
+        state = CHMOD_STATE_2ND_HALF;
+        break;
+      default:
+        if (*p >= '0' && *p <= '7' && !where) {
+          op = CHMOD_OP_SET;
+          state = CHMOD_STATE_OCTAL;
+          where = 1;
+          what = *p - '0';
+        } else {
+          state = CHMOD_STATE_ERROR;
+        }
+        break;
+      }
       break;
-    begin = end + 1;
-    if (!*begin)
-      return false;
+    case CHMOD_STATE_2ND_HALF:
+      switch (*p) {
+      case 'r':
+        what |= 4;
+        break;
+      case 'w':
+        what |= 2;
+        break;
+      case 'X':
+        flags |= CHMOD_FLAG_X_KEEP;
+        /* fall through */
+      case 'x':
+        what |= 1;
+        break;
+      case 's':
+        if (topbits)
+          topoct |= topbits;
+        else
+          topoct = 04000;
+        break;
+      case 't':
+        topoct |= 01000;
+        break;
+      default:
+        state = CHMOD_STATE_ERROR;
+        break;
+      }
+      break;
+    default:
+      if (*p >= '0' && *p <= '7') {
+        what = what * 8 + (*p - '0');
+        if (what > CHMOD_BITS)
+          state = CHMOD_STATE_ERROR;
+      } else {
+        state = CHMOD_STATE_ERROR;
+      }
+      break;
+    }
+    p++;
   }
-  *result = changed;
+  if (state == CHMOD_STATE_ERROR)
+    return false;
+  *result = (changed & (mode_t)CHMOD_BITS) | nonperm;
   return true;
 }

src/shared/chmod.h

@@ -4,7 +4,10 @@
 #include <stdbool.h>
 #include <sys/stat.h>
 
-/* Apply the supported rsync --chmod syntax to a permission mode. */
+/* Apply rsync's --chmod syntax to a permission mode, including the D/F/X
+ * selectors and the s/t special bits.  `mode` should carry the file type bits
+ * (S_IFDIR/S_IFREG) so D/F/X can be evaluated; the type bits are preserved in
+ * `result`.  A spec may contain comma-separated clauses, which accumulate. */
 bool chmod_apply(mode_t mode, const char* spec, mode_t* result);
 
 #endif

src/shared/file.c

@@ -112,21 +112,16 @@ unsigned file_process_umask(void) {
 
 /* Base mode applied when the policy does not take the source mode wholesale
  * (i.e. --perms is off).  A pre-existing destination keeps its own mode; a
- * brand-new file is created like rsync: source_mode & 0777 & ~umask, with
- * S_IWGRP|S_IWOTH always cleared so a client mode can never grant group/other
- * write (the daemon runs with umask(0)).  Only when no metadata is available at
- * all does the historical fixed 0644 default apply.  The -E rule (and no-op for
- * a plain -t) is layered on top of this base. */
+ * brand-new file is created like rsync: source_mode & 0777 & ~umask (special
+ * bits are not part of a mode-preserving transfer without -p).  Only when no
+ * metadata is available at all does the historical fixed 0644 default apply.
+ * The -E rule (and no-op for a plain -t) is layered on top of this base. */
 static mode_t file_mode_base(const FileMetadata* metadata, bool existing_known,
                              mode_t existing_mode) {
   if (existing_known)
     return existing_mode;
   if (metadata)
-    /* A brand-new file follows rsync's source_mode & ~umask base, but a
-     * client-supplied source mode must never grant group/other write (the
-     * daemon runs with umask(0), so an unmasked 0666 would otherwise create a
-     * world-writable file).  S_IWGRP|S_IWOTH are always cleared. */
-    return metadata->mode & 0777 & ~(mode_t)file_process_umask() & ~(S_IWGRP | S_IWOTH);
+    return metadata->mode & 0777 & ~(mode_t)file_process_umask();
   return S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH;
 }
 

src/shared/file_receive.c

@@ -448,10 +448,12 @@ static FileSaveResult file_save_special_to_disk(const char* root_directory, cons
     create_mode = S_IFIFO;
   }
   const char* node_kind = (is_char || is_blk) ? "device" : (is_fifo ? "FIFO" : "socket");
-  /* The creation permission bits come from the source only under -p/--perms;
-   * otherwise a safe default (0644, group/other write never granted) keeps an
-   * unprivileged no--p run from materializing a world-writable node. */
-  mode_t perms = config->preserve_perms ? (mode & 0777 & ~(S_IWGRP | S_IWOTH)) : 0644;
+  /* Under -p/--perms rsync copies the source's permission and special bits; a
+   * kernel that denies setuid/setgid/sticky reports the failure rather than
+   * having them masked here.  Without -p the node is created like any other new
+   * entry: source_mode & 0777 & ~umask. */
+  mode_t perms = config->preserve_perms ? (mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777))
+                                        : (mode & 0777 & ~(mode_t)file_process_umask());
 
   int rc = is_fifo ? mkfifoat(parent_fd, leaf, perms)
                    : mknodat(parent_fd, leaf, create_mode | perms, rdev);
@@ -2638,11 +2640,9 @@ void dir_metadata_list_apply(const DirTimeList* list, const char* root_directory
         mode_ready = false;
       }
       if (mode_ready) {
-        /* Route the directory mode through the SAME sanitization as the
-         * regular-file policy: a client-supplied mode never grants group/other
-         * write. */
-        mode_t safe_mode =
-            (dir_mode & 0777 & ~(S_IWGRP | S_IWOTH)) | (dir_mode & (S_ISGID | S_ISVTX));
+        /* rsync -p copies the source directory mode exactly, including
+         * group/other write and the setgid/sticky bits. */
+        mode_t safe_mode = dir_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777);
         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",

src/shared/metadata.c

@@ -213,8 +213,11 @@ bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrP
                               mode_t* out_mode) {
   const mode_t execute_bits = S_IXUSR | S_IXGRP | S_IXOTH;
   if (policy.perms) {
-    /* Group/other write is never granted from a client-supplied mode. */
-    *out_mode = source_mode & 0777 & ~(S_IWGRP | S_IWOTH);
+    /* rsync --perms copies the source's permission and special bits exactly,
+     * including group/other write and setuid/setgid/sticky.  The kernel may
+     * still clear setgid when the receiver is not in the file's group; the
+     * caller logs a failed chmod rather than silently masking the bits here. */
+    *out_mode = source_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777);
     return true;
   }
   if (policy.executability) {
@@ -223,9 +226,9 @@ bool metadata_mode_for_policy(mode_t source_mode, mode_t current_mode, FileAttrP
      * bits from the DESTINATION's own read bits (so a class that can read may
      * execute); otherwise clear every execute bit.  This runs on the
      * destination-derived base (pre-existing dest mode, or source&~umask for a
-     * new file), and leaves special bits untouched.  --perms wins when both are
-     * set (handled above). */
-    mode_t base = current_mode & 0777;
+     * new file), and leaves the special bits untouched.  --perms wins when both
+     * are set (handled above). */
+    mode_t base = current_mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777);
     if (source_mode & 0111)
       *out_mode = base | ((base & 0444) >> 2);
     else
@@ -310,7 +313,7 @@ bool file_restore_symlink_metadata(const char* path, const FileMetadata* metadat
      platforms that support it and quietly ignore the unsupported case so the
      transfer never fails over it. */
   if (policy.perms) {
-    mode_t link_mode = metadata->mode & 0777 & ~(S_IWGRP | S_IWOTH);
+    mode_t link_mode = metadata->mode & (mode_t)(S_ISUID | S_ISGID | S_ISVTX | 0777);
     if (fchmodat(parent_fd, leaf, link_mode, AT_SYMLINK_NOFOLLOW) != 0 && errno != EOPNOTSUPP &&
         errno != ENOTSUP && errno != ENOSYS) {
       log_message(LOG_LEVEL_DEBUG, "Could not set symlink mode on %s: %s", path, strerror(errno));
@@ -343,15 +346,6 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata, FileAttrPoli
   if (fd < 0 || metadata == NULL)
     return metadata == NULL;
   bool ok = true;
-  if (policy.perms || policy.executability) {
-    struct stat current;
-    if (fstat(fd, &current) != 0)
-      return false;
-    mode_t safe_mode = 0;
-    bool apply_mode = metadata_mode_for_policy(metadata->mode, current.st_mode, policy, &safe_mode);
-    if (apply_mode && fchmod(fd, safe_mode) != 0)
-      ok = false;
-  }
   /* Client uid/gid values are deliberately not authoritative UNLESS the client
      explicitly opted in with an identity flag (--numeric-ids / --usermap /
      --groupmap / --chown / -o/-g).  identity_apply_ownership is the controlled,
@@ -362,9 +356,20 @@ bool file_restore_metadata_fd(int fd, const FileMetadata* metadata, FileAttrPoli
      marks this entry as failed instead of reporting a wrong-owner write as
      success.  With no identity flag set it is a no-op, so a default or plain -M
      transfer keeps FastSync's existing behavior of never applying client
-     ownership. */
+     ownership.  Ownership runs BEFORE the mode because a chown clears
+     setuid/setgid; rsync likewise chowns first and then restores the source
+     mode (including its special bits). */
   if (!identity_apply_ownership(fd, (int32_t)metadata->uid, (int32_t)metadata->gid))
     ok = false;
+  if (policy.perms || policy.executability) {
+    struct stat current;
+    if (fstat(fd, &current) != 0)
+      return false;
+    mode_t safe_mode = 0;
+    bool apply_mode = metadata_mode_for_policy(metadata->mode, current.st_mode, policy, &safe_mode);
+    if (apply_mode && fchmod(fd, safe_mode) != 0)
+      ok = false;
+  }
   /* --crtimes captures and transmits the source birth time, but there is no
    * portable way to set a birth time (utimensat can only set atime/mtime), so
    * the receiver deliberately does NOT apply it.  This is explicit, honest