On Wed, Feb 27, 2019 at 12:15:26PM +0100, Phil Sutter wrote:
> 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@xxxxxx>
> > > ---
> > > 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.
OK, so the object is released and you just clone the fields that you
need around for the lookup, that's fine with me.
Not related to this patch: IIRC this echo support is not using the
nlmsg_seq to correlate the command and the result that we obtain,
right? Telling this because this should work with a batch that
contains several requests to create rules, then use this sequence
number to correlate things the reply with the original rule creation
command.
