[libnftnl PATCH] rule: don't release the tree parameter in the function nft_jansson_parse_rule

[libnftnl PATCH] rule: don't release the tree parameter in the function nft_jansson_parse_rule

[Alvaro Neira Ayuso]

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@gmail.com>
---
 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);
+	return ret;
 #else
 	errno = EOPNOTSUPP;
 	return -1;
-- 
1.7.10.4

[libnftnl PATCH] ruleset: fix a leak when we use the set lists

[Alvaro Neira Ayuso]

When we parse the sets in a ruleset, we set up a id. To do that, we use
a set list. We alloc this set list and we need to free this set list. And we
don't do that. If we import a ruleset, valgrind shows:

==18632== 285 (16 direct, 269 indirect) bytes in 1 blocks are definitely lost in
loss record 6 of 6
==18632==    at 0x4C272B8: calloc (vg_replace_malloc.c:566)
==18632==    by 0x5043822: nft_set_list_alloc (set.c:977)
==18632==    by 0x5045483: nft_ruleset_json_parse (ruleset.c:442)
==18632==    by 0x50458BE: nft_ruleset_do_parse (ruleset.c:696)
==18632==    by 0x408AEC: do_command (rule.c:1317)
==18632==    by 0x406B05: nft_run (main.c:194)
==18632==    by 0x40667C: main (main.c:360)

With this changes, we alloc the set list when we create the nft_parse_ctx and
free it later.

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
 src/ruleset.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/ruleset.c b/src/ruleset.c
index 15e84cf..3ce6ccc 100644
--- a/src/ruleset.c
+++ b/src/ruleset.c
@@ -439,10 +439,6 @@ static int nft_ruleset_json_parse_ruleset(struct nft_parse_ctx *ctx,
 	json_t *node, *array = ctx->json;
 	int len, i, ret;
 
-	ctx->set_list = nft_set_list_alloc();
-	if (ctx->set_list == NULL)
-		return -1;
-
 	len = json_array_size(array);
 	for (i = 0; i < len; i++) {
 		node = json_array_get(array, i);
@@ -525,6 +521,10 @@ static int nft_ruleset_json_parse(const void *json,
 	ctx.cb = cb;
 	ctx.format = type;
 
+	ctx.set_list = nft_set_list_alloc();
+	if (ctx.set_list == NULL)
+		return -1;
+
 	if (arg != NULL)
 		nft_ruleset_ctx_set(&ctx, NFT_RULESET_CTX_DATA, arg);
 
@@ -554,6 +554,7 @@ static int nft_ruleset_json_parse(const void *json,
 			goto err;
 	}
 
+	nft_set_list_free(ctx.set_list);
 	nft_jansson_free_root(root);
 	return 0;
 err:
@@ -573,10 +574,6 @@ static int nft_ruleset_xml_parse_ruleset(struct nft_parse_ctx *ctx,
 	mxml_node_t *node, *array = ctx->xml;
 	int len = 0, ret;
 
-	ctx->set_list = nft_set_list_alloc();
-	if (ctx->set_list == NULL)
-		return -1;
-
 	for (node = mxmlFindElement(array, array, NULL, NULL, NULL,
 				    MXML_DESCEND_FIRST);
 	     node != NULL;
@@ -653,6 +650,10 @@ static int nft_ruleset_xml_parse(const void *xml, struct nft_parse_err *err,
 	ctx.cb = cb;
 	ctx.format = type;
 
+	ctx.set_list = nft_set_list_alloc();
+	if (ctx.set_list == NULL)
+		return -1;
+
 	if (arg != NULL)
 		nft_ruleset_ctx_set(&ctx, NFT_RULESET_CTX_DATA, arg);
 
@@ -670,6 +671,7 @@ static int nft_ruleset_xml_parse(const void *xml, struct nft_parse_err *err,
 		nodecmd = mxmlWalkNext(tree, tree, MXML_NO_DESCEND);
 	}
 
+	nft_set_list_free(ctx.set_list);
 	mxmlDelete(tree);
 	return 0;
 err:
-- 
1.7.10.4

Re: [libnftnl PATCH] ruleset: fix a leak when we use the set lists

[Pablo Neira Ayuso]

On Tue, Feb 10, 2015 at 05:46:14PM +0100, Alvaro Neira Ayuso wrote:
> When we parse the sets in a ruleset, we set up a id. To do that, we use
> a set list. We alloc this set list and we need to free this set list. And we
> don't do that. If we import a ruleset, valgrind shows:

Please, remove this overelaborated description. The valgrind spot is
sufficiente.

More comments below:

> ==18632== 285 (16 direct, 269 indirect) bytes in 1 blocks are definitely lost in
> loss record 6 of 6
> ==18632==    at 0x4C272B8: calloc (vg_replace_malloc.c:566)
> ==18632==    by 0x5043822: nft_set_list_alloc (set.c:977)
> ==18632==    by 0x5045483: nft_ruleset_json_parse (ruleset.c:442)
> ==18632==    by 0x50458BE: nft_ruleset_do_parse (ruleset.c:696)
> ==18632==    by 0x408AEC: do_command (rule.c:1317)
> ==18632==    by 0x406B05: nft_run (main.c:194)
> ==18632==    by 0x40667C: main (main.c:360)
> 
> With this changes, we alloc the set list when we create the nft_parse_ctx and
> free it later.
> 
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
>  src/ruleset.c |   18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ruleset.c b/src/ruleset.c
> index 15e84cf..3ce6ccc 100644
> --- a/src/ruleset.c
> +++ b/src/ruleset.c
> @@ -439,10 +439,6 @@ static int nft_ruleset_json_parse_ruleset(struct nft_parse_ctx *ctx,
>  	json_t *node, *array = ctx->json;
>  	int len, i, ret;
>  
> -	ctx->set_list = nft_set_list_alloc();
> -	if (ctx->set_list == NULL)
> -		return -1;
> -
>  	len = json_array_size(array);
>  	for (i = 0; i < len; i++) {
>  		node = json_array_get(array, i);
> @@ -525,6 +521,10 @@ static int nft_ruleset_json_parse(const void *json,
>  	ctx.cb = cb;
>  	ctx.format = type;
>  
> +	ctx.set_list = nft_set_list_alloc();
> +	if (ctx.set_list == NULL)
> +		return -1;
> +
>  	if (arg != NULL)
>  		nft_ruleset_ctx_set(&ctx, NFT_RULESET_CTX_DATA, arg);

If you look in the ruleset.c file, now you add leaks in the error
path of nft_ruleset_json_parse() after this change.

Please, fix leaks all at once.

Re: [libnftnl PATCH] rule: don't release the tree parameter in the function nft_jansson_parse_rule

[Pablo Neira Ayuso]

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@gmail.com>
> ---
>  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.

Re: [libnftnl PATCH] rule: don't release the tree parameter in the function nft_jansson_parse_rule

[Álvaro Neira Ayuso]

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@gmail.com>
>> ---
>>   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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html