datatype: fix leak and cleanup reference counting for struct datatype

commit 8519ab031d8022999603a69ee9f18e8cfb06645d upstream.

Test `./tests/shell/run-tests.sh -V tests/shell/testcases/maps/nat_addr_port`
fails:

==118== 195 (112 direct, 83 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==118==    at 0x484682C: calloc (vg_replace_malloc.c:1554)
==118==    by 0x48A39DD: xmalloc (utils.c:37)
==118==    by 0x48A39DD: xzalloc (utils.c:76)
==118==    by 0x487BDFD: datatype_alloc (datatype.c:1205)
==118==    by 0x487BDFD: concat_type_alloc (datatype.c:1288)
==118==    by 0x488229D: stmt_evaluate_nat_map (evaluate.c:3786)
==118==    by 0x488229D: stmt_evaluate_nat (evaluate.c:3892)
==118==    by 0x488229D: stmt_evaluate (evaluate.c:4450)
==118==    by 0x488328E: rule_evaluate (evaluate.c:4956)
==118==    by 0x48ADC71: nft_evaluate (libnftables.c:552)
==118==    by 0x48AEC29: nft_run_cmd_from_buffer (libnftables.c:595)
==118==    by 0x402983: main (main.c:534)

I think the reference handling for datatype is wrong. It was introduced
by commit 01a13882bb59 ('src: add reference counter for dynamic
datatypes').

We don't notice it most of the time, because instances are statically
allocated, where datatype_get()/datatype_free() is a NOP.

Fix and rework.

- Commit 01a13882bb59 comments "The reference counter of any newly
  allocated datatype is set to zero". That seems not workable.
  Previously, functions like datatype_clone() would have returned the
  refcnt set to zero. Some callers would then then set the refcnt to one, but
  some wouldn't (set_datatype_alloc()). Calling datatype_free() with a
  refcnt of zero will overflow to UINT_MAX and leak:

       if (--dtype->refcnt > 0)
          return;

  While there could be schemes with such asymmetric counting that juggle the
  appropriate number of datatype_get() and datatype_free() calls, this is
  confusing and error prone. The common pattern is that every
  alloc/clone/get/ref is paired with exactly one unref/free.

  Let datatype_clone() return references with refcnt set 1 and in
  general be always clear about where we transfer ownership (take a
  reference) and where we need to release it.

- set_datatype_alloc() needs to consistently return ownership to the
  reference. Previously, some code paths would and others wouldn't.

- Replace

    datatype_set(key, set_datatype_alloc(dtype, key->byteorder))

  with a __datatype_set() with takes ownership.

Fixes: 01a13882bb59 ('src: add reference counter for dynamic datatypes')
Signed-off-by: Thomas Haller <thaller@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/datatype.h b/include/datatype.h
index 73f38f66c4ce166936a18bc49256976eeb539df2..c040c39762f3fc0313a1d1a2c9ff7a940ca1cf40 100644 (file)
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -176,6 +176,7 @@ 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_set(struct expr *expr, const struct datatype *dtype);
+extern void __datatype_set(struct expr *expr, const struct datatype *dtype);
 extern void datatype_free(const struct datatype *dtype);
 struct datatype *dtype_clone(const struct datatype *orig_dtype);
 

diff --git a/include/expression.h b/include/expression.h
index 3f06a38ae2d377d0c029d90ea727263802e6dd2f..39b89a99e7c562101ba65abe20a71a2fdff55d76 100644 (file)
--- a/include/expression.h
+++ b/include/expression.h
@@ -121,7 +121,11 @@ enum symbol_types {
  * @maxval:    expected maximum value
  */
 struct expr_ctx {
+       /* expr_ctx does not own the reference to dtype. The caller must ensure
+        * the valid lifetime.
+        */
        const struct datatype   *dtype;
+
        enum byteorder          byteorder;
        unsigned int            len;
        unsigned int            maxval;

diff --git a/src/datatype.c b/src/datatype.c
index 002ed46a99316341ac80520c43bf62da5f898b38..dcda32c8e6ddd9e28220225d2f601ec2ff6c1f1a 100644 (file)
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -1099,6 +1099,7 @@ static struct datatype *dtype_alloc(void)
 
        dtype = xzalloc(sizeof(*dtype));
        dtype->flags = DTYPE_F_ALLOC;
+       dtype->refcnt = 1;
 
        return dtype;
 }
@@ -1116,12 +1117,19 @@ struct datatype *datatype_get(const struct datatype *ptr)
        return dtype;
 }
 
+void __datatype_set(struct expr *expr, const struct datatype *dtype)
+{
+       const struct datatype *dtype_free;
+
+       dtype_free = expr->dtype;
+       expr->dtype = dtype;
+       datatype_free(dtype_free);
+}
+
 void datatype_set(struct expr *expr, const struct datatype *dtype)
 {
-       if (dtype == expr->dtype)
-               return;
-       datatype_free(expr->dtype);
-       expr->dtype = datatype_get(dtype);
+       if (dtype != expr->dtype)
+               __datatype_set(expr, datatype_get(dtype));
 }
 
 struct datatype *dtype_clone(const struct datatype *orig_dtype)
@@ -1133,7 +1141,7 @@ struct datatype *dtype_clone(const struct datatype *orig_dtype)
        dtype->name = xstrdup(orig_dtype->name);
        dtype->desc = xstrdup(orig_dtype->desc);
        dtype->flags = DTYPE_F_ALLOC | orig_dtype->flags;
-       dtype->refcnt = 0;
+       dtype->refcnt = 1;
 
        return dtype;
 }
@@ -1146,6 +1154,9 @@ void datatype_free(const struct datatype *ptr)
                return;
        if (!(dtype->flags & DTYPE_F_ALLOC))
                return;
+
+       assert(dtype->refcnt != 0);
+
        if (--dtype->refcnt > 0)
                return;
 
@@ -1198,7 +1209,7 @@ const struct datatype *set_datatype_alloc(const struct datatype *orig_dtype,
 
        /* Restrict dynamic datatype allocation to generic integer datatype. */
        if (orig_dtype != &integer_type)
-               return orig_dtype;
+               return datatype_get(orig_dtype);
 
        dtype = dtype_clone(orig_dtype);
        dtype->byteorder = byteorder;

diff --git a/src/evaluate.c b/src/evaluate.c
index 693eef331946fe81176447b5f3566fee1e47f8cb..f7fdc91deb285bd99907f1fea8c5a00d68559aa5 100644 (file)
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -75,7 +75,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
        if (dtype->byteorder == key->byteorder)
                return;
 
-       datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
+       __datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
 }
 
 static int set_evaluate(struct eval_ctx *ctx, struct set *set);
@@ -1377,8 +1377,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
                        clone = dtype_clone(i->dtype);
                        clone->size = i->len;
                        clone->byteorder = i->byteorder;
-                       clone->refcnt = 1;
-                       i->dtype = clone;
+                       __datatype_set(i, clone);
                }
 
                if (dtype == NULL && i->dtype->size == 0)
