From 49a2fea2b6fe6b4704fef04ea6ac5034d5978b52 Mon Sep 17 00:00:00 2001 From: Quentin Deslandes Date: Fri, 30 Oct 2026 09:48:04 +0100 Subject: [PATCH 1/3] lib: cgen: separate set insertion from group traversal Separate group insertion from set discovery so generation can collect sets from more than one owner. --- src/libbpfilter/cgen/program.c | 84 ++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/src/libbpfilter/cgen/program.c b/src/libbpfilter/cgen/program.c index eb53e6c6..82e3304a 100644 --- a/src/libbpfilter/cgen/program.c +++ b/src/libbpfilter/cgen/program.c @@ -164,57 +164,63 @@ _bf_program_find_set_group(const struct bf_program *program, return NULL; } -static int _bf_program_build_set_groups(struct bf_program *program) +static int _bf_program_add_set_to_group(struct bf_program *program, + const struct bf_set *set) { + _free_bf_set_group_ struct bf_set_group *group = NULL; + int r; + assert(program); + assert(set); - bf_list_clean(&program->set_groups); - program->set_groups = bf_list_default(_bf_set_group_free, NULL); + if (bf_hashset_is_empty(&set->elems)) + return 0; - bf_hashset_foreach (&program->runtime.chain->sets, set_node) { - struct bf_set *set = set_node->data; - struct bf_set_group *match = NULL; - int r; + /* If a group with a matching key already exists, reuse it. For LPM trie, + * sets can't be grouped (as we can't define which rule matches on which + * prefix), so they always are the sole member of their group. */ + if (!set->use_trie) { + bf_list_foreach (&program->set_groups, group_node) { + struct bf_set_group *group = bf_list_node_get_data(group_node); + const struct bf_set *head = + bf_list_node_get_data(bf_list_get_head(&group->sets)); - if (bf_hashset_is_empty(&set->elems)) - continue; + if (bf_set_same_key(set, head)) { + r = bf_list_add_tail(&group->sets, (void *)set); + if (r) + return bf_err_r(r, "failed to add set to existing group"); - /* LPM trie sets are not grouped: BPF LPM trie lookup always - * returns the longest-prefix match, so the read-modify-write - * step in _bf_program_load_sets_maps() can't preserve the - * per-set bitmask when prefixes overlap. */ - if (!set->use_trie) { - bf_list_foreach (&program->set_groups, group_node) { - struct bf_set_group *group = bf_list_node_get_data(group_node); - const struct bf_set *head = - bf_list_node_get_data(bf_list_get_head(&group->sets)); - - if (bf_set_same_key(set, head)) { - match = group; - break; - } + return 0; } } + } - if (match) { - r = bf_list_add_tail(&match->sets, set); - if (r) - return bf_err_r(r, "failed to add set to existing group"); - } else { - _free_bf_set_group_ struct bf_set_group *new_group = NULL; + r = _bf_set_group_new(&group); + if (r) + return bf_err_r(r, "failed to allocate set group"); - r = _bf_set_group_new(&new_group); - if (r) - return bf_err_r(r, "failed to allocate set group"); + r = bf_list_add_tail(&group->sets, (void *)set); + if (r) + return bf_err_r(r, "failed to seed set group"); - r = bf_list_add_tail(&new_group->sets, set); - if (r) - return bf_err_r(r, "failed to seed set group"); + r = bf_list_push(&program->set_groups, (void **)&group); + if (r) + return bf_err_r(r, "failed to register set group"); - r = bf_list_push(&program->set_groups, (void **)&new_group); - if (r) - return bf_err_r(r, "failed to register set group"); - } + return 0; +} + +static int _bf_program_build_set_groups(struct bf_program *program) +{ + assert(program); + + bf_list_clean(&program->set_groups); + program->set_groups = bf_list_default(_bf_set_group_free, NULL); + + bf_hashset_foreach (&program->runtime.chain->sets, set_node) { + int r = _bf_program_add_set_to_group(program, set_node->data); + if (r) + return r; } return 0; From e4a75626e1f56f1431f7b1e8a5d6272b4f3f55f9 Mon Sep 17 00:00:00 2001 From: Quentin Deslandes Date: Fri, 30 Oct 2026 09:53:06 +0100 Subject: [PATCH 2/3] lib: matcher: borrow set pointers instead of copying names Set matchers currently copy only the set name. Adding a rule to a chain can therefore make it use a different set with the same name. Store a borrowed set pointer in each matcher. Reject rules that refer to sets the chain does not own. When loading a chain, look up each set name to get its pointer. When copying a chain, point the copied matchers at the copied sets. --- src/libbpfilter/cgen/cgroup_sock_addr.c | 3 +- src/libbpfilter/cgen/matcher/set.c | 3 +- src/libbpfilter/cgen/packet.c | 3 +- src/libbpfilter/chain.c | 42 ++++++++++-- src/libbpfilter/include/bpfilter/chain.h | 2 +- src/libbpfilter/include/bpfilter/matcher.h | 14 ++-- src/libbpfilter/include/bpfilter/rule.h | 18 ++++- src/libbpfilter/matcher.c | 60 ++++++++++++----- src/libbpfilter/rule.c | 5 +- tests/harness/Chain.hpp | 2 +- tests/harness/Matcher.hpp | 10 ++- tests/harness/Rule.hpp | 5 +- tests/harness/test.c | 13 ++++ tests/unit/libbpfilter/chain.c | 78 +++++++++++++++++++++- tests/unit/libbpfilter/matcher.c | 53 ++++++++++----- tests/unit/libbpfilter/rule.c | 10 +-- 16 files changed, 252 insertions(+), 69 deletions(-) diff --git a/src/libbpfilter/cgen/cgroup_sock_addr.c b/src/libbpfilter/cgen/cgroup_sock_addr.c index babf10da..9ac30e37 100644 --- a/src/libbpfilter/cgen/cgroup_sock_addr.c +++ b/src/libbpfilter/cgen/cgroup_sock_addr.c @@ -294,8 +294,7 @@ static int _bf_cgroup_sock_addr_generate_set(struct bf_program *program, set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher); if (!set) { - return bf_err_r(-ENOENT, "set #%u not found in %s", - *(uint32_t *)bf_matcher_payload(matcher), + return bf_err_r(-ENOENT, "matcher set not found in %s", program->runtime.chain->name); } diff --git a/src/libbpfilter/cgen/matcher/set.c b/src/libbpfilter/cgen/matcher/set.c index c67ebec0..cbf0d9af 100644 --- a/src/libbpfilter/cgen/matcher/set.c +++ b/src/libbpfilter/cgen/matcher/set.c @@ -28,8 +28,7 @@ int bf_set_generate_map_lookup(struct bf_program *program, set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher); if (!set) { - return bf_err_r(-ENOENT, "set '%s' not found in %s", - (const char *)bf_matcher_payload(matcher), + return bf_err_r(-ENOENT, "matcher set not found in %s", program->runtime.chain->name); } diff --git a/src/libbpfilter/cgen/packet.c b/src/libbpfilter/cgen/packet.c index 969c4e85..a904959e 100644 --- a/src/libbpfilter/cgen/packet.c +++ b/src/libbpfilter/cgen/packet.c @@ -338,8 +338,7 @@ static int _bf_matcher_pkt_generate_set(struct bf_program *program, set = bf_chain_get_set_for_matcher(program->runtime.chain, matcher); if (!set) { - return bf_err_r(-ENOENT, "set #%u not found in %s", - *(uint32_t *)bf_matcher_payload(matcher), + return bf_err_r(-ENOENT, "matcher set not found in %s", program->runtime.chain->name); } diff --git a/src/libbpfilter/chain.c b/src/libbpfilter/chain.c index 65b4cb05..7182bb39 100644 --- a/src/libbpfilter/chain.c +++ b/src/libbpfilter/chain.c @@ -317,8 +317,13 @@ int bf_chain_new(struct bf_chain **chain, const char *name, enum bf_hook hook, } r = _bf_chain_validate_rules(_chain); - if (r) + if (r) { + if (rules) + *rules = bf_list_move(&_chain->rules); + if (sets) + *sets = bf_hashset_move(&_chain->sets); return r; + } *chain = TAKE_PTR(_chain); @@ -332,8 +337,8 @@ int bf_chain_new_from_pack(struct bf_chain **chain, bf_rpack_node_t node) enum bf_hook hook; enum bf_verdict policy; bf_rpack_node_t array, array_node; - _clean_bf_list_ bf_list rules = bf_list_default(bf_rule_free, bf_rule_pack); _clean_bf_hashset_ bf_hashset sets = bf_set_hashset_default(); + _clean_bf_list_ bf_list rules = bf_list_default(bf_rule_free, bf_rule_pack); int r; r = bf_rpack_kv_str(node, "name", &name); @@ -369,7 +374,8 @@ int bf_chain_new_from_pack(struct bf_chain **chain, bf_rpack_node_t node) bf_rpack_array_foreach (array, array_node) { _free_bf_rule_ struct bf_rule *rule = NULL; - r = bf_list_emplace(&rules, bf_rule_new_from_pack, rule, array_node); + r = bf_list_emplace(&rules, bf_rule_new_from_pack, rule, array_node, + &sets); if (r) { return bf_err_r(r, "failed to unpack bf_rule into bf_chain.rules"); } @@ -391,8 +397,8 @@ void bf_chain_free(struct bf_chain **chain) if (!*chain) return; - bf_hashset_clean(&(*chain)->sets); bf_list_clean(&(*chain)->rules); + bf_hashset_clean(&(*chain)->sets); BF_FREEP(&(*chain)->name); BF_FREEP(chain); } @@ -498,13 +504,16 @@ const struct bf_set * bf_chain_get_set_for_matcher(const struct bf_chain *chain, const struct bf_matcher *matcher) { + const struct bf_set *set; + assert(chain); assert(matcher); if (bf_matcher_get_type(matcher) != BF_MATCHER_SET) return NULL; - return bf_chain_get_set_by_name(chain, bf_matcher_payload(matcher)); + memcpy(&set, bf_matcher_payload(matcher), sizeof(struct bf_set *)); + return bf_hashset_get(&chain->sets, set) == set ? set : NULL; } static struct bf_set *_bf_chain_get_set_by_name(struct bf_chain *chain, @@ -612,6 +621,29 @@ int bf_chain_new_from_copy(struct bf_chain **dest, const struct bf_chain *src) if (r) return r; + bf_list_foreach (&rule->matchers, matcher_node) { + struct bf_matcher *matcher = bf_list_node_get_data(matcher_node); + const struct bf_set *set; + + if (bf_matcher_get_type(matcher) != BF_MATCHER_SET) + continue; + + set = bf_chain_get_set_for_matcher(src, matcher); + if (!set) + return bf_err_r(-ENOENT, + "matcher set not found in source chain"); + if (!set->name) + continue; + + set = bf_chain_get_set_by_name(copy, set->name); + if (!set) + return bf_err_r(-ENOENT, + "matcher set not found in copied chain"); + + memcpy((void *)bf_matcher_payload(matcher), &set, + sizeof(struct bf_set *)); + } + r = bf_chain_add_rule(copy, rule); if (r) return r; diff --git a/src/libbpfilter/include/bpfilter/chain.h b/src/libbpfilter/include/bpfilter/chain.h index f0a60a37..badd9de2 100644 --- a/src/libbpfilter/include/bpfilter/chain.h +++ b/src/libbpfilter/include/bpfilter/chain.h @@ -140,7 +140,7 @@ int bf_chain_add_rule(struct bf_chain *chain, struct bf_rule *rule); * * The chain will own the set and is responsible for freeing it. To reference * the set in a matcher, pass the set pointer and sizeof(*set) to - * bf_matcher_new(). The matcher stores a copy of the set's name. + * bf_matcher_new(). The matcher borrows the set pointer. * * @param chain Chain to insert the rule into. Can't be NULL. * @param set Set to insert into the chain. Can't be NULL. diff --git a/src/libbpfilter/include/bpfilter/matcher.h b/src/libbpfilter/include/bpfilter/matcher.h index 86510ea6..0ed420ae 100644 --- a/src/libbpfilter/include/bpfilter/matcher.h +++ b/src/libbpfilter/include/bpfilter/matcher.h @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -273,9 +274,9 @@ struct bf_matcher_meta * @param payload Payload of the matcher, its content and size depends on * @p type . Can be NULL but only if @p payload_len is 0, in which case * there is no payload. For BF_MATCHER_SET, pass a pointer to a bf_set. - * The constructor copies its name and retains no pointer to the set. + * The constructor borrows the set. Its owner must outlive the matcher. * @param payload_len Length of the input payload. For BF_MATCHER_SET, pass - * sizeof(struct bf_set). The stored payload is the NUL-terminated name. + * `sizeof(struct bf_set *)`. The stored payload contains the set pointer. * @param negate If true, negate the comparison result. * @return 0 on success, or negative errno value on failure. */ @@ -305,14 +306,17 @@ int bf_matcher_new_from_raw(struct bf_matcher **matcher, * @param matcher On success, points to the newly allocated and initialised * matcher. Can't be NULL. * @param node Serialized matcher. Can't be NULL. + * @param sets Sets used to resolve serialized names. Can be NULL if the + * matcher does not reference a set. The collection retains ownership. * @return 0 on success, or negative error value on failure. */ -int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node); +int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node, + const bf_hashset *sets); /** - * @brief Allocate an independent copy of a matcher and its payload. + * @brief Allocate a copy of a matcher and its payload. * - * Set matchers copy the set name. The referenced set, if any, is not copied. + * Set matchers borrow the same set as the source matcher. * * @param dest Destination matcher. Unchanged on failure. Can't be NULL. * @param src Matcher to copy. Can't be NULL. diff --git a/src/libbpfilter/include/bpfilter/rule.h b/src/libbpfilter/include/bpfilter/rule.h index 7356ca48..45413476 100644 --- a/src/libbpfilter/include/bpfilter/rule.h +++ b/src/libbpfilter/include/bpfilter/rule.h @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -112,16 +113,27 @@ int bf_rule_new(struct bf_rule **rule); * data. The caller will own the object. On failure, `*rule` is * unchanged. Can't be NULL. * @param node Node containing the serialized rule. Can't be NULL. + * @param sets Sets used to resolve matcher references. Can be NULL if no + * matcher references a set. The collection retains ownership. * @return 0 on success, or a negative errno value on failure. */ -int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node); +int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node, + const bf_hashset *sets); /** - * @brief Allocate an independent copy of a rule and its matchers. + * @brief Allocate a copy of a rule and its matchers. + * + * Sets remain borrowed from the same owner as the source rule. That owner + * must outlive the copied rule, including its destruction. * * Runtime counters and disabled state are reset. Adding the copy to a chain * assigns its index, validates set references, and recalculates disabled state. * + * @warning Before inserting the copy into another chain, replace its set + * matchers with matchers that borrow that chain's sets. Matching names alone + * do not rebind references. To copy a complete chain, use + * `bf_chain_new_from_copy()`, which rebinds references to the copied sets. + * * @param dest Destination rule. Unchanged on failure. Can't be NULL. * @param src Rule to copy. Can't be NULL. * @return 0 on success, or a negative errno value on failure. @@ -163,7 +175,7 @@ void bf_rule_dump(const struct bf_rule *rule, prefix_t *prefix); * @param payload Payload of the matcher, its content and size depends on @p * type . Can be NULL but only if @p payload_len is 0, in which case * there is no payload. For BF_MATCHER_SET, pass a pointer to a bf_set. - * The matcher stores an owned copy of its name. + * The matcher borrows the set. Its owner must outlive the matcher. * @param payload_len Length of the input payload. For BF_MATCHER_SET, pass * sizeof(struct bf_set). * @param negate If true, negate the matcher's comparison. diff --git a/src/libbpfilter/matcher.c b/src/libbpfilter/matcher.c index 3fee0f11..47a844b3 100644 --- a/src/libbpfilter/matcher.c +++ b/src/libbpfilter/matcher.c @@ -1369,10 +1369,8 @@ static int _bf_matcher_new(struct bf_matcher **matcher, assert(matcher); assert((payload && payload_len) || (!payload && !payload_len)); - if (type == BF_MATCHER_SET && - (op != BF_MATCHER_IN || !payload || payload_len < 2 || - memchr(payload, '\0', payload_len) != - (const char *)payload + payload_len - 1)) + if (type == BF_MATCHER_SET && (op != BF_MATCHER_IN || !payload || + payload_len != sizeof(struct bf_set *))) return bf_err_r(-EINVAL, "invalid set matcher payload or operator"); if (payload_len > SIZE_MAX - sizeof(*_matcher)) @@ -1402,11 +1400,11 @@ int bf_matcher_new(struct bf_matcher **matcher, enum bf_matcher_type type, if (type == BF_MATCHER_SET) { const struct bf_set *set = payload; - if (!set || payload_len != sizeof(*set) || !set->name) + if (!set || payload_len != sizeof(*set) || !set->name || !*set->name) return bf_err_r(-EINVAL, "set matcher requires a set object"); - payload = set->name; - payload_len = strlen(set->name) + 1; + return _bf_matcher_new(matcher, type, op, &set, sizeof(struct bf_set *), + negate); } return _bf_matcher_new(matcher, type, op, payload, payload_len, negate); @@ -1447,7 +1445,8 @@ int bf_matcher_new_from_raw(struct bf_matcher **matcher, return 0; } -int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node) +int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node, + const bf_hashset *sets) { _free_bf_matcher_ struct bf_matcher *_matcher = NULL; _cleanup_free_ char *set_name = NULL; @@ -1475,18 +1474,25 @@ int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node) } if (type == BF_MATCHER_SET) { + const struct bf_set *set; + r = bf_rpack_kv_str(node, "payload", &set_name); - if (!r) { - payload = set_name; - payload_len = strlen(set_name) + 1; - } + if (r) + return bf_rpack_key_err(r, "bf_matcher.payload"); + + if (!sets || + !(set = bf_hashset_get(sets, &(struct bf_set) {.name = set_name}))) + return bf_err_r(-ENOENT, "set '%s' not found", set_name); + + payload = set; + payload_len = sizeof(*set); } else { r = bf_rpack_kv_bin(node, "payload", &payload, &payload_len); + if (r) + return bf_rpack_key_err(r, "bf_matcher.payload"); } - if (r) - return bf_rpack_key_err(r, "bf_matcher.payload"); - r = _bf_matcher_new(&_matcher, type, op, payload, payload_len, negate); + r = bf_matcher_new(&_matcher, type, op, payload, payload_len, negate); if (r) return bf_err_r(r, "failed to create bf_matcher from pack"); @@ -1511,6 +1517,20 @@ int bf_matcher_new_from_copy(struct bf_matcher **dest, payload_len = src->len - sizeof(*src); + if (src->type == BF_MATCHER_SET) { + const struct bf_set *set; + + if (payload_len != sizeof(struct bf_set *)) + return bf_err_r(-EINVAL, "invalid source set matcher payload"); + + memcpy(&set, src->payload, sizeof(struct bf_set *)); + if (!set) + return bf_err_r(-EINVAL, "missing source set matcher payload"); + + return bf_matcher_new(dest, src->type, src->op, set, sizeof(*set), + src->negate); + } + return _bf_matcher_new(dest, src->type, src->op, payload_len ? src->payload : NULL, payload_len, src->negate); @@ -1535,11 +1555,15 @@ int bf_matcher_pack(const struct bf_matcher *matcher, bf_wpack_t *pack) bf_wpack_kv_int(pack, "type", matcher->type); bf_wpack_kv_int(pack, "op", matcher->op); bf_wpack_kv_bool(pack, "negate", matcher->negate); - if (matcher->type == BF_MATCHER_SET) - bf_wpack_kv_str(pack, "payload", (const char *)matcher->payload); - else + if (matcher->type == BF_MATCHER_SET) { + const struct bf_set *set; + + memcpy(&set, matcher->payload, sizeof(struct bf_set *)); + bf_wpack_kv_str(pack, "payload", set->name); + } else { bf_wpack_kv_bin(pack, "payload", matcher->payload, matcher->len - sizeof(*matcher)); + } return bf_wpack_is_valid(pack) ? 0 : -EINVAL; } diff --git a/src/libbpfilter/rule.c b/src/libbpfilter/rule.c index 3c181c96..482e738d 100644 --- a/src/libbpfilter/rule.c +++ b/src/libbpfilter/rule.c @@ -66,7 +66,8 @@ int bf_rule_new(struct bf_rule **rule) return 0; } -int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node) +int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node, + const bf_hashset *sets) { _free_bf_rule_ struct bf_rule *_rule = NULL; bf_rpack_node_t m_nodes, m_node; @@ -134,7 +135,7 @@ int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node) _free_bf_matcher_ struct bf_matcher *matcher = NULL; r = bf_list_emplace(&_rule->matchers, bf_matcher_new_from_pack, matcher, - m_node); + m_node, sets); if (r) { return bf_err_r( r, "failed to unpack bf_matcher into bf_rule.matchers"); diff --git a/tests/harness/Chain.hpp b/tests/harness/Chain.hpp index 48692335..de362ab9 100644 --- a/tests/harness/Chain.hpp +++ b/tests/harness/Chain.hpp @@ -101,7 +101,7 @@ class Chain } for (const Rule &rule: _rules) { - auto bfrule = rule.get(); + auto bfrule = rule.get(chain); r = bf_chain_add_rule(chain, bfrule.get()); if (r != 0) { bf_chain_free(&chain); diff --git a/tests/harness/Matcher.hpp b/tests/harness/Matcher.hpp index 817ccfb4..d18a1700 100644 --- a/tests/harness/Matcher.hpp +++ b/tests/harness/Matcher.hpp @@ -13,6 +13,7 @@ #include "Set.hpp" extern "C" { +#include #include } @@ -82,12 +83,17 @@ class Matcher return _negate; } - [[nodiscard]] std::unique_ptr get() const + [[nodiscard]] std::unique_ptr + get(const bf_chain *chain = nullptr) const { struct bf_matcher *matcher = nullptr; + const bf_set *set = _set.get(); + if (_type == BF_MATCHER_SET && chain) + set = bf_chain_get_set_by_name(chain, set->name); + const void *payload = _type == BF_MATCHER_SET ? - static_cast(_set.get()) : + static_cast(set) : _payload.data(); size_t payload_len = _type == BF_MATCHER_SET ? sizeof(bf_set) : _payload.size(); diff --git a/tests/harness/Rule.hpp b/tests/harness/Rule.hpp index 11cc8429..968496ea 100644 --- a/tests/harness/Rule.hpp +++ b/tests/harness/Rule.hpp @@ -58,7 +58,8 @@ class Rule return *this; } - [[nodiscard]] std::unique_ptr get() const + [[nodiscard]] std::unique_ptr + get(const bf_chain *chain = nullptr) const { struct bf_rule *rule; @@ -74,7 +75,7 @@ class Rule rule->verdict = _verdict; for (const Matcher &matcher: _matchers) { - auto bfmatcher = matcher.get(); + auto bfmatcher = matcher.get(chain); r = bf_list_add_tail(&rule->matchers, bfmatcher.get()); if (r != 0) diff --git a/tests/harness/test.c b/tests/harness/test.c index 2850d7ce..2948e3ae 100644 --- a/tests/harness/test.c +++ b/tests/harness/test.c @@ -312,6 +312,19 @@ bool bft_rule_equal(const struct bf_rule *rule0, const struct bf_rule *rule1) bool bft_matcher_equal(const struct bf_matcher *matcher0, const struct bf_matcher *matcher1) { + if (bf_matcher_get_type(matcher0) == BF_MATCHER_SET && + bf_matcher_get_type(matcher1) == BF_MATCHER_SET) { + const struct bf_set *set0; + const struct bf_set *set1; + + memcpy(&set0, bf_matcher_payload(matcher0), sizeof(set0)); + memcpy(&set1, bf_matcher_payload(matcher1), sizeof(set1)); + return bf_matcher_get_op(matcher0) == bf_matcher_get_op(matcher1) && + bf_matcher_get_negate(matcher0) == + bf_matcher_get_negate(matcher1) && + bft_set_eq_ordered(set0, set1); + } + return bf_matcher_get_type(matcher0) == bf_matcher_get_type(matcher1) && bf_matcher_get_op(matcher0) == bf_matcher_get_op(matcher1) && bf_matcher_get_negate(matcher0) == bf_matcher_get_negate(matcher1) && diff --git a/tests/unit/libbpfilter/chain.c b/tests/unit/libbpfilter/chain.c index bf82acfd..7e14e910 100644 --- a/tests/unit/libbpfilter/chain.c +++ b/tests/unit/libbpfilter/chain.c @@ -41,6 +41,35 @@ static void new_and_free(void **state) } } +static void new_failure_preserves_inputs(void **state) +{ + _clean_bf_hashset_ bf_hashset sets = bf_set_hashset_default(); + _clean_bf_list_ bf_list rules = bf_list_default(bf_rule_free, NULL); + _free_bf_chain_ struct bf_chain *chain = NULL; + _free_bf_set_ struct bf_set *set = NULL; + _free_bf_rule_ struct bf_rule *rule = NULL; + + (void)state; + + assert_ok( + bf_set_new_from_raw(&set, "addresses", "(ip4.saddr)", "{192.0.2.1}")); + assert_ok(bf_rule_new(&rule)); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, set, + sizeof(*set), false)); + bf_rule_mark_set(rule, 1); + assert_ok(bf_hashset_add(&sets, (void **)&set)); + assert_ok(bf_list_push(&rules, (void **)&rule)); + + // XDP chains cannot set packet marks. + assert_err(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, + &sets, &rules)); + assert_null(chain); + assert_int_equal(bf_hashset_size(&sets), 1); + assert_int_equal(bf_list_size(&rules), 1); + assert_non_null( + bf_hashset_get(&sets, &(struct bf_set) {.name = "addresses"})); +} + static void pack_and_unpack(void **state) { _free_bf_chain_ struct bf_chain *chain0 = NULL; @@ -77,6 +106,40 @@ static void copy_preserves_configuration(void **state) assert_true(bft_chain_equal(source, copy)); } +static void copy_rebinds_shared_set_references(void **state) +{ + _free_bf_chain_ struct bf_chain *source = + bft_chain_with_set("(ip4.saddr)", "{192.0.2.1}", false); + _free_bf_chain_ struct bf_chain *copy = NULL; + const struct bf_set *source_set; + const struct bf_set *copied_set; + struct bf_rule *rule; + uint32_t address = htonl(0xc0000201); + + (void)state; + + assert_non_null(source); + source_set = bf_chain_get_set_by_name(source, "addresses"); + rule = bf_list_get_at(&source->rules, 0); + assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, + source_set, sizeof(*source_set), true)); + + assert_ok(bf_chain_new_from_copy(©, source)); + copied_set = bf_chain_get_set_by_name(copy, "addresses"); + assert_non_null(copied_set); + assert_ptr_not_equal(source_set, copied_set); + assert_int_equal(bf_hashset_size(©->sets), 1); + bf_chain_free(&source); + + rule = bf_list_get_at(©->rules, 0); + assert_int_equal(bf_list_size(&rule->matchers), 2); + bf_list_foreach (&rule->matchers, node) { + assert_ptr_equal(copied_set, bf_chain_get_set_for_matcher( + copy, bf_list_node_get_data(node))); + } + assert_true(bf_hashset_contains(&copied_set->elems, &address)); +} + static void copy_resets_chain_counters(void **state) { _free_bf_chain_ struct bf_chain *source = bft_chain_dummy(false); @@ -184,15 +247,17 @@ static void copy_missing_set_leaves_destination_unchanged(void **state) bft_chain_with_set("(ip4.saddr)", "{192.0.2.1}", false); _free_bf_chain_ struct bf_chain *destination = bft_chain_dummy(false); struct bf_chain *original = destination; + void *taken = NULL; (void)state; assert_non_null(source); assert_non_null(destination); - assert_ok(bf_hashset_delete(&source->sets, - bf_chain_get_set_by_name(source, "addresses"))); + assert_ok(bf_hashset_take( + &source->sets, bf_chain_get_set_by_name(source, "addresses"), &taken)); assert_err(bf_chain_new_from_copy(&destination, source)); + assert_ok(bf_hashset_add(&source->sets, &taken)); assert_ptr_equal(destination, original); assert_true(bf_list_is_empty(&destination->rules)); assert_true(bf_hashset_is_empty(&destination->sets)); @@ -642,6 +707,7 @@ static void named_reference_stability(void **state) _free_bf_set_ struct bf_set *target = NULL; _free_bf_set_ struct bf_set *unrelated = NULL; _free_bf_rule_ struct bf_rule *rule = NULL; + _free_bf_rule_ struct bf_rule *foreign_rule = NULL; struct bf_set unrelated_key = {.name = "unrelated"}; const struct bf_set *stored; struct bf_matcher *matcher; @@ -691,6 +757,12 @@ static void named_reference_stability(void **state) assert_err(bf_chain_add_set(chain, target)); assert_non_null(target); assert_ptr_equal(stored, bf_chain_get_set_by_name(chain, "target")); + + // A different object with the same name cannot reference the stored set. + assert_ok(bf_rule_new(&foreign_rule)); + assert_ok(bf_rule_add_matcher(foreign_rule, BF_MATCHER_SET, BF_MATCHER_IN, + target, sizeof(*target), false)); + assert_err(bf_chain_add_rule(chain, foreign_rule)); } static void unpack_duplicate_sets(void **state) @@ -739,8 +811,10 @@ int main(void) { const struct CMUnitTest tests[] = { cmocka_unit_test(new_and_free), + cmocka_unit_test(new_failure_preserves_inputs), cmocka_unit_test(pack_and_unpack), cmocka_unit_test(copy_preserves_configuration), + cmocka_unit_test(copy_rebinds_shared_set_references), cmocka_unit_test(copy_resets_chain_counters), cmocka_unit_test(copy_set_updates_do_not_change_source), cmocka_unit_test(copy_set_matcher_survives_source_free), diff --git a/tests/unit/libbpfilter/matcher.c b/tests/unit/libbpfilter/matcher.c index a60c9c3a..1a183c81 100644 --- a/tests/unit/libbpfilter/matcher.c +++ b/tests/unit/libbpfilter/matcher.c @@ -40,7 +40,7 @@ static void new_and_free(void **state) assert_non_null(matcher); } -static void set_name_payload(void **state) +static void set_pointer_round_trip(void **state) { enum bf_matcher_type key[] = {BF_MATCHER_IP4_SADDR}; @@ -49,6 +49,7 @@ static void set_name_payload(void **state) (void)state; for (size_t i = 0; i < ARRAY_SIZE(names); ++i) { + _clean_bf_hashset_ bf_hashset sets = bf_set_hashset_default(); _free_bf_set_ struct bf_set *set = NULL; _free_bf_matcher_ struct bf_matcher *matcher = NULL; _free_bf_matcher_ struct bf_matcher *restored = NULL; @@ -63,10 +64,7 @@ static void set_name_payload(void **state) assert_non_null(name = strdup(set->name)); assert_ok(bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, set, sizeof(*set), true)); - assert_string_equal(bf_matcher_payload(matcher), name); - assert_int_equal(bf_matcher_payload_len(matcher), strlen(name) + 1); - bf_set_free(&set); - assert_string_equal(bf_matcher_payload(matcher), name); + assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_wpack_new(&wpack)); assert_ok(bf_matcher_pack(matcher, wpack)); @@ -75,8 +73,17 @@ static void set_name_payload(void **state) assert_ok( bf_rpack_kv_str(bf_rpack_root(rpack), "payload", &serialized_name)); assert_string_equal(serialized_name, name); - assert_ok(bf_matcher_new_from_pack(&restored, bf_rpack_root(rpack))); + assert_err( + bf_matcher_new_from_pack(&restored, bf_rpack_root(rpack), NULL)); + assert_null(restored); + assert_ok( + bf_matcher_new_from_pack(&restored, bf_rpack_root(rpack), &sets)); assert_true(bft_matcher_equal(matcher, restored)); + bf_matcher_free(&matcher); + bf_matcher_free(&restored); + assert_ok(bf_set_add_elem_raw( + bf_hashset_get(&sets, &(struct bf_set) {.name = name}), + "192.0.2.1")); } } @@ -120,7 +127,7 @@ static void set_legacy_payload_rejected(void **state) bf_wpack_kv_bin(wpack, "payload", &index, sizeof(index)); assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_err(bf_matcher_new_from_pack(&matcher, bf_rpack_root(rpack))); + assert_err(bf_matcher_new_from_pack(&matcher, bf_rpack_root(rpack), NULL)); assert_null(matcher); } @@ -190,11 +197,14 @@ static void copy_payload_survives_source_free(void **state) assert_ok(bf_set_new_from_raw(&set, "addresses", "(ip4.saddr)", "{}")); assert_ok(bf_matcher_new(&source, BF_MATCHER_SET, BF_MATCHER_IN, set, sizeof(*set), true)); - bf_set_free(&set); assert_ok(bf_matcher_new_from_copy(©, source)); + assert_true(bft_matcher_equal(source, copy)); bf_matcher_free(&source); - assert_string_equal(bf_matcher_payload(copy), "addresses"); + assert_ok(bf_set_add_elem_raw(set, "192.0.2.1")); + assert_ok(bf_matcher_new(&source, BF_MATCHER_SET, BF_MATCHER_IN, set, + sizeof(*set), true)); + assert_true(bft_matcher_equal(source, copy)); assert_true(bf_matcher_get_negate(copy)); } @@ -234,7 +244,8 @@ static void pack_and_unpack(void **state) // Unpack into destination assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); // Verify they're equal assert_true(bft_matcher_equal(source, destination)); @@ -261,7 +272,8 @@ static void pack_and_unpack_small_payload(void **state) // Unpack assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); assert_true(bft_matcher_equal(source, destination)); } @@ -289,7 +301,8 @@ static void pack_and_unpack_with_negate(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); // Verify negate round-trips correctly assert_true(bf_matcher_get_negate(destination)); @@ -314,7 +327,7 @@ static void unpack_without_negate_defaults_false(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&matcher, bf_rpack_root(rpack))); + assert_ok(bf_matcher_new_from_pack(&matcher, bf_rpack_root(rpack), NULL)); assert_int_equal(bf_matcher_get_type(matcher), BF_MATCHER_TCP_SPORT); assert_int_equal(bf_matcher_get_op(matcher), BF_MATCHER_EQ); @@ -881,7 +894,8 @@ static void meta_probability_pack_unpack(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); assert_true(bft_matcher_equal(source, destination)); bf_matcher_free(&source); @@ -897,7 +911,8 @@ static void meta_probability_pack_unpack(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); assert_true(bft_matcher_equal(source, destination)); } @@ -2071,7 +2086,8 @@ static void meta_flow_probability_pack_unpack(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); assert_true(bft_matcher_equal(source, destination)); bf_matcher_free(&source); @@ -2087,7 +2103,8 @@ static void meta_flow_probability_pack_unpack(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_ok(bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_ok( + bf_matcher_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); assert_true(bft_matcher_equal(source, destination)); } @@ -2098,7 +2115,7 @@ int main(void) cmocka_unit_test(new_and_free), cmocka_unit_test(copy_payload_survives_source_free), cmocka_unit_test(copy_preserves_empty_payload), - cmocka_unit_test(set_name_payload), + cmocka_unit_test(set_pointer_round_trip), cmocka_unit_test(set_invalid_input), cmocka_unit_test(set_legacy_payload_rejected), cmocka_unit_test(new_with_payload), diff --git a/tests/unit/libbpfilter/rule.c b/tests/unit/libbpfilter/rule.c index 10e7c7c5..38a6b00c 100644 --- a/tests/unit/libbpfilter/rule.c +++ b/tests/unit/libbpfilter/rule.c @@ -38,9 +38,9 @@ static void new_and_free(void **state) static void copy_configuration_and_ownership(void **state) { + _free_bf_set_ struct bf_set *set = NULL; _free_bf_rule_ struct bf_rule *source = bft_rule_dummy(2); _free_bf_rule_ struct bf_rule *copy = NULL; - _free_bf_set_ struct bf_set *set = NULL; const struct bf_matcher *set_matcher; (void)state; @@ -67,7 +67,9 @@ static void copy_configuration_and_ownership(void **state) bf_rule_free(&source); set_matcher = bf_list_node_get_data(bf_list_get_tail(©->matchers)); - assert_string_equal(bf_matcher_payload(set_matcher), "rule_copy_set"); + assert_int_equal(bf_matcher_get_type(set_matcher), BF_MATCHER_SET); + assert_true(bf_matcher_get_negate(set_matcher)); + assert_ok(bf_set_add_elem_raw(set, "192.0.2.1")); } static void copy_failure_preserves_destination(void **state) @@ -109,7 +111,7 @@ static void pack_and_unpack(void **state) // Unpack in the destination counter assert_ok(bf_rpack_new(&rpack, data, data_len)); assert_ok(bf_rpack_kv_obj(bf_rpack_root(rpack), "rule", &node)); - assert_ok(bf_rule_new_from_pack(&destination, node)); + assert_ok(bf_rule_new_from_pack(&destination, node, NULL)); assert_true(bft_rule_equal(source, destination)); } @@ -129,7 +131,7 @@ static void unpack_error(void **state) assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); assert_ok(bf_rpack_new(&rpack, data, data_len)); - assert_err(bf_rule_new_from_pack(&destination, bf_rpack_root(rpack))); + assert_err(bf_rule_new_from_pack(&destination, bf_rpack_root(rpack), NULL)); } static void dump(void **state) From 0fd2b531916f51db5c50649efed7df0be4c67864 Mon Sep 17 00:00:00 2001 From: Quentin Deslandes Date: Fri, 30 Oct 2026 10:13:01 +0100 Subject: [PATCH 3/3] lib: matcher: own anonymous sets without generated names Let each matcher own its anonymous set and reserve chain collections for named declarations. Construct anonymous sets inside matchers so callers never transfer raw anonymous set pointers. Serialize anonymous sets inside their matchers and deep-copy them when copying a matcher. Collect named and anonymous sets during generation. --- src/bfcli/parser.y | 13 +- src/libbpfilter/cgen/program.c | 22 +++ src/libbpfilter/chain.c | 18 +++ src/libbpfilter/include/bpfilter/chain.h | 10 +- src/libbpfilter/include/bpfilter/matcher.h | 73 +++++++++- src/libbpfilter/include/bpfilter/rule.h | 22 +-- src/libbpfilter/include/bpfilter/set.h | 14 +- src/libbpfilter/matcher.c | 158 +++++++++++++++++++-- src/libbpfilter/set.c | 93 +++--------- tests/e2e/rules/set_grouping.sh | 14 ++ tests/harness/Chain.hpp | 14 +- tests/harness/Matcher.hpp | 29 +++- tests/harness/Rule.hpp | 3 +- tests/harness/Set.hpp | 21 +-- tests/harness/fake.c | 33 +++-- tests/harness/fake.h | 5 + tests/harness/test.c | 3 +- tests/unit/libbpfilter/chain.c | 105 ++++++++++---- tests/unit/libbpfilter/matcher.c | 143 +++++++++++++++++-- tests/unit/libbpfilter/rule.c | 3 +- tests/unit/libbpfilter/set.c | 33 +---- 21 files changed, 599 insertions(+), 230 deletions(-) diff --git a/src/bfcli/parser.y b/src/bfcli/parser.y index dde21918..da601a81 100644 --- a/src/bfcli/parser.y +++ b/src/bfcli/parser.y @@ -531,7 +531,6 @@ matcher : matcher_type negate matcher_op RAW_PAYLOAD | SET_TYPE negate matcher_op SET_RAW_PAYLOAD { _free_bf_matcher_ struct bf_matcher *matcher = NULL; - _free_bf_set_ struct bf_set *set = NULL; _cleanup_free_ const char *raw_key = $1; _cleanup_free_ const char *payload = $4; enum bf_matcher_op op = $3; @@ -540,18 +539,10 @@ matcher : matcher_type negate matcher_op RAW_PAYLOAD if (op != BF_MATCHER_IN) bf_parse_err("only the 'in' operator is supported for sets"); - r = bf_set_new_from_raw(&set, NULL, raw_key, payload); - if (r) - bf_parse_err("failed to create new set"); - - r = bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, set, sizeof(*set), $2); + r = bf_matcher_new_for_anon_set_from_raw(&matcher, raw_key, payload, $2); if (r) bf_parse_err("failed to create a new matcher"); - r = bf_hashset_add(&ruleset->sets, (void **)&set); - if (r) - bf_parse_err("failed to add new set to the ruleset"); - $$ = TAKE_PTR(matcher); } | SET_TYPE negate matcher_op STRING @@ -578,7 +569,7 @@ matcher : matcher_type negate matcher_op RAW_PAYLOAD if (found_set->n_comps != test_key->n_comps || memcmp(found_set->key, test_key->key, found_set->n_comps * sizeof(enum bf_matcher_type))) bf_parse_err("using named set '%s', but key doesn't match", name); - r = bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, found_set, sizeof(*found_set), $2); + r = bf_matcher_new_for_named_set(&matcher, found_set, $2); if (r) bf_parse_err("failed to create a new matcher"); diff --git a/src/libbpfilter/cgen/program.c b/src/libbpfilter/cgen/program.c index 82e3304a..9ce7c0e5 100644 --- a/src/libbpfilter/cgen/program.c +++ b/src/libbpfilter/cgen/program.c @@ -223,6 +223,28 @@ static int _bf_program_build_set_groups(struct bf_program *program) return r; } + bf_list_foreach (&program->runtime.chain->rules, rule_node) { + const struct bf_rule *rule = bf_list_node_get_data(rule_node); + + bf_list_foreach (&rule->matchers, matcher_node) { + const struct bf_matcher *matcher = + bf_list_node_get_data(matcher_node); + const struct bf_set *set; + int r; + + if (bf_matcher_get_type(matcher) != BF_MATCHER_SET) + continue; + + memcpy(&set, bf_matcher_payload(matcher), sizeof(struct bf_set *)); + if (!bf_set_is_anonymous(set)) + continue; + + r = _bf_program_add_set_to_group(program, set); + if (r) + return r; + } + } + return 0; } diff --git a/src/libbpfilter/chain.c b/src/libbpfilter/chain.c index 7182bb39..05c3bcf2 100644 --- a/src/libbpfilter/chain.c +++ b/src/libbpfilter/chain.c @@ -292,6 +292,15 @@ int bf_chain_new(struct bf_chain **chain, const char *name, enum bf_hook hook, return bf_err_r(-EINVAL, "invalid policy '%s'", bf_verdict_to_str(policy)); + if (sets) { + bf_hashset_foreach (sets, set_node) { + const struct bf_set *set = set_node->data; + + if (!bf_set_is_valid_name(set->name)) + return bf_err_r(-EINVAL, "chain sets must have valid names"); + } + } + _chain = calloc(1, sizeof(*_chain)); if (!_chain) return -ENOMEM; @@ -363,6 +372,9 @@ int bf_chain_new_from_pack(struct bf_chain **chain, bf_rpack_node_t node) if (r) return bf_err_r(r, "failed to unpack bf_set into bf_chain.sets"); + if (bf_set_is_anonymous(set)) + return bf_err_r(-EINVAL, "anonymous set outside a matcher"); + r = bf_hashset_add(&sets, (void **)&set); if (r) return bf_err_r(r, "failed to add unpacked set to chain"); @@ -497,6 +509,9 @@ int bf_chain_add_set(struct bf_chain *chain, struct bf_set *set) { assert(chain && set); + if (!bf_set_is_valid_name(set->name)) + return bf_err_r(-EINVAL, "only named sets can be added to a chain"); + return bf_hashset_add(&chain->sets, (void **)&set); } @@ -513,6 +528,9 @@ bf_chain_get_set_for_matcher(const struct bf_chain *chain, return NULL; memcpy(&set, bf_matcher_payload(matcher), sizeof(struct bf_set *)); + if (bf_set_is_anonymous(set)) + return set; + return bf_hashset_get(&chain->sets, set) == set ? set : NULL; } diff --git a/src/libbpfilter/include/bpfilter/chain.h b/src/libbpfilter/include/bpfilter/chain.h index badd9de2..bbcd74ae 100644 --- a/src/libbpfilter/include/bpfilter/chain.h +++ b/src/libbpfilter/include/bpfilter/chain.h @@ -55,6 +55,8 @@ struct bf_chain uint8_t flags; enum bf_hook hook; enum bf_verdict policy; + + /// Owning collection of named sets. Anonymous sets belong to matchers. bf_hashset sets; bf_list rules; @@ -77,7 +79,7 @@ struct bf_chain * @param name Name of the chain. Can't be NULL. * @param hook Expected hook to attach the chain to. * @param policy Default action of the chain if no rule matched. - * @param sets Owning collection of sets used by `rules`, initialized with + * @param sets Owning collection of named sets, initialized with * bf_set_hashset_default(). Names must be unique within the chain. * @param rules List of rules. * @return 0 on success, or negative errno value on failure. @@ -136,11 +138,10 @@ void bf_chain_dump(const struct bf_chain *chain, prefix_t *prefix); int bf_chain_add_rule(struct bf_chain *chain, struct bf_rule *rule); /** - * @brief Insert a set into a chain. + * @brief Insert a named set into a chain. * * The chain will own the set and is responsible for freeing it. To reference - * the set in a matcher, pass the set pointer and sizeof(*set) to - * bf_matcher_new(). The matcher borrows the set pointer. + * the set in a matcher, use `bf_matcher_new_for_named_set`. * * @param chain Chain to insert the rule into. Can't be NULL. * @param set Set to insert into the chain. Can't be NULL. @@ -155,6 +156,7 @@ int bf_chain_add_set(struct bf_chain *chain, struct bf_set *set); * @param matcher Matching filtering on a set. Can't be NULL. * @return The set `matcher` filters on, or NULL if the set can't be found or * if `matcher->type` is not `BF_MATCHER_SET`. + * The pointer is borrowed. Anonymous sets belong to their matcher. */ const struct bf_set * bf_chain_get_set_for_matcher(const struct bf_chain *chain, diff --git a/src/libbpfilter/include/bpfilter/matcher.h b/src/libbpfilter/include/bpfilter/matcher.h index 0ed420ae..6aec3efa 100644 --- a/src/libbpfilter/include/bpfilter/matcher.h +++ b/src/libbpfilter/include/bpfilter/matcher.h @@ -40,6 +40,7 @@ #define _free_bf_matcher_ __attribute__((__cleanup__(bf_matcher_free))) struct bf_matcher; +struct bf_set; /** * Matcher type. @@ -269,14 +270,14 @@ struct bf_matcher_meta * * @param matcher Matcher object to allocate and initialise. Can't be NULL. On * success, contain a pointer to the matcher object, unchanged on error. - * @param type Matcher type. + * @param type Matcher type. `BF_MATCHER_SET` will be rejected: use + * `bf_matcher_new_for_named_set`, `bf_matcher_new_for_anon_set`, or + * `bf_matcher_new_for_anon_set_from_raw` instead. * @param op Comparison operator. * @param payload Payload of the matcher, its content and size depends on * @p type . Can be NULL but only if @p payload_len is 0, in which case - * there is no payload. For BF_MATCHER_SET, pass a pointer to a bf_set. - * The constructor borrows the set. Its owner must outlive the matcher. - * @param payload_len Length of the input payload. For BF_MATCHER_SET, pass - * `sizeof(struct bf_set *)`. The stored payload contains the set pointer. + * there is no payload. + * @param payload_len Length of the input payload. * @param negate If true, negate the comparison result. * @return 0 on success, or negative errno value on failure. */ @@ -284,6 +285,57 @@ int bf_matcher_new(struct bf_matcher **matcher, enum bf_matcher_type type, enum bf_matcher_op op, const void *payload, size_t payload_len, bool negate); +/** + * @brief Create a matcher that borrows an existing named set. + * + * The matcher uses `BF_MATCHER_SET` and `BF_MATCHER_IN`. The set's owner must + * outlive the matcher. `set` must be part of the chain `matcher` will be + * inserted into. + * + * @param matcher Destination matcher. Unchanged on failure. Can't be NULL. + * @param set Set to borrow. Must have a valid non-NULL name. + * @param negate Whether to negate membership. + * @return 0 on success, or a negative errno value on failure. + */ +int bf_matcher_new_for_named_set(struct bf_matcher **matcher, + const struct bf_set *set, bool negate); + +/** + * @brief Create a matcher that owns a new anonymous set. + * + * The matcher uses `BF_MATCHER_SET` and `BF_MATCHER_IN`. The caller's elements + * are copied. The anonymous set is freed with the matcher. + * + * @param matcher Destination matcher. Unchanged on failure. Can't be NULL. + * @param key Set key components. Can't be NULL. + * @param n_comps Number of key components. + * @param elems Array of element pointers. Each element contains the packed + * key components. Can be NULL when `n_elems` is zero. + * @param n_elems Number of elements in `elems`. + * @param negate Whether to negate membership. + * @return 0 on success, or a negative errno value on failure. + */ +int bf_matcher_new_for_anon_set(struct bf_matcher **matcher, + const enum bf_matcher_type *key, size_t n_comps, + const void * const *elems, size_t n_elems, + bool negate); + +/** + * @brief Create a matcher that owns a new anonymous set from raw values. + * + * The matcher uses `BF_MATCHER_SET` and `BF_MATCHER_IN`. Partial allocations + * are freed on failure. The anonymous set is freed with the matcher. + * + * @param matcher Destination matcher. Unchanged on failure. Can't be NULL. + * @param raw_key Set key, as accepted by `bf_set_new_from_raw()`. Can't be NULL. + * @param raw_payload Set elements to parse. Can't be NULL. + * @param negate Whether to negate membership. + * @return 0 on success, or a negative errno value on failure. + */ +int bf_matcher_new_for_anon_set_from_raw(struct bf_matcher **matcher, + const char *raw_key, + const char *raw_payload, bool negate); + /** * @brief Allocate and initialise a new matcher from a raw payload (string). * @@ -307,7 +359,9 @@ int bf_matcher_new_from_raw(struct bf_matcher **matcher, * matcher. Can't be NULL. * @param node Serialized matcher. Can't be NULL. * @param sets Sets used to resolve serialized names. Can be NULL if the - * matcher does not reference a set. The collection retains ownership. + * matcher does not reference a named set. Anonymous sets are loaded + * inline and owned by the matcher. `sets` retains ownership of the + * named sets. * @return 0 on success, or negative error value on failure. */ int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node, @@ -316,7 +370,12 @@ int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node, /** * @brief Allocate a copy of a matcher and its payload. * - * Set matchers borrow the same set as the source matcher. + * Anonymous sets are deep-copied. Named sets remain borrowed from the same + * owner as the source matcher. + * + * @warning If you manually deep-copy a chain (i.e. without + * `bf_chain_new_from_copy`), you also need to update the reference to named + * sets contained in the matchers. * * @param dest Destination matcher. Unchanged on failure. Can't be NULL. * @param src Matcher to copy. Can't be NULL. diff --git a/src/libbpfilter/include/bpfilter/rule.h b/src/libbpfilter/include/bpfilter/rule.h index 45413476..191b0341 100644 --- a/src/libbpfilter/include/bpfilter/rule.h +++ b/src/libbpfilter/include/bpfilter/rule.h @@ -113,8 +113,8 @@ int bf_rule_new(struct bf_rule **rule); * data. The caller will own the object. On failure, `*rule` is * unchanged. Can't be NULL. * @param node Node containing the serialized rule. Can't be NULL. - * @param sets Sets used to resolve matcher references. Can be NULL if no - * matcher references a set. The collection retains ownership. + * @param sets Sets used to resolve named matcher references. Can be NULL if + * no matcher references a named set. * @return 0 on success, or a negative errno value on failure. */ int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node, @@ -123,13 +123,14 @@ int bf_rule_new_from_pack(struct bf_rule **rule, bf_rpack_node_t node, /** * @brief Allocate a copy of a rule and its matchers. * - * Sets remain borrowed from the same owner as the source rule. That owner - * must outlive the copied rule, including its destruction. + * Anonymous sets are deep-copied. Named sets remain borrowed from the same + * owner as the source rule. That owner must outlive the copied rule, including + * its destruction. * * Runtime counters and disabled state are reset. Adding the copy to a chain * assigns its index, validates set references, and recalculates disabled state. * - * @warning Before inserting the copy into another chain, replace its set + * @warning Before inserting the copy into another chain, replace its named-set * matchers with matchers that borrow that chain's sets. Matching names alone * do not rebind references. To copy a complete chain, use * `bf_chain_new_from_copy()`, which rebinds references to the copied sets. @@ -170,14 +171,13 @@ void bf_rule_dump(const struct bf_rule *rule, prefix_t *prefix); * Create a new matcher and add it to the rule. * * @param rule Rule to add the matcher to. Can't be NULL. - * @param type Matcher type. + * @param type Matcher type. `BF_MATCHER_SET` is not supported. Create set + * matchers with their constructors and add them to `rule->matchers`. * @param op Comparison operator. * @param payload Payload of the matcher, its content and size depends on @p - * type . Can be NULL but only if @p payload_len is 0, in which case - * there is no payload. For BF_MATCHER_SET, pass a pointer to a bf_set. - * The matcher borrows the set. Its owner must outlive the matcher. - * @param payload_len Length of the input payload. For BF_MATCHER_SET, pass - * sizeof(struct bf_set). + * type . Can be NULL but only if `payload_len` is 0, in which case + * there is no payload. + * @param payload_len Length of the input payload. * @param negate If true, negate the matcher's comparison. * @return 0 on success, or negative errno value on failure. */ diff --git a/src/libbpfilter/include/bpfilter/set.h b/src/libbpfilter/include/bpfilter/set.h index e55d6b31..3ffcf873 100644 --- a/src/libbpfilter/include/bpfilter/set.h +++ b/src/libbpfilter/include/bpfilter/set.h @@ -44,8 +44,7 @@ */ struct bf_set { - /** Immutable name of the set. Anonymous sets receive a generated name. - * Never NULL. */ + /** Immutable name of the set. NULL for anonymous sets. */ const char *name; /** Key of the set. Can't contain more than `BF_SET_MAX_N_COMPS` types. */ @@ -85,7 +84,7 @@ struct bf_set bool bf_set_is_valid_name(const char *name); /** - * @brief Check whether a set has a generated name. + * @brief Check whether a set is anonymous (no name). * * @param set Set to check. Can't be NULL. * @return true if the set is anonymous, false if it has a user-defined name. @@ -93,7 +92,7 @@ bool bf_set_is_valid_name(const char *name); bool bf_set_is_anonymous(const struct bf_set *set); /** - * @brief Initialize an empty owning collection of sets, keyed by name. + * @brief Initialize an empty owning collection of named sets. * * @return An empty hashset that preserves insertion order and frees its sets. */ @@ -104,7 +103,7 @@ bf_hashset bf_set_hashset_default(void); * * @param set Set to allocate and initialise. Can't be NULL. * @param name Name of the set. Non-NULL names must satisfy - * `bf_set_is_valid_name()`. + * `bf_set_is_valid_name()`. NULL creates an anonymous set. * @param key Key of the set, as an array of `bf_matcher_type`. Not all the * matcher types can be used as set key components. Can't be NULL. * @param n_comps Number of components in `key`. @@ -118,7 +117,7 @@ int bf_set_new(struct bf_set **set, const char *name, * * @param set Set to allocate and initialise. Can't be NULL. * @param name Name of the set. Non-NULL names must satisfy - * `bf_set_is_valid_name()`. + * `bf_set_is_valid_name()`. NULL creates an anonymous set. * @param raw_key Set key, as a string of comma-separated matcher types enclosed * in parentheses. Can't be NULL. * @param raw_payload Set payload, to parse according to `raw_key`. Can't be @@ -131,9 +130,6 @@ int bf_set_new_from_raw(struct bf_set **set, const char *name, /** * @brief Allocate and initialize a new set from serialized data. * - * Validates the stored name, accepting user-defined and internal generated - * names. - * * @param set Set object to allocate and initialize from the serialized data. * The caller will own the object. On failure, `*set` is unchanged. * Can't be NULL. diff --git a/src/libbpfilter/matcher.c b/src/libbpfilter/matcher.c index 47a844b3..0a59eb3a 100644 --- a/src/libbpfilter/matcher.c +++ b/src/libbpfilter/matcher.c @@ -1397,17 +1397,92 @@ int bf_matcher_new(struct bf_matcher **matcher, enum bf_matcher_type type, { assert(matcher); - if (type == BF_MATCHER_SET) { - const struct bf_set *set = payload; + if (type == BF_MATCHER_SET) + return bf_err_r(-EINVAL, "use a set matcher constructor"); + + return _bf_matcher_new(matcher, type, op, payload, payload_len, negate); +} + +int bf_matcher_new_for_named_set(struct bf_matcher **matcher, + const struct bf_set *set, bool negate) +{ + assert(matcher); + + if (!set || !bf_set_is_valid_name(set->name)) + return bf_err_r(-EINVAL, "set matcher requires a named set"); + + return _bf_matcher_new(matcher, BF_MATCHER_SET, BF_MATCHER_IN, &set, + sizeof(struct bf_set *), negate); +} + +static int _bf_matcher_new_for_anon_set(struct bf_matcher **matcher, + struct bf_set **set, bool negate) +{ + int r; + + assert(matcher); + assert(set && *set); + + r = _bf_matcher_new(matcher, BF_MATCHER_SET, BF_MATCHER_IN, set, + sizeof(struct bf_set *), negate); + if (r) + return r; + + TAKE_PTR(*set); + + return 0; +} + +int bf_matcher_new_for_anon_set(struct bf_matcher **matcher, + const enum bf_matcher_type *key, size_t n_comps, + const void * const *elems, size_t n_elems, + bool negate) +{ + _free_bf_set_ struct bf_set *set = NULL; + int r; + + assert(matcher); + assert(key); + + if (n_elems && !elems) + return bf_err_r(-EINVAL, "missing anonymous set elements"); - if (!set || payload_len != sizeof(*set) || !set->name || !*set->name) - return bf_err_r(-EINVAL, "set matcher requires a set object"); + r = bf_set_new(&set, NULL, key, n_comps); + if (r) + return r; - return _bf_matcher_new(matcher, type, op, &set, sizeof(struct bf_set *), - negate); + r = bf_set_reserve(set, n_elems); + if (r) + return r; + + for (size_t i = 0; i < n_elems; ++i) { + if (!elems[i]) + return bf_err_r(-EINVAL, "missing anonymous set element"); + + r = bf_set_add_elem(set, elems[i]); + if (r) + return r; } - return _bf_matcher_new(matcher, type, op, payload, payload_len, negate); + return _bf_matcher_new_for_anon_set(matcher, &set, negate); +} + +int bf_matcher_new_for_anon_set_from_raw(struct bf_matcher **matcher, + const char *raw_key, + const char *raw_payload, bool negate) +{ + _free_bf_set_ struct bf_set *set = NULL; + int r; + + assert(matcher); + assert(raw_key); + assert(raw_payload); + + r = bf_set_new_from_raw(&set, NULL, raw_key, raw_payload); + if (r) + return r; + + return _bf_matcher_new_for_anon_set(matcher, &set, negate); } int bf_matcher_new_from_raw(struct bf_matcher **matcher, @@ -1476,6 +1551,31 @@ int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node, if (type == BF_MATCHER_SET) { const struct bf_set *set; + if (op != BF_MATCHER_IN) + return bf_err_r(-EINVAL, "invalid set matcher operator"); + + if (bf_rpack_kv_contains(node, "set")) { + _free_bf_set_ struct bf_set *anonymous = NULL; + bf_rpack_node_t set_node; + + if (bf_rpack_kv_contains(node, "payload")) + return bf_err_r(-EINVAL, "ambiguous set matcher payload"); + + r = bf_rpack_kv_obj(node, "set", &set_node); + if (r) + return bf_rpack_key_err(r, "bf_matcher.set"); + + r = bf_set_new_from_pack(&anonymous, set_node); + if (r) + return r; + if (!bf_set_is_anonymous(anonymous)) { + return bf_err_r(-EINVAL, + "inline matcher set must be anonymous"); + } + + return _bf_matcher_new_for_anon_set(matcher, &anonymous, negate); + } + r = bf_rpack_kv_str(node, "payload", &set_name); if (r) return bf_rpack_key_err(r, "bf_matcher.payload"); @@ -1492,7 +1592,10 @@ int bf_matcher_new_from_pack(struct bf_matcher **matcher, bf_rpack_node_t node, return bf_rpack_key_err(r, "bf_matcher.payload"); } - r = bf_matcher_new(&_matcher, type, op, payload, payload_len, negate); + if (type == BF_MATCHER_SET) + r = bf_matcher_new_for_named_set(&_matcher, payload, negate); + else + r = bf_matcher_new(&_matcher, type, op, payload, payload_len, negate); if (r) return bf_err_r(r, "failed to create bf_matcher from pack"); @@ -1520,15 +1623,26 @@ int bf_matcher_new_from_copy(struct bf_matcher **dest, if (src->type == BF_MATCHER_SET) { const struct bf_set *set; - if (payload_len != sizeof(struct bf_set *)) - return bf_err_r(-EINVAL, "invalid source set matcher payload"); + if (src->op != BF_MATCHER_IN || payload_len != sizeof(struct bf_set *)) + return bf_err_r(-EINVAL, + "invalid source set matcher payload or operator"); memcpy(&set, src->payload, sizeof(struct bf_set *)); if (!set) return bf_err_r(-EINVAL, "missing source set matcher payload"); - return bf_matcher_new(dest, src->type, src->op, set, sizeof(*set), - src->negate); + if (bf_set_is_anonymous(set)) { + _free_bf_set_ struct bf_set *copy = NULL; + int r; + + r = bf_set_new_from_copy(©, set); + if (r) + return r; + + return _bf_matcher_new_for_anon_set(dest, ©, src->negate); + } + + return bf_matcher_new_for_named_set(dest, set, src->negate); } return _bf_matcher_new(dest, src->type, src->op, @@ -1543,6 +1657,14 @@ void bf_matcher_free(struct bf_matcher **matcher) if (!*matcher) return; + if ((*matcher)->type == BF_MATCHER_SET) { + struct bf_set *set; + + memcpy(&set, (*matcher)->payload, sizeof(struct bf_set *)); + if (bf_set_is_anonymous(set)) + bf_set_free(&set); + } + free(*matcher); *matcher = NULL; } @@ -1559,7 +1681,17 @@ int bf_matcher_pack(const struct bf_matcher *matcher, bf_wpack_t *pack) const struct bf_set *set; memcpy(&set, matcher->payload, sizeof(struct bf_set *)); - bf_wpack_kv_str(pack, "payload", set->name); + if (bf_set_is_anonymous(set)) { + int r; + + bf_wpack_open_object(pack, "set"); + r = bf_set_pack(set, pack); + if (r) + return r; + bf_wpack_close_object(pack); + } else { + bf_wpack_kv_str(pack, "payload", set->name); + } } else { bf_wpack_kv_bin(pack, "payload", matcher->payload, matcher->len - sizeof(*matcher)); diff --git a/src/libbpfilter/set.c b/src/libbpfilter/set.c index e2992a8e..6b7cf319 100644 --- a/src/libbpfilter/set.c +++ b/src/libbpfilter/set.c @@ -10,8 +10,6 @@ #include #include #include -#include -#include #include "bpfilter/core/hashset.h" #include "bpfilter/dump.h" @@ -21,9 +19,6 @@ #define _BF_SET_NAME_CHARS \ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-" -#define _BF_SET_ANON_NAME_PREFIX "@anon:" -#define _BF_SET_ANON_NAME_ID_LEN 16 -#define _BF_SET_ANON_NAME_ID_CHARS "0123456789abcdef" /// Mask value of matcher types supporting LPM trie maps. #define _BF_SET_USE_TRIE_MASK \ @@ -97,43 +92,20 @@ bool bf_set_is_valid_name(const char *name) bool bf_set_is_anonymous(const struct bf_set *set) { assert(set); - assert(set->name); - return strncmp(set->name, _BF_SET_ANON_NAME_PREFIX, - sizeof(_BF_SET_ANON_NAME_PREFIX) - 1) == 0; + return !set->name; } -static bool _bf_set_is_valid_internal_name(const char *name) -{ - const char *identifier; - - assert(name); - - if (bf_set_is_valid_name(name)) - return true; - - if (strncmp(name, _BF_SET_ANON_NAME_PREFIX, - sizeof(_BF_SET_ANON_NAME_PREFIX) - 1) != 0) - return false; - - identifier = name + sizeof(_BF_SET_ANON_NAME_PREFIX) - 1; - - return strlen(identifier) == _BF_SET_ANON_NAME_ID_LEN && - strspn(identifier, _BF_SET_ANON_NAME_ID_CHARS) == - _BF_SET_ANON_NAME_ID_LEN; -} - -static int _bf_set_new(struct bf_set **set, const char *name, - const enum bf_matcher_type *key, size_t n_comps) +int bf_set_new(struct bf_set **set, const char *name, + const enum bf_matcher_type *key, size_t n_comps) { _free_bf_set_ struct bf_set *_set = NULL; uint64_t mask = 0; assert(set); - assert(name); assert(key); - if (!_bf_set_is_valid_internal_name(name)) + if (name && !bf_set_is_valid_name(name)) return bf_err_r(-EINVAL, "invalid set name '%s'", name); if (n_comps == 0) @@ -149,9 +121,11 @@ static int _bf_set_new(struct bf_set **set, const char *name, if (!_set) return -ENOMEM; - _set->name = strdup(name); - if (!_set->name) - return bf_err_r(-ENOMEM, "failed to allocate memory for set name"); + if (name) { + _set->name = strdup(name); + if (!_set->name) + return bf_err_r(-ENOMEM, "failed to allocate memory for set name"); + } memcpy(&_set->key, key, n_comps * sizeof(enum bf_matcher_type)); _set->n_comps = n_comps; @@ -186,36 +160,6 @@ static int _bf_set_new(struct bf_set **set, const char *name, return 0; } -int bf_set_new(struct bf_set **set, const char *name, - const enum bf_matcher_type *key, size_t n_comps) -{ - char generated_name[sizeof(_BF_SET_ANON_NAME_PREFIX) + - _BF_SET_ANON_NAME_ID_LEN] = _BF_SET_ANON_NAME_PREFIX; - - assert(set); - assert(key); - - if (name && !bf_set_is_valid_name(name)) - return bf_err_r(-EINVAL, "invalid user-defined set name '%s'", name); - - // Generate a random identifier for anonymous sets. - if (!name) { - uint8_t random_bytes[_BF_SET_ANON_NAME_ID_LEN]; - char *identifier = - generated_name + sizeof(_BF_SET_ANON_NAME_PREFIX) - 1; - - if (getentropy(random_bytes, sizeof(random_bytes)) < 0) - return bf_err_r(-errno, "failed to generate anonymous set name"); - - for (size_t i = 0; i < ARRAY_SIZE(random_bytes); ++i) - identifier[i] = _BF_SET_ANON_NAME_ID_CHARS[random_bytes[i] % 16]; - - name = generated_name; - } - - return _bf_set_new(set, name, key, n_comps); -} - /** * @brief Parse a set's raw key into an array of `bf_matcher_type`. * @@ -449,10 +393,16 @@ int bf_set_new_from_pack(struct bf_set **set, bf_rpack_node_t node) assert(set); - r = bf_rpack_kv_str(node, "name", &name); + r = bf_rpack_kv_node(node, "name", &child); if (r) return bf_rpack_key_err(r, "bf_set.name"); + if (!bf_rpack_is_nil(child)) { + r = bf_rpack_str(child, &name); + if (r) + return bf_rpack_key_err(r, "bf_set.name"); + } + r = bf_rpack_kv_array(node, "key", &child); if (r) return bf_rpack_key_err(r, "bf_set.key"); @@ -470,7 +420,7 @@ int bf_set_new_from_pack(struct bf_set **set, bf_rpack_node_t node) return bf_rpack_key_err(r, "bf_set.key"); } - r = _bf_set_new(&_set, name, key, n_comps); + r = bf_set_new(&_set, name, key, n_comps); if (r) return bf_err_r(r, "failed to create bf_set from pack"); @@ -513,7 +463,7 @@ int bf_set_new_from_copy(struct bf_set **dest, const struct bf_set *src) assert(dest); assert(src); - r = _bf_set_new(©, src->name, src->key, src->n_comps); + r = bf_set_new(©, src->name, src->key, src->n_comps); if (r) return r; @@ -554,7 +504,10 @@ int bf_set_pack(const struct bf_set *set, bf_wpack_t *pack) assert(set); assert(pack); - bf_wpack_kv_str(pack, "name", set->name); + if (set->name) + bf_wpack_kv_str(pack, "name", set->name); + else + bf_wpack_kv_nil(pack, "name"); bf_wpack_open_array(pack, "key"); for (size_t i = 0; i < set->n_comps; ++i) @@ -581,7 +534,7 @@ void bf_set_dump(const struct bf_set *set, prefix_t *prefix) DUMP(prefix, "struct bf_set at %p", set); bf_dump_prefix_push(prefix); - DUMP(prefix, "name: %s", set->name); + DUMP(prefix, "name: %s", set->name ?: ""); DUMP(prefix, "key: bf_matcher_type[%zu]", set->n_comps); bf_dump_prefix_push(prefix); for (size_t i = 0; i < set->n_comps; ++i) { diff --git a/tests/e2e/rules/set_grouping.sh b/tests/e2e/rules/set_grouping.sh index ea519660..337e2c4d 100755 --- a/tests/e2e/rules/set_grouping.sh +++ b/tests/e2e/rules/set_grouping.sh @@ -35,6 +35,20 @@ ${FROM_NS} ${BFCLI} chain set --from-str "chain isolation BF_HOOK_XDP{ifindex=${ ping -c 1 -W 1 ${NS_IP_ADDR} || { echo "ERROR: ping should have succeeded"; exit 1; } ${FROM_NS} ${BFCLI} chain flush --name isolation +# Named declarations and anonymous matchers must keep independent contents. +${FROM_NS} ${BFCLI} chain set --from-str "chain mixed BF_HOOK_XDP{ifindex=${NS_IFINDEX}} ACCEPT + set unused (ip4.saddr) in { ${HOST_IP_ADDR} } + rule (ip4.saddr) in { 192.0.2.40 } counter DROP + rule (ip4.saddr) in { ${HOST_IP_ADDR} } counter DROP" +(! ping -c 1 -W 1 ${NS_IP_ADDR}) || { echo "ERROR: anonymous set should drop ping"; exit 1; } +test "$(get_counter mixed 0)" = "0" +test "$(get_counter mixed 1)" -ge 1 + +dump=$(${FROM_NS} ${BFCLI} chain get --name mixed) +[[ "${dump}" == *"set unused ("* ]] || { echo "ERROR: unused named declaration was lost"; exit 1; } +[[ "${dump}" != *"@anon:"* ]] || { echo "ERROR: anonymous sets must not have names"; exit 1; } +${FROM_NS} ${BFCLI} chain flush --name mixed + # Multibyte bitmask: more than 8 same-key sets force a 2-byte value # (1 byte per 8 sets). The host's address sits in s8 only (bit_index 8 -> # byte 1, bit 0). Rule 0 references s0 (byte 0, bit 0) and must miss; diff --git a/tests/harness/Chain.hpp b/tests/harness/Chain.hpp index de362ab9..48c4f621 100644 --- a/tests/harness/Chain.hpp +++ b/tests/harness/Chain.hpp @@ -6,6 +6,7 @@ #pragma once #include +#include #include extern "C" { @@ -89,29 +90,32 @@ class Chain if (r != 0) throw std::runtime_error("failed to create bf_chain"); + std::unique_ptr owned(chain); + for (const Set &set: _sets) { + if (bf_set_is_anonymous(set.ref().get())) + continue; + auto bfset = set.get(); r = bf_chain_add_set(chain, bfset.get()); if (r != 0) { - bf_chain_free(&chain); throw std::runtime_error("failed to add bf_set to bf_chain"); } - bfset.release(); + std::ignore = bfset.release(); } for (const Rule &rule: _rules) { auto bfrule = rule.get(chain); r = bf_chain_add_rule(chain, bfrule.get()); if (r != 0) { - bf_chain_free(&chain); throw std::runtime_error("failed to add bf_rule to bf_chain"); } - bfrule.release(); + std::ignore = bfrule.release(); } - return std::unique_ptr(chain); + return owned; } }; diff --git a/tests/harness/Matcher.hpp b/tests/harness/Matcher.hpp index d18a1700..55a51986 100644 --- a/tests/harness/Matcher.hpp +++ b/tests/harness/Matcher.hpp @@ -89,16 +89,31 @@ class Matcher struct bf_matcher *matcher = nullptr; const bf_set *set = _set.get(); + if (_type == BF_MATCHER_SET && bf_set_is_anonymous(set)) { + std::vector elements; + + for (auto *node = set->elems.head; node; node = node->next) + elements.push_back(node->data); + + int r = bf_matcher_new_for_anon_set(&matcher, set->key, + set->n_comps, elements.data(), + elements.size(), _negate); + if (r != 0) + throw std::runtime_error( + "failed to create anonymous set matcher"); + + return std::unique_ptr(matcher); + } + if (_type == BF_MATCHER_SET && chain) set = bf_chain_get_set_by_name(chain, set->name); - const void *payload = _type == BF_MATCHER_SET ? - static_cast(set) : - _payload.data(); - size_t payload_len = - _type == BF_MATCHER_SET ? sizeof(bf_set) : _payload.size(); - int r = - bf_matcher_new(&matcher, _type, _op, payload, payload_len, _negate); + int r; + if (_type == BF_MATCHER_SET) + r = bf_matcher_new_for_named_set(&matcher, set, _negate); + else + r = bf_matcher_new(&matcher, _type, _op, _payload.data(), + _payload.size(), _negate); if (r != 0) throw std::runtime_error("failed to create bf_matcher"); diff --git a/tests/harness/Rule.hpp b/tests/harness/Rule.hpp index 968496ea..e997b58d 100644 --- a/tests/harness/Rule.hpp +++ b/tests/harness/Rule.hpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "Matcher.hpp" @@ -81,7 +82,7 @@ class Rule if (r != 0) throw std::runtime_error("failed to add bf_matcher to bf_rule"); - bfmatcher.release(); + std::ignore = bfmatcher.release(); } return owned; diff --git a/tests/harness/Set.hpp b/tests/harness/Set.hpp index 3ba9a7d5..67d86130 100644 --- a/tests/harness/Set.hpp +++ b/tests/harness/Set.hpp @@ -6,8 +6,8 @@ #pragma once #include +#include #include -#include #include extern "C" { @@ -30,14 +30,14 @@ class Set }; std::shared_ptr _prototype; - std::vector> _elements; public: - Set(std::vector key) + Set(std::vector key, std::optional name = {}) { struct bf_set *set = nullptr; - if (bf_set_new(&set, nullptr, key.data(), key.size()) != 0) + if (bf_set_new(&set, name ? name->c_str() : nullptr, key.data(), + key.size()) != 0) throw std::runtime_error("failed to create a new bf_set"); _prototype = std::shared_ptr(set, deleter()); @@ -50,7 +50,8 @@ class Set "number of elements in the vector should match the number of components in the set key"); } - _elements.push_back(element); + if (bf_set_add_elem(_prototype.get(), element.data()) != 0) + throw std::runtime_error("failed to add element to bf_set"); return *this; } @@ -63,16 +64,6 @@ class Set if (r != 0) throw std::runtime_error("failed to create a new bf_set"); - for (const auto &element: _elements) { - r = bf_set_add_elem( - set, const_cast( - reinterpret_cast(element.data()))); - if (r != 0) { - bf_set_free(&set); - throw std::runtime_error("failed to add element to bf_set"); - } - } - return std::unique_ptr(set); } diff --git a/tests/harness/fake.c b/tests/harness/fake.c index 980e39d7..253ada07 100644 --- a/tests/harness/fake.c +++ b/tests/harness/fake.c @@ -83,6 +83,21 @@ const void *bft_get_randomly_filled_buffer(size_t len) return TAKE_PTR(buffer); } +int bft_rule_add_named_set_matcher(struct bf_rule *rule, + const struct bf_set *set, bool negate) +{ + _free_bf_matcher_ struct bf_matcher *matcher = NULL; + int r; + + assert(rule); + + r = bf_matcher_new_for_named_set(&matcher, set, negate); + if (r) + return r; + + return bf_list_push(&rule->matchers, (void **)&matcher); +} + struct bf_chain *bft_chain_dummy(bool with_rules) { _free_bf_chain_ struct bf_chain *chain = NULL; @@ -96,17 +111,16 @@ struct bf_chain *bft_chain_dummy(bool with_rules) if (with_rules) { { // Create a rule with SET, any matcher, SET + _free_bf_set_ struct bf_set *set0 = bft_set_dummy("first", 4); + _free_bf_set_ struct bf_set *set1 = bft_set_dummy("second", 4); _free_bf_rule_ struct bf_rule *rule = NULL; - _free_bf_set_ struct bf_set *set0 = bft_set_dummy(NULL, 4); - _free_bf_set_ struct bf_set *set1 = bft_set_dummy(NULL, 4); uint32_t ip = 0xff; r = bf_rule_new(&rule); if (r) return NULL; - r = bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, set0, - sizeof(*set0), false); + r = bft_rule_add_named_set_matcher(rule, set0, false); if (r) return NULL; @@ -115,8 +129,7 @@ struct bf_chain *bft_chain_dummy(bool with_rules) if (r) return NULL; - r = bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, set1, - sizeof(*set1), false); + r = bft_rule_add_named_set_matcher(rule, set1, false); if (r) return NULL; @@ -148,15 +161,14 @@ struct bf_chain *bft_chain_dummy(bool with_rules) { // Create a rule with a single set, no other matcher + _free_bf_set_ struct bf_set *set0 = bft_set_dummy("third", 4); _free_bf_rule_ struct bf_rule *rule = NULL; - _free_bf_set_ struct bf_set *set0 = bft_set_dummy(NULL, 4); r = bf_rule_new(&rule); if (r) return NULL; - r = bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, set0, - sizeof(*set0), false); + r = bft_rule_add_named_set_matcher(rule, set0, false); if (r) return NULL; @@ -198,8 +210,7 @@ struct bf_chain *bft_chain_with_set(const char *key, const char *elements, rule->verdict = BF_VERDICT_DROP; rule->has_counters = 1; - r = bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, set, - sizeof(*set), negate); + r = bft_rule_add_named_set_matcher(rule, set, negate); if (r) return NULL; diff --git a/tests/harness/fake.h b/tests/harness/fake.h index 709d5f8c..defc8a6b 100644 --- a/tests/harness/fake.h +++ b/tests/harness/fake.h @@ -10,6 +10,9 @@ #include +struct bf_rule; +struct bf_set; + typedef bool (*bft_list_eq_cb)(const void *, const void *); typedef int (*bft_list_dummy_inserter)(bf_list *, void *); @@ -66,5 +69,7 @@ struct bf_chain *bft_chain_with_set(const char *key, const char *elements, bool negate); struct bf_rule *bft_rule_dummy(size_t n_matchers); +int bft_rule_add_named_set_matcher(struct bf_rule *rule, + const struct bf_set *set, bool negate); struct bf_matcher *bft_matcher_dummy(const void *data, size_t data_len); struct bf_set *bft_set_dummy(const char *name, size_t n_elems); diff --git a/tests/harness/test.c b/tests/harness/test.c index 2948e3ae..0951d676 100644 --- a/tests/harness/test.c +++ b/tests/harness/test.c @@ -265,7 +265,8 @@ bool bft_set_eq_ordered(const struct bf_set *lhs, const struct bf_set *rhs) return false; } - return bf_streq(lhs->name, rhs->name) && lhs->n_comps == rhs->n_comps && + return (lhs->name == rhs->name || bf_streq(lhs->name, rhs->name)) && + lhs->n_comps == rhs->n_comps && 0 == memcmp(lhs->key, rhs->key, sizeof(enum bf_matcher_type) * lhs->n_comps) && lhs->use_trie == rhs->use_trie && lhs->min_size == rhs->min_size; diff --git a/tests/unit/libbpfilter/chain.c b/tests/unit/libbpfilter/chain.c index 7e14e910..a85383f8 100644 --- a/tests/unit/libbpfilter/chain.c +++ b/tests/unit/libbpfilter/chain.c @@ -54,8 +54,7 @@ static void new_failure_preserves_inputs(void **state) assert_ok( bf_set_new_from_raw(&set, "addresses", "(ip4.saddr)", "{192.0.2.1}")); assert_ok(bf_rule_new(&rule)); - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, set, - sizeof(*set), false)); + assert_ok(bft_rule_add_named_set_matcher(rule, set, false)); bf_rule_mark_set(rule, 1); assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_list_push(&rules, (void **)&rule)); @@ -121,8 +120,7 @@ static void copy_rebinds_shared_set_references(void **state) assert_non_null(source); source_set = bf_chain_get_set_by_name(source, "addresses"); rule = bf_list_get_at(&source->rules, 0); - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, - source_set, sizeof(*source_set), true)); + assert_ok(bft_rule_add_named_set_matcher(rule, source_set, true)); assert_ok(bf_chain_new_from_copy(©, source)); copied_set = bf_chain_get_set_by_name(copy, "addresses"); @@ -263,6 +261,66 @@ static void copy_missing_set_leaves_destination_unchanged(void **state) assert_true(bf_hashset_is_empty(&destination->sets)); } +static void anonymous_matchers_survive_chain_copy(void **state) +{ + _free_bf_chain_ struct bf_chain *source = + bft_chain_with_set("(ip4.saddr)", "{192.0.2.1}", false); + _free_bf_chain_ struct bf_chain *copy = NULL; + _free_bf_chain_ struct bf_chain *restored = NULL; + _free_bf_set_ struct bf_set *unused = NULL; + _free_bf_matcher_ struct bf_matcher *matcher = NULL; + _free_bf_wpack_ bf_wpack_t *wpack = NULL; + _free_bf_rpack_ bf_rpack_t *rpack = NULL; + struct bf_rule *rule; + const void *data; + size_t data_len; + + (void)state; + + assert_non_null(source); + assert_ok(bf_set_new_from_raw(&unused, "unused", "(tcp.dport)", "{80}")); + assert_ok(bf_chain_add_set(source, unused)); + TAKE_PTR(unused); + rule = bf_list_get_at(&source->rules, 0); + assert_ok(bf_matcher_new_for_anon_set_from_raw(&matcher, "(ip4.saddr)", + "{192.0.2.2}", true)); + assert_ok(bf_list_push(&rule->matchers, (void **)&matcher)); + + assert_ok(bf_chain_new_from_copy(©, source)); + assert_true(bft_chain_equal(source, copy)); + bf_chain_free(&source); + assert_int_equal(bf_hashset_size(©->sets), 2); + assert_non_null(bf_chain_get_set_by_name(copy, "unused")); + + assert_ok(bf_wpack_new(&wpack)); + assert_ok(bf_chain_pack(copy, wpack)); + assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); + assert_ok(bf_rpack_new(&rpack, data, data_len)); + assert_ok(bf_chain_new_from_pack(&restored, bf_rpack_root(rpack))); + assert_true(bft_chain_equal(copy, restored)); + bf_chain_free(©); + + rule = bf_list_get_at(&restored->rules, 0); + bf_list_delete(&rule->matchers, bf_list_get_tail(&rule->matchers)); + assert_int_equal(bf_hashset_size(&restored->sets), 2); + assert_non_null(bf_chain_get_set_by_name(restored, "addresses")); + assert_non_null(bf_chain_get_set_by_name(restored, "unused")); +} + +static void reject_anonymous_chain_set(void **state) +{ + _free_bf_chain_ struct bf_chain *chain = bft_chain_dummy(false); + _free_bf_set_ struct bf_set *set = NULL; + + (void)state; + + assert_non_null(chain); + assert_ok(bf_set_new_from_raw(&set, NULL, "(ip4.saddr)", "{192.0.2.1}")); + assert_int_equal(bf_chain_add_set(chain, set), -EINVAL); + assert_true(bf_hashset_is_empty(&chain->sets)); + assert_ok(bf_set_add_elem_raw(set, "192.0.2.2")); +} + static void dump(void **state) { _free_bf_chain_ struct bf_chain *chain0 = bft_chain_dummy(false); @@ -314,8 +372,8 @@ static void mixed_enabled_disabled_log_flag(void **state) _free_bf_chain_ struct bf_chain *chain = NULL; _clean_bf_hashset_ bf_hashset sets = bf_set_hashset_default(); _clean_bf_list_ bf_list rules = bf_list_default(bf_rule_free, bf_rule_pack); - _free_bf_set_ struct bf_set *set0 = bft_set_dummy(NULL, 0); - _free_bf_set_ struct bf_set *set1 = bft_set_dummy(NULL, 4); + _free_bf_set_ struct bf_set *set0 = bft_set_dummy("empty", 0); + _free_bf_set_ struct bf_set *set1 = bft_set_dummy("populated", 4); struct bf_rule *r0 = NULL; struct bf_rule *r1 = NULL; @@ -325,13 +383,11 @@ static void mixed_enabled_disabled_log_flag(void **state) assert_non_null(set1); assert_ok(bf_rule_new(&r0)); r0->log = 1; - assert_ok(bf_rule_add_matcher(r0, BF_MATCHER_SET, BF_MATCHER_IN, set0, - sizeof(*set0), false)); + assert_ok(bft_rule_add_named_set_matcher(r0, set0, false)); assert_ok(bf_list_add_tail(&rules, r0)); assert_ok(bf_rule_new(&r1)); - assert_ok(bf_rule_add_matcher(r1, BF_MATCHER_SET, BF_MATCHER_IN, set1, - sizeof(*set1), false)); + assert_ok(bft_rule_add_named_set_matcher(r1, set1, false)); assert_ok(bf_list_add_tail(&rules, r1)); assert_ok(bf_hashset_add(&sets, (void **)&set0)); @@ -376,8 +432,7 @@ static void apply_set_delta_updates_derived_state(void **state) rule->log = BF_LOG_OPT_DEFAULT; rule->log_rate_ns = 1; - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, - matcher_set, sizeof(*matcher_set), false)); + assert_ok(bft_rule_add_named_set_matcher(rule, matcher_set, false)); assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP6_NEXTHDR, BF_MATCHER_EQ, &nexthdr, sizeof(nexthdr), false)); assert_ok(bf_list_add_tail(&rules, rule)); @@ -429,13 +484,11 @@ static void empty_set_only_disables_in_rules(void **state) assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_rule_new(&in_rule)); - assert_ok(bf_rule_add_matcher(in_rule, BF_MATCHER_SET, BF_MATCHER_IN, - matcher_set, sizeof(*matcher_set), false)); + assert_ok(bft_rule_add_named_set_matcher(in_rule, matcher_set, false)); assert_ok(bf_list_add_tail(&rules, in_rule)); assert_ok(bf_rule_new(¬_in_rule)); - assert_ok(bf_rule_add_matcher(not_in_rule, BF_MATCHER_SET, BF_MATCHER_IN, - matcher_set, sizeof(*matcher_set), true)); + assert_ok(bft_rule_add_named_set_matcher(not_in_rule, matcher_set, true)); assert_ok(bf_list_add_tail(&rules, not_in_rule)); assert_ok(bf_chain_new(&chain, "test", BF_HOOK_XDP, BF_VERDICT_ACCEPT, @@ -512,9 +565,7 @@ static void incompatible_matchers_disable_rule(void **state) assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_rule_new(&rule)); - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, - matcher_set, sizeof(*matcher_set), - false)); + assert_ok(bft_rule_add_named_set_matcher(rule, matcher_set, false)); assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_IP6_DADDR, BF_MATCHER_EQ, ip6_addr, sizeof(ip6_addr), false)); assert_ok(bf_list_add_tail(&rules, rule)); @@ -603,16 +654,15 @@ static void set_component_unsupported_hook(void **state) assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_rule_new(&rule)); - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, - matcher_set, sizeof(*matcher_set), false)); + assert_ok(bft_rule_add_named_set_matcher(rule, matcher_set, false)); assert_ok(bf_list_add_tail(&rules, rule)); assert_err(bf_chain_new(&chain, "test", BF_HOOK_CGROUP_SOCK_ADDR_CONNECT4, BF_VERDICT_ACCEPT, &sets, &rules)); // Supported component: ip4.daddr on CONNECT4 should succeed. - bf_hashset_clean(&sets); bf_list_clean(&rules); + bf_hashset_clean(&sets); enum bf_matcher_type daddr_key[] = {BF_MATCHER_IP4_DADDR}; @@ -622,8 +672,7 @@ static void set_component_unsupported_hook(void **state) assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_rule_new(&rule)); - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, - matcher_set, sizeof(*matcher_set), false)); + assert_ok(bft_rule_add_named_set_matcher(rule, matcher_set, false)); assert_ok(bf_list_add_tail(&rules, rule)); assert_ok(bf_chain_new(&chain, "test", BF_HOOK_CGROUP_SOCK_ADDR_CONNECT4, @@ -722,8 +771,7 @@ static void named_reference_stability(void **state) assert_ok( bf_set_new_from_raw(&unrelated, "unrelated", "(ip4.saddr)", "{}")); assert_ok(bf_rule_new(&rule)); - assert_ok(bf_rule_add_matcher(rule, BF_MATCHER_SET, BF_MATCHER_IN, target, - sizeof(*target), false)); + assert_ok(bft_rule_add_named_set_matcher(rule, target, false)); matcher = bf_list_node_get_data(bf_list_get_head(&rule->matchers)); assert_null(bf_chain_get_set_for_matcher(chain, matcher)); assert_err(bf_chain_add_rule(chain, rule)); @@ -760,8 +808,7 @@ static void named_reference_stability(void **state) // A different object with the same name cannot reference the stored set. assert_ok(bf_rule_new(&foreign_rule)); - assert_ok(bf_rule_add_matcher(foreign_rule, BF_MATCHER_SET, BF_MATCHER_IN, - target, sizeof(*target), false)); + assert_ok(bft_rule_add_named_set_matcher(foreign_rule, target, false)); assert_err(bf_chain_add_rule(chain, foreign_rule)); } @@ -821,6 +868,8 @@ int main(void) cmocka_unit_test(copy_preserves_disabled_empty_set_rule), cmocka_unit_test(copy_preserves_negated_empty_set_rule), cmocka_unit_test(copy_missing_set_leaves_destination_unchanged), + cmocka_unit_test(anonymous_matchers_survive_chain_copy), + cmocka_unit_test(reject_anonymous_chain_set), cmocka_unit_test(dump), cmocka_unit_test(get_set_from_matcher), cmocka_unit_test(mixed_enabled_disabled_log_flag), diff --git a/tests/unit/libbpfilter/matcher.c b/tests/unit/libbpfilter/matcher.c index 1a183c81..be44233d 100644 --- a/tests/unit/libbpfilter/matcher.c +++ b/tests/unit/libbpfilter/matcher.c @@ -44,7 +44,7 @@ static void set_pointer_round_trip(void **state) { enum bf_matcher_type key[] = {BF_MATCHER_IP4_SADDR}; - const char *names[] = {"blocked", NULL}; + const char *names[] = {"blocked", "allowed"}; (void)state; @@ -62,8 +62,7 @@ static void set_pointer_round_trip(void **state) assert_ok(bf_set_new(&set, names[i], key, ARRAY_SIZE(key))); assert_non_null(name = strdup(set->name)); - assert_ok(bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, set, - sizeof(*set), true)); + assert_ok(bf_matcher_new_for_named_set(&matcher, set, true)); assert_ok(bf_hashset_add(&sets, (void **)&set)); assert_ok(bf_wpack_new(&wpack)); @@ -87,6 +86,102 @@ static void set_pointer_round_trip(void **state) } } +static void anonymous_set_copy_and_round_trip(void **state) +{ + _free_bf_matcher_ struct bf_matcher *source = NULL; + _free_bf_matcher_ struct bf_matcher *expected = NULL; + _free_bf_matcher_ struct bf_matcher *copy = NULL; + _free_bf_matcher_ struct bf_matcher *restored = NULL; + _free_bf_wpack_ bf_wpack_t *wpack = NULL; + _free_bf_rpack_ bf_rpack_t *rpack = NULL; + const enum bf_matcher_type key[] = {BF_MATCHER_IP4_SADDR}; + uint32_t addresses[] = {htonl(0xc0000201), htonl(0xc0000202)}; + const void *elements[] = {&addresses[0], &addresses[1], &addresses[0]}; + const void *data; + size_t data_len; + + (void)state; + + assert_ok(bf_matcher_new_for_anon_set( + &source, key, ARRAY_SIZE(key), elements, ARRAY_SIZE(elements), true)); + addresses[0] = 0; + addresses[1] = 0; + assert_ok(bf_matcher_new_for_anon_set_from_raw( + &expected, "(ip4.saddr)", "{192.0.2.1; 192.0.2.2}", true)); + assert_true(bft_matcher_equal(source, expected)); + + assert_ok(bf_matcher_new_from_copy(©, source)); + bf_matcher_free(&source); + assert_true(bft_matcher_equal(copy, expected)); + + assert_ok(bf_wpack_new(&wpack)); + assert_ok(bf_matcher_pack(copy, wpack)); + assert_ok(bf_wpack_get_data(wpack, &data, &data_len)); + assert_ok(bf_rpack_new(&rpack, data, data_len)); + assert_ok(bf_matcher_new_from_pack(&restored, bf_rpack_root(rpack), NULL)); + bf_matcher_free(©); + assert_true(bft_matcher_equal(restored, expected)); +} + +static void anonymous_set_failures_preserve_destination(void **state) +{ + _free_bf_matcher_ struct bf_matcher *matcher = NULL; + const enum bf_matcher_type key[] = {BF_MATCHER_IP4_SADDR}; + uint32_t address = htonl(0xc0000201); + const void *elements[] = {&address, NULL}; + struct bf_matcher *original; + + (void)state; + + assert_ok(bf_matcher_new_for_anon_set_from_raw(&matcher, "(ip4.saddr)", + "{192.0.2.1}", false)); + original = matcher; + assert_err(bf_matcher_new_for_anon_set(&matcher, key, 0, NULL, 0, false)); + assert_ptr_equal(matcher, original); + assert_err(bf_matcher_new_for_anon_set(&matcher, key, ARRAY_SIZE(key), NULL, + 1, false)); + assert_ptr_equal(matcher, original); + assert_err(bf_matcher_new_for_anon_set( + &matcher, key, ARRAY_SIZE(key), elements, ARRAY_SIZE(elements), false)); + assert_ptr_equal(matcher, original); + assert_err(bf_matcher_new_for_anon_set_from_raw( + &matcher, "(ip4.saddr)", "{192.0.2.1; invalid}", false)); + assert_ptr_equal(matcher, original); + assert_err(bf_matcher_new_for_anon_set_from_raw(&matcher, "(invalid)", "{}", + false)); + assert_ptr_equal(matcher, original); +} + +static void anonymous_set_empty(void **state) +{ + _free_bf_matcher_ struct bf_matcher *typed = NULL; + _free_bf_matcher_ struct bf_matcher *raw = NULL; + const enum bf_matcher_type key[] = {BF_MATCHER_TCP_DPORT}; + + (void)state; + + assert_ok(bf_matcher_new_for_anon_set(&typed, key, ARRAY_SIZE(key), NULL, 0, + false)); + assert_ok( + bf_matcher_new_for_anon_set_from_raw(&raw, "(tcp.dport)", "{}", false)); + assert_true(bft_matcher_equal(typed, raw)); +} + +static void anonymous_set_pointer_rejected(void **state) +{ + _free_bf_set_ struct bf_set *set = NULL; + _free_bf_matcher_ struct bf_matcher *matcher = NULL; + + (void)state; + + assert_ok(bf_set_new_from_raw(&set, NULL, "(ip4.saddr)", "{192.0.2.1}")); + assert_int_equal(bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, + set, sizeof(*set), false), + -EINVAL); + assert_null(matcher); + assert_ok(bf_set_add_elem_raw(set, "192.0.2.2")); +} + static void set_invalid_input(void **state) { _free_bf_matcher_ struct bf_matcher *matcher = NULL; @@ -105,12 +200,39 @@ static void set_invalid_input(void **state) assert_err(bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, &set, sizeof(set), false)); set.name = "blocked"; + assert_err(bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_IN, &set, + sizeof(set), false)); assert_err(bf_matcher_new(&matcher, BF_MATCHER_SET, BF_MATCHER_EQ, &set, sizeof(set), false)); assert_null(matcher); } -static void set_legacy_payload_rejected(void **state) +static void named_set_failures_preserve_destination(void **state) +{ + _free_bf_set_ struct bf_set *set = NULL; + _free_bf_matcher_ struct bf_matcher *matcher = NULL; + struct bf_set invalid = {.name = "invalid.name"}; + struct bf_matcher *original; + + (void)state; + + assert_ok(bf_set_new_from_raw(&set, NULL, "(ip4.saddr)", "{192.0.2.1}")); + assert_ok(bf_matcher_new_for_anon_set_from_raw(&matcher, "(tcp.dport)", + "{80}", false)); + original = matcher; + assert_int_equal(bf_matcher_new_for_named_set(&matcher, NULL, false), + -EINVAL); + assert_ptr_equal(matcher, original); + assert_int_equal(bf_matcher_new_for_named_set(&matcher, set, false), + -EINVAL); + assert_ptr_equal(matcher, original); + assert_int_equal(bf_matcher_new_for_named_set(&matcher, &invalid, false), + -EINVAL); + assert_ptr_equal(matcher, original); + assert_ok(bf_set_add_elem_raw(set, "192.0.2.2")); +} + +static void set_binary_payload_rejected(void **state) { _free_bf_matcher_ struct bf_matcher *matcher = NULL; _free_bf_wpack_ bf_wpack_t *wpack = NULL; @@ -195,15 +317,13 @@ static void copy_payload_survives_source_free(void **state) bf_matcher_free(©); assert_ok(bf_set_new_from_raw(&set, "addresses", "(ip4.saddr)", "{}")); - assert_ok(bf_matcher_new(&source, BF_MATCHER_SET, BF_MATCHER_IN, set, - sizeof(*set), true)); + assert_ok(bf_matcher_new_for_named_set(&source, set, true)); assert_ok(bf_matcher_new_from_copy(©, source)); assert_true(bft_matcher_equal(source, copy)); bf_matcher_free(&source); assert_ok(bf_set_add_elem_raw(set, "192.0.2.1")); - assert_ok(bf_matcher_new(&source, BF_MATCHER_SET, BF_MATCHER_IN, set, - sizeof(*set), true)); + assert_ok(bf_matcher_new_for_named_set(&source, set, true)); assert_true(bft_matcher_equal(source, copy)); assert_true(bf_matcher_get_negate(copy)); } @@ -2116,8 +2236,13 @@ int main(void) cmocka_unit_test(copy_payload_survives_source_free), cmocka_unit_test(copy_preserves_empty_payload), cmocka_unit_test(set_pointer_round_trip), + cmocka_unit_test(anonymous_set_copy_and_round_trip), + cmocka_unit_test(anonymous_set_failures_preserve_destination), + cmocka_unit_test(anonymous_set_empty), + cmocka_unit_test(anonymous_set_pointer_rejected), cmocka_unit_test(set_invalid_input), - cmocka_unit_test(set_legacy_payload_rejected), + cmocka_unit_test(named_set_failures_preserve_destination), + cmocka_unit_test(set_binary_payload_rejected), cmocka_unit_test(new_with_payload), cmocka_unit_test(getters), cmocka_unit_test(pack_and_unpack), diff --git a/tests/unit/libbpfilter/rule.c b/tests/unit/libbpfilter/rule.c index 38a6b00c..21def26f 100644 --- a/tests/unit/libbpfilter/rule.c +++ b/tests/unit/libbpfilter/rule.c @@ -53,8 +53,7 @@ static void copy_configuration_and_ownership(void **state) bf_rule_mark_set(source, 0x1234); bf_rule_set_redirect(source, 8, BF_REDIRECT_EGRESS); assert_ok(bf_set_new_from_raw(&set, "rule_copy_set", "(ip4.saddr)", "{}")); - assert_ok(bf_rule_add_matcher(source, BF_MATCHER_SET, BF_MATCHER_IN, set, - sizeof(*set), true)); + assert_ok(bft_rule_add_named_set_matcher(source, set, true)); assert_ok(bf_rule_new_from_copy(©, source)); assert_true(bft_rule_equal(source, copy)); diff --git a/tests/unit/libbpfilter/set.c b/tests/unit/libbpfilter/set.c index 8d2be1f0..397ee076 100644 --- a/tests/unit/libbpfilter/set.c +++ b/tests/unit/libbpfilter/set.c @@ -35,7 +35,6 @@ static void name_is_valid(void **state) static void new_with_invalid_names(void **state) { _free_bf_set_ struct bf_set *set = NULL; - _clean_bft_mock_ bft_mock getentropy_mock = bft_mock_default(); enum bf_matcher_type key[] = {BF_MATCHER_IP4_SADDR}; const char *names[] = { "", "two words", "name.with.dot", "name/slash", "caf\xc3\xa9", @@ -50,13 +49,6 @@ static void new_with_invalid_names(void **state) assert_null(set); } - getentropy_mock = bft_mock_get(getentropy); - assert_err(bf_set_new(&set, NULL, key, ARRAY_SIZE(key))); - assert_null(set); - assert_err(bf_set_new_from_raw(&set, NULL, "(ip4.saddr)", "{}")); - assert_null(set); - - // User-defined names do not require entropy. assert_ok(bf_set_new(&set, "named", key, ARRAY_SIZE(key))); assert_false(bf_set_is_anonymous(set)); } @@ -66,7 +58,6 @@ static void new_and_free(void **state) _free_bf_set_ struct bf_set *set = NULL; _free_bf_set_ struct bf_set *other = NULL; _free_bf_set_ struct bf_set *copy = NULL; - _clean_bft_mock_ bft_mock getentropy_mock = bft_mock_default(); enum bf_matcher_type key[] = {BF_MATCHER_IP4_SADDR}; (void)state; @@ -83,29 +74,20 @@ static void new_and_free(void **state) // Free set using the cleanup attribute assert_ok(bf_set_new(&set, NULL, key, ARRAY_SIZE(key))); - assert_non_null(set->name); + assert_null(set->name); assert_true(bf_set_is_anonymous(set)); - assert_false(bf_set_is_valid_name(set->name)); - assert_string_not_equal(set->name, ""); - - assert_err(bf_set_new(©, set->name, key, ARRAY_SIZE(key))); - assert_null(copy); - assert_err(bf_set_new_from_raw(©, set->name, "(ip4.saddr)", "{}")); - assert_null(copy); assert_ok(bf_set_new_from_raw(&other, NULL, "(ip4.saddr)", "{192.0.2.1}")); assert_true(bf_set_is_anonymous(other)); - assert_string_not_equal(set->name, other->name); + assert_null(other->name); - // Copying must preserve the name without generating a new one. - getentropy_mock = bft_mock_get(getentropy); + // Copies of anonymous sets remain anonymous. assert_ok(bf_set_new_from_copy(©, other)); assert_true(bf_set_is_anonymous(copy)); - assert_string_equal(copy->name, other->name); + assert_null(copy->name); assert_true(bft_set_eq_ordered(copy, other)); - // After the source is freed, the copy must retain its anonymous name - // and original element while accepting another element. + // The copy remains independent after the source is freed. bf_set_free(&other); assert_true(bf_set_is_anonymous(copy)); @@ -281,7 +263,6 @@ static void pack_and_unpack_anon(void **state) _free_bf_set_ struct bf_set *copy = NULL; _free_bf_wpack_ bf_wpack_t *wpack = NULL; _free_bf_rpack_ bf_rpack_t *rpack = NULL; - _clean_bft_mock_ bft_mock getentropy_mock = bft_mock_default(); bf_rpack_node_t node; const void *data; size_t data_len; @@ -293,7 +274,6 @@ static void pack_and_unpack_anon(void **state) assert_true(bf_set_is_anonymous(source)); source->min_size = 16; - getentropy_mock = bft_mock_get(getentropy); assert_ok(bf_wpack_new(&wpack)); bf_wpack_open_object(wpack, "set"); assert_ok(bf_set_pack(source, wpack)); @@ -399,7 +379,8 @@ static void copy_empty_set_accepts_elements_after_source_free(void **state) static void pack_and_unpack_err(void **state) { const char *names[] = { - "", "two words", "name.with.dot", "name/slash", "caf\xc3\xa9", + "", "two words", "name.with.dot", + "name/slash", "caf\xc3\xa9", "@anon:0123456789abcdef", }; (void)state;