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