Re: [libnftnl PATCH 2/7] ruleset: Prevent memleak in nftnl_ruleset_snprintf_*() functions

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

On Fri, Aug 12, 2016 at 02:44:58AM +0200, Phil Sutter wrote:
> On Fri, Aug 12, 2016 at 01:42:02AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Aug 12, 2016 at 01:33:34AM +0200, Phil Sutter wrote:
> > > From: Phil Sutter <psutter@redhat.com>
> > > 
> > > This is an ugly aspect of the SNPRINTF_BUFFER_SIZE() macro: it contains
> > > a return statement and if that triggers, the function returns without
> > > freeing the iterator object. Therefore duplicate the 'ret < 0' check
> > > before calling it, freeing the iterator knowing that we will bail out
> > > immediately afterwards anyway.
> > > 
> > > Cc: Arturo Borrero <arturo.borrero.glez@gmail.com>
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  src/ruleset.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/src/ruleset.c b/src/ruleset.c
> > > index 666bcc7a246b6..93cf95ab61e15 100644
> > > --- a/src/ruleset.c
> > > +++ b/src/ruleset.c
> > > @@ -888,12 +888,16 @@ nftnl_ruleset_snprintf_table(char *buf, size_t size,
> > >  	t = nftnl_table_list_iter_next(ti);
> > >  	while (t != NULL) {
> > >  		ret = nftnl_table_snprintf(buf+offset, len, t, type, flags);
> > > +		if (ret < 0)
> > > +			nftnl_table_list_iter_destroy(ti);
> > >  		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> > 
> > Better get rid of the obscure if (ret < 0) hidden in
> > SNPRINT_BUFFER_SIZE.
> > 
> > Or simply set:
> > 
> >         if (ret < 0)
> >                 ret = 0;
> > 
> > in SNPRINT_BUFFER_SIZE.
> 
> Hmm. This means we will lose error propagation. Given how widely this
> macro is being used (grep says 200 calls), this needs a good second
> thought.

The usual suggested idiom to deal with this looks like:

    snprintf(cp, blen, "AF=%d ", sau->soa.sa_family)
    blen -= strlen(cp);
    cp += strlen(cp);

See: https://goo.gl/AUGE5m

No need to second thought, we can just ignore the -1 case.