[PATCH 1/2,libnftnl] Free user data in unsetters

[PATCH 1/2,libnftnl] Free user data in unsetters

[Carlos Falgueras García]

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 src/rule.c     | 2 ++
 src/set_elem.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/rule.c b/src/rule.c
index 8ee8648..3576e32 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -112,6 +112,8 @@ void nftnl_rule_unset(struct nftnl_rule *r, uint16_t attr)
 	case NFTNL_RULE_POSITION:
 	case NFTNL_RULE_FAMILY:
 	case NFTNL_RULE_USERDATA:
+		xfree(r->user.data);
+		r->user.len = 0;
 		break;
 	}
 
diff --git a/src/set_elem.c b/src/set_elem.c
index b9c7e1e..47ad6f4 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -82,6 +82,8 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
 	case NFTNL_SET_ELEM_TIMEOUT:	/* NFTA_SET_ELEM_TIMEOUT */
 	case NFTNL_SET_ELEM_EXPIRATION:	/* NFTA_SET_ELEM_EXPIRATION */
 	case NFTNL_SET_ELEM_USERDATA:	/* NFTA_SET_ELEM_USERDATA */
+		xfree(s->user.data);
+		s->user.len = 0;
 		break;
 	case NFTNL_SET_ELEM_EXPR:
 		if (s->flags & (1 << NFTNL_SET_ELEM_EXPR)) {
-- 
2.8.2

--
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

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

[Carlos Falgueras García]

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;
 		break;
 	case NFTNL_CHAIN_HOOKNUM:
 		memcpy(&c->hooknum, data, sizeof(c->hooknum));
@@ -208,16 +212,20 @@ void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
 		c->family = *((uint32_t *)data);
 		break;
 	case NFTNL_CHAIN_TYPE:
-		if (c->type)
-			xfree(c->type);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		c->type = strdup(data);
+		nftnl_chain_unset(c, attr);
+		c->type = newstr;
 		break;
 	case NFTNL_CHAIN_DEV:
-		if (c->dev)
-			xfree(c->dev);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		c->dev = strdup(data);
+		nftnl_chain_unset(c, attr);
+		c->dev = newstr;
 		break;
 	}
 	c->flags |= (1 << attr);
