Two datatypes are using runtime datatype allocation: * Concatenations. * Integer, that require byteorder adjustment.
Add a reference counter to release the dynamic datatype object when it is shared, which occurs from the following paths: * netlink_delinearize_setelem(), when elements refer to the datatype of an existing set in the cache. * set_expr_alloc(), for implicit set/maps and update statement from the packet path. * set_ref_expr_alloc(), for set and map references via @. * expr_evaluate_set_elem(), when elements are added and they refer to the set datatype, ie. add element command. valgrind reports memleaks like this one: ==28352== 1,350 (440 direct, 910 indirect) bytes in 5 blocks are definitely lost in loss recor 3 of 3 ==28352== at 0x4C2BBAF: malloc (vg_replace_malloc.c:299) ==28352== by 0x4E79558: xmalloc (utils.c:36) ==28352== by 0x4E7963D: xzalloc (utils.c:65) ==28352== by 0x4E6029B: dtype_alloc (datatype.c:1073) ==28352== by 0x4E6029B: concat_type_alloc (datatype.c:1127) ==28352== by 0x4E6D3B3: netlink_delinearize_set (netlink.c:578) ==28352== by 0x4E6D68E: list_set_cb (netlink.c:648) ==28352== by 0x5D74023: nftnl_set_list_foreach (set.c:780) ==28352== by 0x4E6D6F3: netlink_list_sets (netlink.c:669) ==28352== by 0x4E5A7A3: cache_init_objects (rule.c:159) ==28352== by 0x4E5A7A3: cache_init (rule.c:216) ==28352== by 0x4E5A7A3: cache_update (rule.c:266) ==28352== by 0x4E7E0EE: nft_evaluate (libnftables.c:388) ==28352== by 0x4E7EADD: nft_run_cmd_from_filename (libnftables.c:479) ==28352== by 0x109A53: main (main.c:310) Remove DTYPE_F_CLONE flag, which is replaced by proper reference counter. Call datatype_free() from expr_free() path. Signed-off-by: Pablo Neira Ayuso <[email protected]> --- v3: deal with dynamic integer datatype allocations. include/datatype.h | 6 ++++-- src/datatype.c | 42 +++++++++++++++++++++++++++++++----------- src/evaluate.c | 24 ++++++++++++------------ src/expression.c | 11 ++++++----- src/netlink.c | 2 +- src/segtree.c | 9 +++++---- 6 files changed, 59 insertions(+), 35 deletions(-) diff --git a/include/datatype.h b/include/datatype.h index 14ece282902c..5765c1ba7502 100644 --- a/include/datatype.h +++ b/include/datatype.h @@ -117,12 +117,10 @@ struct expr; * * @DTYPE_F_ALLOC: datatype is dynamically allocated * @DTYPE_F_PREFIX: preferred representation for ranges is a prefix - * @DTYPE_F_CLONE: this is an instance from original datatype */ enum datatype_flags { DTYPE_F_ALLOC = (1 << 0), DTYPE_F_PREFIX = (1 << 1), - DTYPE_F_CLONE = (1 << 2), }; /** @@ -140,6 +138,7 @@ enum datatype_flags { * @print: function to print a constant of this type * @parse: function to parse a symbol and return an expression * @sym_tbl: symbol table for this type + * @refcnt: reference counter (only for DTYPE_F_ALLOC) */ struct datatype { uint32_t type; @@ -158,10 +157,13 @@ struct datatype { struct error_record *(*parse)(const struct expr *sym, struct expr **res); const struct symbol_table *sym_tbl; + unsigned int refcnt; }; extern const struct datatype *datatype_lookup(enum datatypes type); extern const struct datatype *datatype_lookup_byname(const char *name); +extern struct datatype *datatype_get(const struct datatype *dtype); +extern void datatype_free(const struct datatype *dtype); extern struct error_record *symbol_parse(const struct expr *sym, struct expr **res); diff --git a/src/datatype.c b/src/datatype.c index 1d5ed6f798de..2aabddbdd0ec 100644 --- a/src/datatype.c +++ b/src/datatype.c @@ -1072,10 +1072,22 @@ static struct datatype *dtype_alloc(void) dtype = xzalloc(sizeof(*dtype)); dtype->flags = DTYPE_F_ALLOC; + dtype->refcnt = 1; return dtype; } +struct datatype *datatype_get(const struct datatype *ptr) +{ + struct datatype *dtype = (struct datatype *)ptr; + + if (!(dtype->flags & DTYPE_F_ALLOC)) + return dtype; + + dtype->refcnt++; + return dtype; +} + static struct datatype *dtype_clone(const struct datatype *orig_dtype) { struct datatype *dtype; @@ -1084,18 +1096,26 @@ static struct datatype *dtype_clone(const struct datatype *orig_dtype) *dtype = *orig_dtype; dtype->name = xstrdup(orig_dtype->name); dtype->desc = xstrdup(orig_dtype->desc); - dtype->flags = DTYPE_F_ALLOC | DTYPE_F_CLONE; + dtype->flags = DTYPE_F_ALLOC; + dtype->refcnt = 1; return dtype; } -static void dtype_free(const struct datatype *dtype) +void datatype_free(const struct datatype *ptr) { - if (dtype->flags & DTYPE_F_ALLOC) { - xfree(dtype->name); - xfree(dtype->desc); - xfree(dtype); - } + struct datatype *dtype = (struct datatype *)ptr; + + if (!dtype) + return; + if (!(dtype->flags & DTYPE_F_ALLOC)) + return; + if (--dtype->refcnt > 0) + return; + + xfree(dtype->name); + xfree(dtype->desc); + xfree(dtype); } const struct datatype *concat_type_alloc(uint32_t type) @@ -1137,7 +1157,7 @@ const struct datatype *concat_type_alloc(uint32_t type) void concat_type_destroy(const struct datatype *dtype) { - dtype_free(dtype); + datatype_free(dtype); } const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype, @@ -1152,13 +1172,13 @@ const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype, dtype = dtype_clone(orig_dtype); dtype->byteorder = byteorder; - return dtype; + return datatype_get(dtype); } void set_datatype_destroy(const struct datatype *dtype) { - if (dtype && dtype->flags & DTYPE_F_CLONE) - dtype_free(dtype); + if (dtype) + datatype_free(dtype); } static struct error_record *time_unit_parse(const struct location *loc, diff --git a/src/evaluate.c b/src/evaluate.c index 39101b486b2f..904e65718ed5 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -229,7 +229,7 @@ static int expr_evaluate_symbol(struct eval_ctx *ctx, struct expr **expr) switch ((*expr)->symtype) { case SYMBOL_VALUE: - (*expr)->dtype = ctx->ectx.dtype; + (*expr)->dtype = datatype_get(ctx->ectx.dtype); erec = symbol_parse(*expr, &new); if (erec != NULL) { erec_queue(erec, ctx->msgs); @@ -312,7 +312,7 @@ static int expr_evaluate_string(struct eval_ctx *ctx, struct expr **exprp) prefix = prefix_expr_alloc(&expr->location, value, datalen * BITS_PER_BYTE); - prefix->dtype = ctx->ectx.dtype; + prefix->dtype = datatype_get(ctx->ectx.dtype); prefix->flags |= EXPR_F_CONSTANT; prefix->byteorder = BYTEORDER_HOST_ENDIAN; @@ -915,7 +915,7 @@ static int expr_evaluate_range(struct eval_ctx *ctx, struct expr **expr) return expr_error(ctx->msgs, range, "Range has zero or negative size"); - range->dtype = left->dtype; + range->dtype = datatype_get(left->dtype); range->flags |= EXPR_F_CONSTANT; return 0; } @@ -1239,7 +1239,7 @@ static int expr_evaluate_set_elem(struct eval_ctx *ctx, struct expr **expr) } } - elem->dtype = elem->key->dtype; + elem->dtype = datatype_get(elem->key->dtype); elem->len = elem->key->len; elem->flags = elem->key->flags; return 0; @@ -1285,7 +1285,7 @@ static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr) set->set_flags |= NFT_SET_CONSTANT; - set->dtype = ctx->ectx.dtype; + set->dtype = datatype_get(ctx->ectx.dtype); set->len = ctx->ectx.len; set->flags |= EXPR_F_CONSTANT; return 0; @@ -1311,9 +1311,9 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr) switch (map->mappings->etype) { case EXPR_SET: key = constant_expr_alloc(&map->location, - ctx->ectx.dtype, - ctx->ectx.byteorder, - ctx->ectx.len, NULL); + datatype_get(ctx->ectx.dtype), + ctx->ectx.byteorder, + ctx->ectx.len, NULL); mappings = implicit_set_declaration(ctx, "__map%d", key, @@ -1356,7 +1356,7 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr) map->mappings->set->key->dtype->desc, map->map->dtype->desc); - map->dtype = map->mappings->set->datatype; + map->dtype = datatype_get(map->mappings->set->datatype); map->flags |= EXPR_F_CONSTANT; /* Data for range lookups needs to be in big endian order */ @@ -1413,7 +1413,7 @@ static void expr_dtype_integer_compatible(struct eval_ctx *ctx, if (ctx->ectx.dtype && ctx->ectx.dtype->basetype == &integer_type && ctx->ectx.len == 4 * BITS_PER_BYTE) { - expr->dtype = ctx->ectx.dtype; + expr->dtype = datatype_get(ctx->ectx.dtype); expr->len = ctx->ectx.len; } } @@ -1554,7 +1554,7 @@ static void binop_transfer_handle_lhs(struct expr **expr) case OP_LSHIFT: case OP_XOR: tmp = expr_get(left->left); - tmp->dtype = left->dtype; + tmp->dtype = datatype_get(left->dtype); expr_free(left); *expr = tmp; break; @@ -2965,7 +2965,7 @@ static int stmt_evaluate_objref_map(struct eval_ctx *ctx, struct stmt *stmt) map->mappings->set->key->dtype->desc, map->map->dtype->desc); - map->dtype = map->mappings->set->datatype; + map->dtype = datatype_get(map->mappings->set->datatype); map->flags |= EXPR_F_CONSTANT; /* Data for range lookups needs to be in big endian order */ diff --git a/src/expression.c b/src/expression.c index ef694f2a194d..d1cfb24681d9 100644 --- a/src/expression.c +++ b/src/expression.c @@ -57,8 +57,8 @@ struct expr *expr_clone(const struct expr *expr) { struct expr *new; - new = expr_alloc(&expr->location, expr->etype, expr->dtype, - expr->byteorder, expr->len); + new = expr_alloc(&expr->location, expr->etype, + datatype_get(expr->dtype), expr->byteorder, expr->len); new->flags = expr->flags; new->op = expr->op; expr_ops(expr)->clone(new, expr); @@ -86,6 +86,8 @@ void expr_free(struct expr *expr) if (--expr->refcnt > 0) return; + datatype_free(expr->dtype); + /* EXPR_INVALID expressions lack ->ops structure. * This happens for compound types. */ @@ -803,7 +805,6 @@ void compound_expr_remove(struct expr *compound, struct expr *expr) static void concat_expr_destroy(struct expr *expr) { - concat_type_destroy(expr->dtype); compound_expr_destroy(expr); } @@ -936,7 +937,7 @@ struct expr *set_expr_alloc(const struct location *loc, const struct set *set) return set_expr; set_expr->set_flags = set->flags; - set_expr->dtype = set->key->dtype; + set_expr->dtype = datatype_get(set->key->dtype); return set_expr; } @@ -1067,7 +1068,7 @@ struct expr *set_ref_expr_alloc(const struct location *loc, struct set *set) { struct expr *expr; - expr = expr_alloc(loc, EXPR_SET_REF, set->key->dtype, 0, 0); + expr = expr_alloc(loc, EXPR_SET_REF, datatype_get(set->key->dtype), 0, 0); expr->set = set_get(set); expr->flags |= EXPR_F_CONSTANT; return expr; diff --git a/src/netlink.c b/src/netlink.c index 7a4312498ce8..c5567c0d29ca 100644 --- a/src/netlink.c +++ b/src/netlink.c @@ -767,7 +767,7 @@ int netlink_delinearize_setelem(struct nftnl_set_elem *nlse, flags = nftnl_set_elem_get_u32(nlse, NFTNL_SET_ELEM_FLAGS); key = netlink_alloc_value(&netlink_location, &nld); - key->dtype = set->key->dtype; + key->dtype = datatype_get(set->key->dtype); key->byteorder = set->key->byteorder; if (set->key->dtype->subtypes) key = netlink_parse_concat_elem(set->key->dtype, key); diff --git a/src/segtree.c b/src/segtree.c index a21270a08c46..082f2093068a 100644 --- a/src/segtree.c +++ b/src/segtree.c @@ -540,7 +540,8 @@ static void set_insert_interval(struct expr *set, struct seg_tree *tree, { struct expr *expr; - expr = constant_expr_alloc(&internal_location, tree->keytype, + expr = constant_expr_alloc(&internal_location, + datatype_get(tree->keytype), tree->byteorder, tree->keylen, NULL); mpz_set(expr->value, ei->left); expr = set_elem_expr_alloc(&internal_location, expr); @@ -606,7 +607,7 @@ static void set_elem_add(const struct set *set, struct expr *init, mpz_t value, { struct expr *expr; - expr = constant_expr_alloc(&internal_location, set->key->dtype, + expr = constant_expr_alloc(&internal_location, datatype_get(set->key->dtype), set->key->byteorder, set->key->len, NULL); mpz_set(expr->value, value); expr = set_elem_expr_alloc(&internal_location, expr); @@ -888,7 +889,7 @@ void interval_map_decompose(struct expr *set) mpz_cmp_ui(p, 0)) { struct expr *tmp; - tmp = constant_expr_alloc(&low->location, low->dtype, + tmp = constant_expr_alloc(&low->location, datatype_get(low->dtype), low->byteorder, expr_value(low)->len, NULL); @@ -964,7 +965,7 @@ void interval_map_decompose(struct expr *set) if (!low) /* no unclosed interval at end */ goto out; - i = constant_expr_alloc(&low->location, low->dtype, + i = constant_expr_alloc(&low->location, datatype_get(low->dtype), low->byteorder, expr_value(low)->len, NULL); mpz_init_bitmask(i->value, i->len); -- 2.11.0