@@ -1404,7 +1403,7 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
        }
 
        (*expr)->flags |= flags;
-       datatype_set(*expr, concat_type_alloc(ntype));
+       __datatype_set(*expr, concat_type_alloc(ntype));
        (*expr)->len   = size;
 
        if (off > 0)
@@ -1741,7 +1740,6 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
 {
        struct expr *map = *expr, *mappings;
        struct expr_ctx ectx = ctx->ectx;
-       const struct datatype *dtype;
        struct expr *key, *data;
 
        if (map->map->etype == EXPR_CT &&
@@ -1782,12 +1780,16 @@ static int expr_evaluate_map(struct eval_ctx *ctx, struct expr **expr)
                                                  ctx->ectx.len, NULL);
                }
 
-               dtype = set_datatype_alloc(ectx.dtype, ectx.byteorder);
-               if (dtype->type == TYPE_VERDICT)
+               if (ectx.dtype->type == TYPE_VERDICT) {
                        data = verdict_expr_alloc(&netlink_location, 0, NULL);
-               else
+               } else {
+                       const struct datatype *dtype;
+
+                       dtype = set_datatype_alloc(ectx.dtype, ectx.byteorder);
                        data = constant_expr_alloc(&netlink_location, dtype,
                                                   dtype->byteorder, ectx.len, NULL);
+                       datatype_free(dtype);
+               }
 
                mappings = implicit_set_declaration(ctx, "__map%d",
                                                    key, data,
@@ -3541,8 +3543,10 @@ static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
 {
        struct proto_ctx *pctx = &ctx->pctx;
        struct expr *one, *two, *data, *tmp;
-       const struct datatype *dtype;
-       int addr_type, err;
+       const struct datatype *dtype = NULL;
+       const struct datatype *dtype2;
+       int addr_type;
+       int err;
 
        if (stmt->nat.family == NFPROTO_INET)
                expr_family_infer(pctx, stmt->nat.addr, &stmt->nat.family);
@@ -3562,18 +3566,23 @@ static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
        dtype = concat_type_alloc((addr_type << TYPE_BITS) | TYPE_INET_SERVICE);
 
        expr_set_context(&ctx->ectx, dtype, dtype->size);
-       if (expr_evaluate(ctx, &stmt->nat.addr))
-               return -1;
+       if (expr_evaluate(ctx, &stmt->nat.addr)) {
+               err = -1;
+               goto out;
+       }
 
        if (pctx->protocol[PROTO_BASE_TRANSPORT_HDR].desc == NULL &&
            !nat_evaluate_addr_has_th_expr(stmt->nat.addr)) {
-               return stmt_binary_error(ctx, stmt->nat.addr, stmt,
+               err = stmt_binary_error(ctx, stmt->nat.addr, stmt,
                                         "transport protocol mapping is only "
                                         "valid after transport protocol match");
+               goto out;
        }
 
-       if (stmt->nat.addr->etype != EXPR_MAP)
-               return 0;
+       if (stmt->nat.addr->etype != EXPR_MAP) {
+               err = 0;
+               goto out;
+       }
 
        data = stmt->nat.addr->mappings->set->data;
        if (data->flags & EXPR_F_INTERVAL)
@@ -3581,36 +3590,42 @@ static int stmt_evaluate_nat_map(struct eval_ctx *ctx, struct stmt *stmt)
 
        datatype_set(data, dtype);
 
-       if (expr_ops(data)->type != EXPR_CONCAT)
-               return __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
+       if (expr_ops(data)->type != EXPR_CONCAT) {
+               err = __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
                                           BYTEORDER_BIG_ENDIAN,
                                           &stmt->nat.addr);
+               goto out;
+       }
 
        one = list_first_entry(&data->expressions, struct expr, list);
        two = list_entry(one->list.next, struct expr, list);
 
-       if (one == two || !list_is_last(&two->list, &data->expressions))
-               return __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
+       if (one == two || !list_is_last(&two->list, &data->expressions)) {
+               err = __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
                                           BYTEORDER_BIG_ENDIAN,
                                           &stmt->nat.addr);
+               goto out;
+       }
 
-       dtype = get_addr_dtype(stmt->nat.family);
+       dtype2 = get_addr_dtype(stmt->nat.family);
        tmp = one;
-       err = __stmt_evaluate_arg(ctx, stmt, dtype, dtype->size,
+       err = __stmt_evaluate_arg(ctx, stmt, dtype2, dtype2->size,
                                  BYTEORDER_BIG_ENDIAN,
                                  &tmp);
        if (err < 0)
-               return err;
+               goto out;
        if (tmp != one)
                BUG("Internal error: Unexpected alteration of l3 expression");
 
        tmp = two;
        err = nat_evaluate_transport(ctx, stmt, &tmp);
        if (err < 0)
-               return err;
+               goto out;
        if (tmp != two)
                BUG("Internal error: Unexpected alteration of l4 expression");
 
+out:
+       datatype_free(dtype);
        return err;
 }
 
@@ -4316,8 +4331,7 @@ static int set_expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
                        dtype = dtype_clone(i->dtype);
                        dtype->size = i->len;
                        dtype->byteorder = i->byteorder;
-                       dtype->refcnt = 1;
-                       i->dtype = dtype;
+                       __datatype_set(i, dtype);
                }
 
                if (i->dtype->size == 0 && i->len == 0)