diff --git a/src/expr/dynset.c b/src/expr/dynset.c
index c8d97a5..e54d9e9 100644
--- a/src/expr/dynset.c
+++ b/src/expr/dynset.c
@@ -37,6 +37,7 @@ nftnl_expr_dynset_set(struct nftnl_expr *e, uint16_t type,
 			 const void *data, uint32_t data_len)
 {
 	struct nftnl_expr_dynset *dynset = nftnl_expr_data(e);
+	char *newstr;
 
 	switch (type) {
 	case NFTNL_EXPR_DYNSET_SREG_KEY:
@@ -52,7 +53,12 @@ nftnl_expr_dynset_set(struct nftnl_expr *e, uint16_t type,
 		dynset->timeout = *((uint64_t *)data);
 		break;
 	case NFTNL_EXPR_DYNSET_SET_NAME:
-		dynset->set_name = strdup((const char *)data);
+		newstr = strdup(data);
+		if (!newstr)
+			return -1;
+
+		xfree(dynset->set_name);
+		dynset->set_name = newstr;
 		break;
 	case NFTNL_EXPR_DYNSET_SET_ID:
 		dynset->set_id = *((uint32_t *)data);
diff --git a/src/expr/immediate.c b/src/expr/immediate.c
index eb2ca0f..60e7ae4 100644
--- a/src/expr/immediate.c
+++ b/src/expr/immediate.c
@@ -30,6 +30,7 @@ nftnl_expr_immediate_set(struct nftnl_expr *e, uint16_t type,
 			    const void *data, uint32_t data_len)
 {
 	struct nftnl_expr_immediate *imm = nftnl_expr_data(e);
+	char *newstr;
 
 	switch(type) {
 	case NFTNL_EXPR_IMM_DREG:
@@ -43,10 +44,12 @@ nftnl_expr_immediate_set(struct nftnl_expr *e, uint16_t type,
 		imm->data.verdict = *((uint32_t *)data);
 		break;
 	case NFTNL_EXPR_IMM_CHAIN:
-		if (imm->data.chain)
-			xfree(imm->data.chain);
+		newstr = strdup(data);
+		if (!newstr)
+			return -1;
 
-		imm->data.chain = strdup(data);
+		xfree(imm->data.chain);
+		imm->data.chain = newstr;
 		break;
 	default:
 		return -1;
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);
 	return 0;
 }
 
diff --git a/src/expr/lookup.c b/src/expr/lookup.c
index ed32ba6..99d1d1b 100644
--- a/src/expr/lookup.c
+++ b/src/expr/lookup.c
@@ -33,6 +33,7 @@ nftnl_expr_lookup_set(struct nftnl_expr *e, uint16_t type,
 			  const void *data, uint32_t data_len)
 {
 	struct nftnl_expr_lookup *lookup = nftnl_expr_data(e);
+	char *newstr;
 
 	switch(type) {
 	case NFTNL_EXPR_LOOKUP_SREG:
@@ -42,7 +43,12 @@ nftnl_expr_lookup_set(struct nftnl_expr *e, uint16_t type,
 		lookup->dreg = *((uint32_t *)data);
 		break;
 	case NFTNL_EXPR_LOOKUP_SET:
-		lookup->set_name = strdup((const char *)data);
+		newstr = strdup(data);
+		if (!newstr)
+			return -1;
+
+		xfree(lookup->set_name);
+		lookup->set_name = newstr;
 		break;
 	case NFTNL_EXPR_LOOKUP_SET_ID:
 		lookup->set_id = *((uint32_t *)data);
diff --git a/src/rule.c b/src/rule.c
index 3576e32..223a92d 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -132,6 +132,9 @@ static uint32_t nftnl_rule_validate[NFTNL_RULE_MAX + 1] = {
 void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 			    const void *data, uint32_t data_len)
 {
+	char *newstr;
+	void *newud;
+
 	if (attr > NFTNL_RULE_MAX)
 		return;
 
@@ -139,16 +142,20 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 
 	switch(attr) {
 	case NFTNL_RULE_TABLE:
-		if (r->table)
-			xfree(r->table);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		r->table = strdup(data);
+		nftnl_rule_unset(r, attr);
+		r->table = newstr;
 		break;
 	case NFTNL_RULE_CHAIN:
-		if (r->chain)
-			xfree(r->chain);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		r->chain = strdup(data);
+		nftnl_rule_unset(r, attr);
+		r->chain = newstr;
 		break;
 	case NFTNL_RULE_HANDLE:
 		r->handle = *((uint64_t *)data);
@@ -166,13 +173,12 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 		r->position = *((uint64_t *)data);
 		break;
 	case NFTNL_RULE_USERDATA:
-		if (r->user.data != NULL)
-			xfree(r->user.data);
-
-		r->user.data = malloc(data_len);
-		if (!r->user.data)
+		newud = malloc(data_len);
+		if (!newud)
 			return;
 
+		nftnl_rule_unset(r, attr);
+		r->user.data = newud;
 		memcpy(r->user.data, data, data_len);
 		r->user.len = data_len;
 		break;
diff --git a/src/set.c b/src/set.c
index dbea93b..258f771 100644
--- a/src/set.c
+++ b/src/set.c
@@ -116,6 +116,8 @@ static uint32_t nftnl_set_validate[NFTNL_SET_MAX + 1] = {
 void nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
 			   uint32_t data_len)
 {
+	char *newstr;
+
 	if (attr > NFTNL_SET_MAX)
 		return;
 
@@ -123,16 +125,20 @@ void nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
 
 	switch(attr) {
 	case NFTNL_SET_TABLE:
-		if (s->table)
-			xfree(s->table);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		s->table = strdup(data);
+		nftnl_set_unset(s, attr);
+		s->table = newstr;
 		break;
 	case NFTNL_SET_NAME:
-		if (s->name)
-			xfree(s->name);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		s->name = strdup(data);
+		nftnl_set_unset(s, attr);
+		s->name = newstr;
 		break;
 	case NFTNL_SET_FLAGS:
 		s->set_flags = *((uint32_t *)data);
diff --git a/src/set_elem.c b/src/set_elem.c
index 47ad6f4..1b8d7e6 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -102,6 +102,9 @@ EXPORT_SYMBOL_ALIAS(nftnl_set_elem_unset, nft_set_elem_attr_unset);
 void nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
 			   const void *data, uint32_t data_len)
 {
+	char *newstr;
+	void *newud;
+
 	switch(attr) {
 	case NFTNL_SET_ELEM_FLAGS:
 		s->set_elem_flags = *((uint32_t *)data);
@@ -114,10 +117,12 @@ void nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
 		s->data.verdict = *((uint32_t *)data);
 		break;
 	case NFTNL_SET_ELEM_CHAIN:	/* NFTA_SET_ELEM_DATA */
-		if (s->data.chain)
-			xfree(s->data.chain);
+		newstr = strdup(data);
+		if (!newstr)
+			return;
 
-		s->data.chain = strdup(data);
+		nftnl_set_elem_unset(s, attr);
+		s->data.chain = newstr;
 		break;
 	case NFTNL_SET_ELEM_DATA:	/* NFTA_SET_ELEM_DATA */
 		memcpy(s->data.val, data, data_len);
@@ -127,12 +132,12 @@ void nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
 		s->timeout = *((uint64_t *)data);
 		break;
 	case NFTNL_SET_ELEM_USERDATA: /* NFTA_SET_ELEM_USERDATA */
-		if (s->user.data != NULL)
-			xfree(s->user.data);
-
-		s->user.data = malloc(data_len);
-		if (!s->user.data)
+		newud = malloc(data_len);
+		if (!newud)
 			return;
+
+		nftnl_set_elem_unset(s, attr);
+		s->user.data = newud;
 		memcpy(s->user.data, data, data_len);
 		s->user.len = data_len;
 		break;
-- 
2.8.2

--
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

Re: [PATCH 1/2,libnftnl] Free user data in unsetters

[Pablo Neira Ayuso]

On Thu, Jun 02, 2016 at 12:40:23PM +0200, Carlos Falgueras García wrote:
> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> ---
>  src/rule.c     | 2 ++
>  src/set_elem.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/src/rule.c b/src/rule.c
> index 8ee8648..3576e32 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -112,6 +112,8 @@ void nftnl_rule_unset(struct nftnl_rule *r, uint16_t attr)
>  	case NFTNL_RULE_POSITION:
>  	case NFTNL_RULE_FAMILY:
>  	case NFTNL_RULE_USERDATA:
> +		xfree(r->user.data);
> +		r->user.len = 0;

I think we don't need to reset user.len, right?

>  		break;
>  	}
>  
> diff --git a/src/set_elem.c b/src/set_elem.c
> index b9c7e1e..47ad6f4 100644
> --- a/src/set_elem.c
> +++ b/src/set_elem.c
> @@ -82,6 +82,8 @@ void nftnl_set_elem_unset(struct nftnl_set_elem *s, uint16_t attr)
>  	case NFTNL_SET_ELEM_TIMEOUT:	/* NFTA_SET_ELEM_TIMEOUT */
>  	case NFTNL_SET_ELEM_EXPIRATION:	/* NFTA_SET_ELEM_EXPIRATION */
>  	case NFTNL_SET_ELEM_USERDATA:	/* NFTA_SET_ELEM_USERDATA */
> +		xfree(s->user.data);
> +		s->user.len = 0;
>  		break;
>  	case NFTNL_SET_ELEM_EXPR:
>  		if (s->flags & (1 << NFTNL_SET_ELEM_EXPR)) {
> -- 
> 2.8.2
> 
--
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

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

[Pablo Neira Ayuso]

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

Re: [PATCH 1/2,libnftnl] Free user data in unsetters

[Pablo Neira Ayuso]

On Thu, Jun 02, 2016 at 12:55:38PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 02, 2016 at 12:40:23PM +0200, Carlos Falgueras García wrote:
> > Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> > ---
> >  src/rule.c     | 2 ++
> >  src/set_elem.c | 2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/src/rule.c b/src/rule.c
> > index 8ee8648..3576e32 100644
> > --- a/src/rule.c
> > +++ b/src/rule.c
> > @@ -112,6 +112,8 @@ void nftnl_rule_unset(struct nftnl_rule *r, uint16_t attr)
> >  	case NFTNL_RULE_POSITION:
> >  	case NFTNL_RULE_FAMILY:
> >  	case NFTNL_RULE_USERDATA:
> > +		xfree(r->user.data);
> > +		r->user.len = 0;
> 
> I think we don't need to reset user.len, right?

BTW, I'd suggest you call this: "Fix leak in nftnl_rule_unset()"

I would review other any other existing unsetter to see if we're also
leaking there. Thanks.
--
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