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

["Álvaro Neira Ayuso" <alvaroneay@gmail.com>]

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