@@ -4340,7 +4354,7 @@ static int set_expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
        }
 
        (*expr)->flags |= flags;
-       datatype_set(*expr, concat_type_alloc(ntype));
+       __datatype_set(*expr, concat_type_alloc(ntype));
        (*expr)->len   = size;
 
        expr_set_context(&ctx->ectx, (*expr)->dtype, (*expr)->len);

diff --git a/src/expression.c b/src/expression.c
index 37788b18e1631830cd6f0fd27c1cec64a93b522a..ea11b60f2ad305332bc4b2f1309ffef4b37eb6ab 100644 (file)
--- a/src/expression.c
+++ b/src/expression.c
@@ -1012,7 +1012,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
        if (!dtype)
                goto err_free;
 
-       concat_expr->dtype = datatype_get(dtype);
+       __datatype_set(concat_expr, dtype);
        concat_expr->len = len;
 
        return concat_expr;

diff --git a/src/netlink.c b/src/netlink.c
index 4cf1a98d664614b70d7138e1d49c8787441fb6f5..a34071218be50469042980877d5e2cb5e735cc5d 100644 (file)
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -795,6 +795,10 @@ enum nft_data_types dtype_map_to_kernel(const struct datatype *dtype)
 
 static const struct datatype *dtype_map_from_kernel(enum nft_data_types type)
 {
+       /* The function always returns ownership of a reference. But for
+        * &verdict_Type and datatype_lookup(), those are static instances,
+        * we can omit the datatype_get() call.
+        */
        switch (type) {
        case NFT_DATA_VERDICT:
                return &verdict_type;
@@ -930,12 +934,14 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
        const struct nftnl_udata *ud[NFTNL_UDATA_SET_MAX + 1] = {};
        enum byteorder keybyteorder = BYTEORDER_INVALID;
        enum byteorder databyteorder = BYTEORDER_INVALID;
-       const struct datatype *keytype, *datatype = NULL;
        struct expr *typeof_expr_key, *typeof_expr_data;
        struct setelem_parse_ctx set_parse_ctx;
+       const struct datatype *datatype = NULL;
+       const struct datatype *keytype = NULL;
+       const struct datatype *dtype2 = NULL;
+       const struct datatype *dtype = NULL;
        const char *udata, *comment = NULL;
        uint32_t flags, key, objtype = 0;
-       const struct datatype *dtype;
        uint32_t data_interval = 0;
        bool automerge = false;
        struct set *set;
@@ -987,8 +993,8 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
                        netlink_io_error(ctx, NULL,
                                         "Unknown data type in set key %u",
                                         data);
-                       datatype_free(keytype);
-                       return NULL;
+                       set = NULL;
+                       goto out;
                }
        }
 
