Merge branch 'fix/audit-filterx' into fix/audit-cycle

CMakeLists.txt

@@ -226,6 +226,7 @@ set(TEST_SRCS
 	tests/test_file.c
 	tests/test_file_list.c
 	tests/test_file_sendfile.c
+	tests/test_filter.c
 	tests/test_format.c
 	tests/test_fuzz_smoke.c
 	tests/test_glob.c

src/shared/filter.c

@@ -172,14 +172,50 @@ static bool is_modifier_char(char c) {
   return c == 's' || c == 'r' || c == 'p' || c == 'x' || c == '/' || c == '!' || c == 'C';
 }
 
+/* Modifiers rsync defines but FastSync does not implement.  They must still be
+ * consumed as part of the modifier run so they are rejected explicitly instead
+ * of leaking into the pattern (which produced misleading failures such as
+ * "could not read merge file 'n file'"). */
+static bool is_unsupported_modifier_char(char c) {
+  return c == 'e' || c == 'n' || c == 'w';
+}
+
+/* Characters that are part of a modifier run, whether supported or not. */
+static bool is_modifier_scan_char(char c) {
+  return is_modifier_char(c) || is_unsupported_modifier_char(c);
+}
+
+/* Inspect the token that follows a rule name (up to the first space/underscore
+ * or the end).  If the token is composed *solely* of modifier characters and
+ * includes one FastSync does not implement, it is unambiguously a modifier run:
+ * return that character so the caller can reject it.  A token that contains any
+ * non-modifier character is a pattern (e.g. "-newfile") and returns '\0', which
+ * keeps the historical parsing of mixed tokens such as "H,!secret" intact. */
+static char unsupported_modifier_in_token(const char* tok) {
+  if (*tok == '\0' || *tok == ' ' || *tok == '_')
+    return '\0';
+  char bad = '\0';
+  for (const char* q = tok; *q != '\0' && *q != ' ' && *q != '_'; q++) {
+    if (!is_modifier_scan_char(*q))
+      return '\0';
+    if (is_unsupported_modifier_char(*q))
+      bad = *q;
+  }
+  return bad;
+}
+
 /* Parse "RULE[,MODIFIERS] [PATTERN]".  On success `kind`, `sides`,
  * `sides_explicit`, `negate`, `anchored_mod`, `perishable`, `xattr`,
  * `cvs_inject` and the pattern span (`pat_start`/`pat_len`, possibly 0 for
- * merge/clear) are filled.  Returns true on success. */
+ * merge/clear) are filled.  Returns true on success.
+ *
+ * On failure `*bad_mod` is set to the offending modifier character when the
+ * rule carried a modifier FastSync does not implement, and left '\0' for a
+ * generic syntax error so callers can emit a precise diagnostic. */
 static bool parse_rule_syntax(const char* text, RuleKind* kind, unsigned* sides,
                               bool* sides_explicit, bool* negate, bool* anchored_mod,
                               bool* perishable, bool* xattr, bool* cvs_inject,
-                              const char** pat_start, size_t* pat_len) {
+                              const char** pat_start, size_t* pat_len, char* bad_mod) {
   const char* p = text;
   *sides = FILTER_SIDE_SENDER | FILTER_SIDE_RECEIVER;
   *sides_explicit = false;
@@ -190,6 +226,7 @@ static bool parse_rule_syntax(const char* text, RuleKind* kind, unsigned* sides,
   *cvs_inject = false;
   *pat_start = NULL;
   *pat_len = 0;
+  *bad_mod = '\0';
 
   bool is_short = false;
   if (short_rule_char(*p, kind)) {
@@ -210,6 +247,14 @@ static bool parse_rule_syntax(const char* text, RuleKind* kind, unsigned* sides,
   /* Modifiers: long names require a comma; short names may attach directly.
      Only commit a modifier run that terminates at a separator or the end, so a
      pattern such as "*.tmp" written as "-*.tmp" is not mistaken for modifiers. */
+  if (*p == ',') {
+    *bad_mod = unsupported_modifier_in_token(p + 1);
+  } else if (is_short) {
+    *bad_mod = unsupported_modifier_in_token(p);
+  }
+  if (*bad_mod != '\0')
+    return false;
+
   const char* mod_start = p;
   const char* mod_end = p;
   if (*p == ',') {
@@ -290,9 +335,13 @@ FilterRule* filter_rule_parse(const char* line, const FilterParseOptions* opts,
   bool sides_explicit, negate, anchored_mod, perishable, xattr, cvs_inject;
   const char* pat;
   size_t pat_len;
+  char bad_mod;
   if (!parse_rule_syntax(p, &kind, &sides, &sides_explicit, &negate, &anchored_mod, &perishable,
-                         &xattr, &cvs_inject, &pat, &pat_len)) {
+                         &xattr, &cvs_inject, &pat, &pat_len, &bad_mod)) {
-    filter_set_error(err, err_size, "unrecognized filter rule syntax");
+    if (bad_mod != '\0')
+      filter_set_error(err, err_size, "unsupported filter modifier '%c'", bad_mod);
+    else
+      filter_set_error(err, err_size, "unrecognized filter rule syntax");
     return NULL;
   }
   if (cvs_inject) {
@@ -401,7 +450,6 @@ FilterRule* filter_rule_parse(const char* line, const FilterParseOptions* opts,
   rule->dir_only = dir_only;
   rule->negate = negate;
   rule->perishable = perishable;
-  (void)xattr; /* xattr-name rules never match file/dir names; accepted/ignored */
   return rule;
 }
 
@@ -530,16 +578,27 @@ static bool filter_list_parse_append_depth(FilterRuleList* list, const char* lin
   bool sides_explicit, negate, anchored_mod, perishable, xattr, cvs_inject;
   const char* pat;
   size_t pat_len;
+  char bad_mod;
   if (!parse_rule_syntax(p, &kind, &sides, &sides_explicit, &negate, &anchored_mod, &perishable,
-                         &xattr, &cvs_inject, &pat, &pat_len)) {
+                         &xattr, &cvs_inject, &pat, &pat_len, &bad_mod)) {
-    filter_set_error(err, err_size, "unrecognized filter rule syntax: %s", p);
+    if (bad_mod != '\0')
+      filter_set_error(err, err_size, "unsupported filter modifier '%c': %s", bad_mod, p);
+    else
+      filter_set_error(err, err_size, "unrecognized filter rule syntax: %s", p);
     return false;
   }
   (void)sides_explicit;
   (void)negate;
   (void)anchored_mod;
   (void)perishable;
-  (void)xattr;
+
+  /* xattr-name rules are not implemented; reject them everywhere (including on
+   * merge/dir-merge, where the flag would otherwise be silently dropped) with
+   * the same diagnostic the standalone parser gives. */
+  if (xattr) {
+    filter_set_error(err, err_size, "xattr-name filter rules (the x modifier) are not supported");
+    return false;
+  }
 
   if (cvs_inject) {
     /* "C" injects the CVS defaults in place; no pattern is expected. */

src/shared/filter.h

@@ -23,7 +23,9 @@
  *   dir-merge/: per-directory merge file (registered for the scanner)
  *   clear/!     clear the current rule list (takes no argument)
  * Modifiers: '/' absolute anchor, '!' negate match, 'C' inject CVS defaults,
- * 's' sender side, 'r' receiver side, 'p' perishable, 'x' xattr name rule.
+ * 's' sender side, 'r' receiver side, 'p' perishable.  The rsync 'x'
+ * (xattr-name) modifier and the merge-only 'e'/'n'/'w' modifiers are not
+ * implemented and are rejected explicitly.
  * A trailing '/' makes a pattern match directories only.  A leading '/' anchors
  * the pattern to its owner directory.
  */

tests/runner.c

@@ -16,6 +16,7 @@
 #include "test_file.h"
 #include "test_file_list.h"
 #include "test_file_sendfile.h"
+#include "test_filter.h"
 #include "test_format.h"
 #include "test_fuzz_smoke.h"
 #include "test_glob.h"
@@ -80,6 +81,7 @@ int main() {
   RUN_TEST(test_receiver_timeout);
   RUN_TEST(test_metadata);
   RUN_TEST(test_glob);
+  RUN_TEST(test_filter);
   RUN_TEST(test_iconv);
   RUN_TEST(test_file);
   RUN_TEST(test_file_list);

tests/test_filter.c

@@ -0,0 +1,222 @@
+#include "test_filter.h"
+#include "filter.h"
+#include "test_utils.h"
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+/* Every `-f`/`--filter` rule string is validated through the list parser, so
+ * the list parser must reject the modifiers the standalone parser rejects
+ * rather than silently folding them into a pattern. */
+
+static void test_filter_list_rejects_xattr_modifier() {
+  static const char* const rules[] = {
+      "-x user.foo",             /* short exclude + x */
+      "exclude,x user.foo",      /* long exclude + x */
+      "+x user.foo",             /* include + x */
+      "hide,x *.tmp",            /* hide + x */
+      "dir-merge,x .rules",      /* x must not be dropped on dir-merge */
+      "merge,x /tmp/nonexistent" /* x must not be dropped on merge */
+  };
+  for (size_t i = 0; i < sizeof(rules) / sizeof(rules[0]); i++) {
+    FilterRuleList* list = filter_rule_list_create();
+    EXPECT_NOT_NULL(list);
+    char err[256] = "";
+    bool ok = filter_rule_list_parse_append(list, rules[i], NULL, NULL, err, sizeof(err));
+    EXPECT_FALSE(ok);
+    EXPECT_TRUE(strstr(err, "xattr") != NULL);
+    filter_rule_list_free(list);
+  }
+
+  /* A bare "x" is not a rule at all: rejected as generic bad syntax. */
+  FilterRuleList* list = filter_rule_list_create();
+  EXPECT_NOT_NULL(list);
+  char err[256] = "";
+  EXPECT_FALSE(filter_rule_list_parse_append(list, "x user.foo", NULL, NULL, err, sizeof(err)));
+  EXPECT_TRUE(err[0] != '\0');
+  filter_rule_list_free(list);
+}
+
+static void test_filter_list_rejects_unsupported_modifiers() {
+  static const char* const rules[] = {
+      "-e foo", /* e: merge-only in rsync */
+      "-n foo", /* n: merge-only in rsync */
+      "-w foo", /* w: merge-only in rsync */
+      "merge,n /tmp/x", ".e /tmp/x", "dir-merge,e .rules", "exclude,w foo",
+  };
+  for (size_t i = 0; i < sizeof(rules) / sizeof(rules[0]); i++) {
+    FilterRuleList* list = filter_rule_list_create();
+    EXPECT_NOT_NULL(list);
+    char err[256] = "";
+    bool ok = filter_rule_list_parse_append(list, rules[i], NULL, NULL, err, sizeof(err));
+    EXPECT_FALSE(ok);
+    EXPECT_TRUE(strstr(err, "unsupported filter modifier") != NULL);
+    filter_rule_list_free(list);
+  }
+}
+
+static void test_filter_list_accepts_supported_rules_and_modifiers() {
+  static const char* const rules[] = {
+      "- *.tmp",  "+ /a.txt",         "-s foo",     "-r foo",     "-p foo",
+      "-! *.o",   "-/ foo",           "hide *.tmp", "show *.txt", "protect *.bak",
+      "risk *.o", "dir-merge .rules", "-C",
+  };
+  for (size_t i = 0; i < sizeof(rules) / sizeof(rules[0]); i++) {
+    FilterRuleList* list = filter_rule_list_create();
+    EXPECT_NOT_NULL(list);
+    char err[256] = "";
+    bool ok = filter_rule_list_parse_append(list, rules[i], NULL, NULL, err, sizeof(err));
+    if (!ok)
+      printf("    rule '%s' errored: %s\n", rules[i], err);
+    EXPECT_TRUE(ok);
+    filter_rule_list_free(list);
+  }
+
+  /* A glued word is a pattern, not a modifier run (no separator). */
+  {
+    FilterRuleList* list = filter_rule_list_create();
+    char err[128] = "";
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "-newfile", NULL, NULL, err, sizeof(err)));
+    EXPECT_EQ_INT(list->count, 1);
+    EXPECT_EQ_STR(list->items[0]->pattern, "newfile");
+    filter_rule_list_free(list);
+
+    list = filter_rule_list_create();
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "-e2e", NULL, NULL, err, sizeof(err)));
+    EXPECT_EQ_INT(list->count, 1);
+    EXPECT_EQ_STR(list->items[0]->pattern, "e2e");
+    filter_rule_list_free(list);
+
+    list = filter_rule_list_create();
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "-*.o", NULL, NULL, err, sizeof(err)));
+    EXPECT_EQ_INT(list->count, 1);
+    EXPECT_EQ_STR(list->items[0]->pattern, "*.o");
+    filter_rule_list_free(list);
+
+    /* A comma with no modifier still treats the rest as the pattern. */
+    list = filter_rule_list_create();
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "exclude,foo", NULL, NULL, err, sizeof(err)));
+    EXPECT_EQ_INT(list->count, 1);
+    EXPECT_EQ_STR(list->items[0]->pattern, "foo");
+    filter_rule_list_free(list);
+  }
+
+  /* `!` clears the list. */
+  {
+    FilterRuleList* list = filter_rule_list_create();
+    char err[128] = "";
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "- *.tmp", NULL, NULL, err, sizeof(err)));
+    EXPECT_EQ_INT(list->count, 1);
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "!", NULL, NULL, err, sizeof(err)));
+    EXPECT_EQ_INT(list->count, 0);
+    filter_rule_list_free(list);
+  }
+
+  /* -C injects the CVS defaults. */
+  {
+    FilterRuleList* list = filter_rule_list_create();
+    char err[128] = "";
+    EXPECT_TRUE(filter_rule_list_parse_append(list, "-C", NULL, NULL, err, sizeof(err)));
+    EXPECT_TRUE(list->count > 0);
+    filter_rule_list_free(list);
+  }
+}
+
+static void test_filter_list_merge_file_still_supported() {
+  char tmpl[] = "/tmp/fastsync_filter_XXXXXX";
+  EXPECT_TRUE(mkdtemp(tmpl) != NULL);
+  char path[512];
+  snprintf(path, sizeof(path), "%s/rules", tmpl);
+  FILE* fp = fopen(path, "w");
+  EXPECT_NOT_NULL(fp);
+  fputs("- *.tmp\n", fp);
+  fclose(fp);
+
+  FilterRuleList* list = filter_rule_list_create();
+  char err[256] = "";
+  char rule[600];
+  snprintf(rule, sizeof(rule), "merge %s", path);
+  EXPECT_TRUE(filter_rule_list_parse_append(list, rule, NULL, NULL, err, sizeof(err)));
+  EXPECT_EQ_INT(list->count, 1);
+  filter_rule_list_free(list);
+
+  /* The same merge with the x modifier is rejected, not silently read. */
+  list = filter_rule_list_create();
+  snprintf(rule, sizeof(rule), "merge,x %s", path);
+  EXPECT_FALSE(filter_rule_list_parse_append(list, rule, NULL, NULL, err, sizeof(err)));
+  EXPECT_TRUE(strstr(err, "xattr") != NULL);
+  filter_rule_list_free(list);
+
+  unlink(path);
+  rmdir(tmpl);
+}
+
+static void test_filter_rule_parse_rejects_unsupported_and_keeps_supported() {
+  char err[256] = "";
+
+  EXPECT_NULL(filter_rule_parse("-x user.foo", NULL, err, sizeof(err)));
+  EXPECT_TRUE(strstr(err, "xattr") != NULL);
+
+  EXPECT_NULL(filter_rule_parse("-e foo", NULL, err, sizeof(err)));
+  EXPECT_TRUE(strstr(err, "unsupported filter modifier") != NULL);
+
+  FilterRule* rule = filter_rule_parse("- *.tmp", NULL, err, sizeof(err));
+  EXPECT_NOT_NULL(rule);
+  EXPECT_EQ_STR(rule->pattern, "*.tmp");
+  filter_rule_free(rule);
+
+  rule = filter_rule_parse("-newfile", NULL, err, sizeof(err));
+  EXPECT_NOT_NULL(rule);
+  EXPECT_EQ_STR(rule->pattern, "newfile");
+  filter_rule_free(rule);
+}
+
+static void test_filter_rules_apply_supported_modifiers() {
+  /* exclude */
+  {
+    const char* texts[] = {"- *.tmp"};
+    FilterRuleList* list = filter_base_build(texts, 1, false, false, NULL, 0);
+    EXPECT_NOT_NULL(list);
+    EXPECT_EQ_INT(filter_rules_apply(list, "b.tmp", "b.tmp", false), FILTER_ACTION_EXCLUDE);
+    EXPECT_EQ_INT(filter_rules_apply(list, "a.txt", "a.txt", false), FILTER_ACTION_NONE);
+    filter_rule_list_free(list);
+  }
+  /* anchored include then exclude-all */
+  {
+    const char* texts[] = {"+ /a.txt", "- *"};
+    FilterRuleList* list = filter_base_build(texts, 2, false, false, NULL, 0);
+    EXPECT_NOT_NULL(list);
+    EXPECT_EQ_INT(filter_rules_apply(list, "a.txt", "a.txt", false), FILTER_ACTION_INCLUDE);
+    EXPECT_EQ_INT(filter_rules_apply(list, "b.txt", "b.txt", false), FILTER_ACTION_EXCLUDE);
+    filter_rule_list_free(list);
+  }
+  /* negate */
+  {
+    const char* texts[] = {"-! *.o"};
+    FilterRuleList* list = filter_base_build(texts, 1, false, false, NULL, 0);
+    EXPECT_NOT_NULL(list);
+    EXPECT_EQ_INT(filter_rules_apply(list, "foo.c", "foo.c", false), FILTER_ACTION_EXCLUDE);
+    EXPECT_EQ_INT(filter_rules_apply(list, "foo.o", "foo.o", false), FILTER_ACTION_NONE);
+    filter_rule_list_free(list);
+  }
+  /* dir-only trailing slash */
+  {
+    const char* texts[] = {"+ dir/", "- *"};
+    FilterRuleList* list = filter_base_build(texts, 2, false, false, NULL, 0);
+    EXPECT_NOT_NULL(list);
+    EXPECT_EQ_INT(filter_rules_apply(list, "dir", "dir", true), FILTER_ACTION_INCLUDE);
+    EXPECT_EQ_INT(filter_rules_apply(list, "dir", "dir", false), FILTER_ACTION_EXCLUDE);
+    filter_rule_list_free(list);
+  }
+}
+
+void test_filter() {
+  test_filter_list_rejects_xattr_modifier();
+  test_filter_list_rejects_unsupported_modifiers();
+  test_filter_list_accepts_supported_rules_and_modifiers();
+  test_filter_list_merge_file_still_supported();
+  test_filter_rule_parse_rejects_unsupported_and_keeps_supported();
+  test_filter_rules_apply_supported_modifiers();
+}

tests/test_filter.h

@@ -0,0 +1,6 @@
+#ifndef TEST_FILTER_H
+#define TEST_FILTER_H
+
+void test_filter(void);
+
+#endif