El 11/02/15 a las 00:49, Pablo Neira Ayuso escribió:
On Tue, Feb 10, 2015 at 05:46:13PM +0100, Alvaro Neira Ayuso wrote:
The function nft_jansson_parse_rule receives the tree like parameter and we
release it inside the functions. We use this function in the ruleset import
support. Therefore, we release the child nodes in the tree that contains the
rule information before to release the tree. Then, if we import a ruleset,
valgrind shows:
==16551== Invalid write of size 8
==16551== at 0x5EC92AA: json_delete (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC4FC4: ??? (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC5058: ??? (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC9270: json_delete (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC92B4: json_delete (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC4FC4: ??? (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC5058: ??? (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x5EC9270: json_delete (in
/usr/lib/x86_64-linux-gnu/libjansson.so.4.3.1)
==16551== by 0x50455D0: nft_ruleset_json_parse (ruleset.c:557)
==16551== by 0x50458AE: nft_ruleset_do_parse (ruleset.c:696)
==16551== by 0x408AEC: do_command (rule.c:1317)
==16551== by 0x406B05: nft_run (main.c:194)
With this patch, we release the tree only in the function nft_rule_json_parse
(the caller function of nft_jansson_parse_rule).
Signed-off-by: Alvaro Neira Ayuso <alvaroneay@xxxxxxxxx>
---
src/rule.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/rule.c b/src/rule.c
index 7f4d049..028dc2e 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -597,10 +597,8 @@ int nft_jansson_parse_rule(struct nft_rule *r, json_t *tree,
nft_rule_add_expr(r, e);
}
- nft_jansson_free_root(tree);
return 0;
err:
- nft_jansson_free_root(tree);
return -1;
}
#endif
@@ -613,12 +611,16 @@ static int nft_rule_json_parse(struct nft_rule *r, const void *json,
#ifdef JSON_PARSING
json_t *tree;
json_error_t error;
+ int ret;
tree = nft_jansson_create_root(json, &error, err, input);
if (tree == NULL)
return -1;
- return nft_jansson_parse_rule(r, tree, err, set_list);
+ ret = nft_jansson_parse_rule(r, tree, err, set_list);
+
+ nft_jansson_free_root(tree);
I like this cleanup.
It's a good idea to release things from the same function where you
allocate for tracebility and readability reasons.
But I don't understand why this is fixing the valgrind spot that you
indicate above.
Sure, maybe the description is confused. The goal of this patch is a
cleanup because I release a tree that we receive from the parameter. And
this is not clear. It's better release the tree where we create it.
Moreover, I remember that the node json_t in Jansson has a reference
counter. Info from Jansson website
(https://jansson.readthedocs.org/en/2.2/apiref.html):
json_t
This data structure is used throughout the library to represent all JSON
values. It always contains the type of the JSON value it holds and the
value’s reference count.
I have to take a look the code of Jansson. But to release a tree, we
decrease this reference counter (using the function json_decref) and
later delete it.
Maybe I'm wrong, Pablo. But if we release the tree in the function
nft_jansson_parse_rule, we will say that this node are unused.
Therefore, when we decrease the reference counter of the tree in
nft_ruleset_parse, we try to decrease the reference counter for nodes
that is already decreased. And Valgrind shows that we're trying to do a
invalid write because we try to write in nodes that they are not available.
This patch removes this error.
In short, you're right. It's confused, add the Valgrind output in this
description. I'm going to rework the description and send you another
patch like a cleanup. Sorry for the noise.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
