summaryrefslogtreecommitdiffstats
path: root/include/datatype.h
diff options
context:
space:
mode:
authorThomas Haller <thaller@redhat.com>2023-09-12 09:30:54 +0200
committerPablo Neira Ayuso <pablo@netfilter.org>2023-09-14 14:45:14 +0200
commit8519ab031d8022999603a69ee9f18e8cfb06645d (patch)
treec6574e21984de79f9ff92e7b9360feb400c07239 /include/datatype.h
parent65d3e81ce8b98d0bde24f4d8c392c252981c7d8b (diff)
datatype: fix leak and cleanup reference counting for struct datatype
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>
Diffstat (limited to 'include/datatype.h')
-rw-r--r--include/datatype.h1
1 files changed, 1 insertions, 0 deletions
diff --git a/include/datatype.h b/include/datatype.h
index 6146eda1..52a2e943 100644
--- 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 *datatype_clone(const struct datatype *orig_dtype);