Re: [PATCH 2/2,libnftnl] Check memory allocations in setters

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

On Thu, Jun 02, 2016 at 12:40:24PM +0200, Carlos Falgueras García wrote:
> When you set an object attribute the memory is copied, sometimes an
> allocations is needed and it must be checked. By now all setters methods
> returns void, so the policy adopted in case of error is keep the object
> unchanged.
> 
> What this patch makes:
> 	* All memory allocations inside setters are checked
> 	* The object remains unchanged in case of error
> 	* Unsetters are used if is possible in order to consolidate
> 
> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> ---
>  src/chain.c          | 26 +++++++++++++++++---------
>  src/expr/dynset.c    |  8 +++++++-
>  src/expr/immediate.c |  9 ++++++---
>  src/expr/log.c       | 12 +++++++++---
>  src/expr/lookup.c    |  8 +++++++-
>  src/rule.c           | 28 +++++++++++++++++-----------
>  src/set.c            | 18 ++++++++++++------
>  src/set_elem.c       | 21 +++++++++++++--------
>  8 files changed, 88 insertions(+), 42 deletions(-)
> 
> diff --git a/src/chain.c b/src/chain.c
> index 990c576..c4c4ff7 100644
> --- a/src/chain.c
> +++ b/src/chain.c
> @@ -168,6 +168,8 @@ static uint32_t nftnl_chain_validate[NFTNL_CHAIN_MAX + 1] = {
>  void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
>  			     const void *data, uint32_t data_len)
>  {
> +	char *newstr;
> +
>  	if (attr > NFTNL_CHAIN_MAX)
>  		return;
>  
> @@ -178,10 +180,12 @@ void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
>  		strncpy(c->name, data, NFT_CHAIN_MAXNAMELEN);
>  		break;
>  	case NFTNL_CHAIN_TABLE:
> -		if (c->table)
> -			xfree(c->table);
> +		newstr = strdup(data);
> +		if (!newstr)
> +			return;
>  
> -		c->table = strdup(data);
> +		nftnl_chain_unset(c, attr);
> +		c->table = newstr;

This looks a bit tangled. Probably something more simple, like this
below?

                xfree(c->table);
                c->table = strdup(data);
                if (!c->table)
                        return;

Another comment below.

[...]
> diff --git a/src/expr/log.c b/src/expr/log.c
> index c3dc0a6..369174f 100644
> --- a/src/expr/log.c
> +++ b/src/expr/log.c
> @@ -34,13 +34,17 @@ static int nftnl_expr_log_set(struct nftnl_expr *e, uint16_t type,
>  				 const void *data, uint32_t data_len)
>  {
>  	struct nftnl_expr_log *log = nftnl_expr_data(e);
> +	char *newstr;
>  
>  	switch(type) {
>  	case NFTNL_EXPR_LOG_PREFIX:
> -		if (log->prefix)
> -			xfree(log->prefix);
> +		newstr = strdup(data);
> +		if (!newstr)
> +			return -1;
>  
> -		log->prefix = strdup(data);
> +		if (log->flags & (1 << NFTNL_EXPR_LOG_PREFIX))
> +			xfree(log->prefix);
> +		log->prefix = newstr;
>  		break;
>  	case NFTNL_EXPR_LOG_GROUP:
>  		log->group = *((uint16_t *)data);
> @@ -60,6 +64,8 @@ static int nftnl_expr_log_set(struct nftnl_expr *e, uint16_t type,
>  	default:
>  		return -1;
>  	}
> +
> +	log->flags |= (1 << type);

Do we need this here?

>  	return 0;
>  }
>  
--
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