@@ -1026,19 +1032,18 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
        if (datatype) {
                uint32_t dlen;
 
-               dtype = set_datatype_alloc(datatype, databyteorder);
+               dtype2 = set_datatype_alloc(datatype, databyteorder);
                klen = nftnl_set_get_u32(nls, NFTNL_SET_DATA_LEN) * BITS_PER_BYTE;
 
                dlen = data_interval ?  klen / 2 : klen;
 
                if (set_udata_key_valid(typeof_expr_data, dlen)) {
                        typeof_expr_data->len = klen;
-                       datatype_free(datatype_get(dtype));
                        set->data = typeof_expr_data;
                } else {
                        expr_free(typeof_expr_data);
                        set->data = constant_expr_alloc(&netlink_location,
-                                                       dtype,
+                                                       dtype2,
                                                        databyteorder, klen,
                                                        NULL);
 
@@ -1049,16 +1054,12 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
 
                if (data_interval)
                        set->data->flags |= EXPR_F_INTERVAL;
-
-               if (dtype != datatype)
-                       datatype_free(datatype);
        }
 
        dtype = set_datatype_alloc(keytype, keybyteorder);
        klen = nftnl_set_get_u32(nls, NFTNL_SET_KEY_LEN) * BITS_PER_BYTE;
 
        if (set_udata_key_valid(typeof_expr_key, klen)) {
-               datatype_free(datatype_get(dtype));
                set->key = typeof_expr_key;
                set->key_typeof_valid = true;
        } else {
@@ -1068,9 +1069,6 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
                                               NULL);
        }
 
-       if (dtype != keytype)
-               datatype_free(keytype);
-
        set->flags   = nftnl_set_get_u32(nls, NFTNL_SET_FLAGS);
        set->handle.handle.id = nftnl_set_get_u64(nls, NFTNL_SET_HANDLE);
 
@@ -1098,6 +1096,11 @@ struct set *netlink_delinearize_set(struct netlink_ctx *ctx,
                }
        }
 
+out:
+       datatype_free(datatype);
+       datatype_free(keytype);
+       datatype_free(dtype2);
+       datatype_free(dtype);
        return set;
 }
 

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index efb3fe6a27da41c476707c949a91aa95983a0b19..2695fa9349c4e44d40233d30b612312b5646221d 100644 (file)
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -2576,7 +2576,7 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
                }
                ctx->flags &= ~RULE_PP_IN_CONCATENATION;
                list_splice(&tmp, &expr->expressions);
-               datatype_set(expr, concat_type_alloc(ntype));
+               __datatype_set(expr, concat_type_alloc(ntype));
                break;
        }
        case EXPR_UNARY:

diff --git a/src/payload.c b/src/payload.c
index 101bfbda587895b681552213e3ad1f159fdece0c..115c3dcc5be6a6f8733db18bd4cf20b4900230be 100644 (file)
--- a/src/payload.c
+++ b/src/payload.c
@@ -241,8 +241,7 @@ static struct expr *payload_expr_parse_udata(const struct nftnl_udata *attr)
                dtype = dtype_clone(&xinteger_type);
                dtype->size = len;
                dtype->byteorder = BYTEORDER_BIG_ENDIAN;
-               dtype->refcnt = 1;
-               expr->dtype = dtype;
+               __datatype_set(expr, dtype);
        }
 
        return expr;