Re: [nft PATCH 4/5] json: Fix memleaks in echo support

[Phil Sutter <phil@nwl.cc>]

Hi Pablo,

On Wed, Feb 27, 2019 at 11:29:20AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 26, 2019 at 10:13:41PM +0100, Phil Sutter wrote:
> > When extracting netlink message data for populating JSON objects with
> > handles, allocated nftnl objects were not freed. Though since freeing
> > these objects also frees retrieved string attributes, copy them using
> > strdupa() which takes care of memory deallocation upon function return.
> > This is ideal since these strings are used only to find the right JSON
> > object to insert the handle into.
> > 
> > Fixes: bb32d8db9a125 ("JSON: Add support for echo option")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/parser_json.c | 28 ++++++++++++++++++----------
> >  1 file changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/parser_json.c b/src/parser_json.c
> > index 6755d39c34f0a..c92113ba516c2 100644
> > --- a/src/parser_json.c
> > +++ b/src/parser_json.c
> > @@ -1,3 +1,4 @@
> > +#define _GNU_SOURCE
> >  #include <errno.h>
> >  #include <stdint.h> /* needed by gmputil.h */
> >  #include <string.h>
> > @@ -3485,8 +3486,9 @@ static int json_update_table(struct netlink_mon_handler *monh,
> >  
> >  	nlt = netlink_table_alloc(nlh);
> >  	family = family2str(nftnl_table_get_u32(nlt, NFTNL_TABLE_FAMILY));
> > -	name = nftnl_table_get_str(nlt, NFTNL_TABLE_NAME);
> > +	name = strdupa(nftnl_table_get_str(nlt, NFTNL_TABLE_NAME));
> 
> Hm, I still don't see why we need this strdupa() call...
> 
> >  	handle = nftnl_table_get_u64(nlt, NFTNL_TABLE_HANDLE);
> > +	nftnl_table_free(nlt);
> 
> The table name attribute is owned by the nlt object.
> 
> This is used by obj_info_matches() which doesn't take this?

Strictly it is not necessary, but the call to nftnl_table_free() would
have to be delayed till after the search. Since the function either
returns from the loop or after it, I would either duplicate the free
call or have to introduce some return value handling.

The original problem is that I first extract the table name using
nftnl_table_get_str(), then two lines later free the nftnl_table object
(which makes the extracted const char *name point at freed memory).

The semantics I follow in all those update functions is:

1) parse netlink message into nftnl object
2) extract required info from nftnl object
3) free nftnl object again (since it's not used anymore afterwards)
4) lookup JSON object with data from (2) and insert handle (also from
(2)).

Using strdupa() allows for this without the need for a final cleanup
step.

Cheers, Phil