From 0f40e747f08b8b18c4b3798bcba6f6bc8463a25d Mon Sep 17 00:00:00 2001 From: TapTap Date: Mon, 21 Sep 2026 19:19:06 +0200 Subject: [PATCH] fix(filter): reject the x modifier in the --filter list parser The standalone filter_rule_parse() already rejected the rsync xattr-name 'x' modifier, but the list parser used by --filter/-f silently dropped the flag for merge/dir-merge rules (and relied on a second parse for plain rules). Reject it explicitly in filter_list_parse_append_depth() with the same diagnostic, so '-x', 'merge,x' and 'dir-merge,x' all fail cleanly. Also reject the unimplemented rsync merge modifiers 'e', 'n' and 'w' instead of folding them into the pattern, which previously produced misleading errors such as "could not read merge file 'n file'". Only a token made up solely of modifier characters is treated as a modifier run, so glued patterns ('-newfile', '-e2e') and mixed tokens ("H,!secret") keep their historical parsing. Adds tests/test_filter.c with focused rejection and supported-syntax cases. --- CMakeLists.txt | 1 + src/shared/filter.c | 75 +++++++++++++-- src/shared/filter.h | 4 +- tests/runner.c | 2 + tests/test_filter.c | 222 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_filter.h | 6 ++ 6 files changed, 301 insertions(+), 9 deletions(-) create mode 100644 tests/test_filter.c create mode 100644 tests/test_filter.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 905daec..09c10f6 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/src/shared/filter.c b/src/shared/filter.c index 2a2cd7a..f478286 100644 --- a/src/shared/filter.c +++ b/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)) { - filter_set_error(err, err_size, "unrecognized filter rule syntax"); + &xattr, &cvs_inject, &pat, &pat_len, &bad_mod)) { + 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)) { - filter_set_error(err, err_size, "unrecognized filter rule syntax: %s", p); + &xattr, &cvs_inject, &pat, &pat_len, &bad_mod)) { + 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. */ diff --git a/src/shared/filter.h b/src/shared/filter.h index 691860b..bd9877b 100644 --- a/src/shared/filter.h +++ b/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. */ diff --git a/tests/runner.c b/tests/runner.c index de1b4b5..d790287 100644 --- a/tests/runner.c +++ b/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); diff --git a/tests/test_filter.c b/tests/test_filter.c new file mode 100644 index 0000000..76e2729 --- /dev/null +++ b/tests/test_filter.c @@ -0,0 +1,222 @@ +#include "test_filter.h" +#include "filter.h" +#include "test_utils.h" +#include +#include +#include +#include +#include + +/* 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(); +} diff --git a/tests/test_filter.h b/tests/test_filter.h new file mode 100644 index 0000000..26fa58c --- /dev/null +++ b/tests/test_filter.h @@ -0,0 +1,6 @@ +#ifndef TEST_FILTER_H +#define TEST_FILTER_H + +void test_filter(void); + +#endif