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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.