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>

1 files changed, 17 insertions, 14 deletions

diff --git a/src/netlink.c b/src/netlink.c
index 4cf1a98d..a3407121 100644
--- 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;
 }