[PATCH v2 libnftnl] Check all strdup

[PATCH v2 libnftnl] Check all strdup

[Carlos Falgueras García]

Check all strdup possible error and treat it consequently.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 src/chain.c          | 12 ++++++++++++
 src/expr/data_reg.c  |  6 ++++++
 src/expr/dynset.c    |  4 ++++
 src/expr/immediate.c |  2 ++
 src/expr/log.c       |  4 ++++
 src/expr/lookup.c    |  4 ++++
 src/rule.c           |  8 ++++++++
 src/set.c            | 18 ++++++++++++++++--
 src/set_elem.c       | 14 +++++++++++++-
 9 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/src/chain.c b/src/chain.c
index 990c576..b20e007 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -182,6 +182,8 @@ void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
 			xfree(c->table);
 
 		c->table = strdup(data);
+		if (!c->table)
+			return;
 		break;
 	case NFTNL_CHAIN_HOOKNUM:
 		memcpy(&c->hooknum, data, sizeof(c->hooknum));
@@ -212,12 +214,16 @@ void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
 			xfree(c->type);
 
 		c->type = strdup(data);
+		if (!c->type)
+			return;
 		break;
 	case NFTNL_CHAIN_DEV:
 		if (c->dev)
 			xfree(c->dev);
 
 		c->dev = strdup(data);
+		if (!c->dev)
+			return;
 		break;
 	}
 	c->flags |= (1 << attr);
@@ -514,6 +520,8 @@ static int nftnl_chain_parse_hook(struct nlattr *attr, struct nftnl_chain *c)
 	}
 	if (tb[NFTA_HOOK_DEV]) {
 		c->dev = strdup(mnl_attr_get_str(tb[NFTA_HOOK_DEV]));
+		if (!c->dev)
+			return -1;
 		c->flags |= (1 << NFTNL_CHAIN_DEV);
 	}
 
@@ -537,6 +545,8 @@ int nftnl_chain_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_chain *c)
 	if (tb[NFTA_CHAIN_TABLE]) {
 		xfree(c->table);
 		c->table = strdup(mnl_attr_get_str(tb[NFTA_CHAIN_TABLE]));
+		if (!c->table)
+			return -1;
 		c->flags |= (1 << NFTNL_CHAIN_TABLE);
 	}
 	if (tb[NFTA_CHAIN_HOOK]) {
@@ -564,6 +574,8 @@ int nftnl_chain_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_chain *c)
 	if (tb[NFTA_CHAIN_TYPE]) {
 		xfree(c->type);
 		c->type = strdup(mnl_attr_get_str(tb[NFTA_CHAIN_TYPE]));
+		if (!c->type)
+			return -1;
 		c->flags |= (1 << NFTNL_CHAIN_TYPE);
 	}
 
diff --git a/src/expr/data_reg.c b/src/expr/data_reg.c
index 2a23285..f4e7125 100644
--- a/src/expr/data_reg.c
+++ b/src/expr/data_reg.c
@@ -51,6 +51,8 @@ static int nftnl_data_reg_verdict_json_parse(union nftnl_data_reg *reg, json_t *
 			return DATA_NONE;
 
 		reg->chain = strdup(chain);
+		if (!reg->chain)
+			return -1;
 	}
 
 	return DATA_VERDICT;
@@ -126,6 +128,8 @@ static int nftnl_data_reg_verdict_xml_parse(union nftnl_data_reg *reg,
 			xfree(reg->chain);
 
 		reg->chain = strdup(chain);
+		if (!reg->chain)
+			return -1;
 	}
 
 	return DATA_VERDICT;
@@ -455,6 +459,8 @@ nftnl_parse_verdict(union nftnl_data_reg *data, const struct nlattr *attr, int *
 			return -1;
 
 		data->chain = strdup(mnl_attr_get_str(tb[NFTA_VERDICT_CHAIN]));
+		if (!data->chain)
+			return -1;
 		if (type)
 			*type = DATA_CHAIN;
 		break;
diff --git a/src/expr/dynset.c b/src/expr/dynset.c
index c8d97a5..0404359 100644
--- a/src/expr/dynset.c
+++ b/src/expr/dynset.c
@@ -53,6 +53,8 @@ nftnl_expr_dynset_set(struct nftnl_expr *e, uint16_t type,
 		break;
 	case NFTNL_EXPR_DYNSET_SET_NAME:
 		dynset->set_name = strdup((const char *)data);
+		if (!dynset->set_name)
+			return -1;
 		break;
 	case NFTNL_EXPR_DYNSET_SET_ID:
 		dynset->set_id = *((uint32_t *)data);
@@ -183,6 +185,8 @@ nftnl_expr_dynset_parse(struct nftnl_expr *e, struct nlattr *attr)
 	if (tb[NFTA_DYNSET_SET_NAME]) {
 		dynset->set_name =
 			strdup(mnl_attr_get_str(tb[NFTA_DYNSET_SET_NAME]));
+		if (!dynset->set_name)
+			return -1;
 		e->flags |= (1 << NFTNL_EXPR_DYNSET_SET_NAME);
 	}
 	if (tb[NFTA_DYNSET_SET_ID]) {
diff --git a/src/expr/immediate.c b/src/expr/immediate.c
index eb2ca0f..243f0e0 100644
--- a/src/expr/immediate.c
+++ b/src/expr/immediate.c
@@ -47,6 +47,8 @@ nftnl_expr_immediate_set(struct nftnl_expr *e, uint16_t type,
 			xfree(imm->data.chain);
 
 		imm->data.chain = strdup(data);
+		if (!imm->data.chain)
+			return -1;
 		break;
 	default:
 		return -1;
diff --git a/src/expr/log.c b/src/expr/log.c
index c3dc0a6..5b774a4 100644
--- a/src/expr/log.c
+++ b/src/expr/log.c
@@ -41,6 +41,8 @@ static int nftnl_expr_log_set(struct nftnl_expr *e, uint16_t type,
 			xfree(log->prefix);
 
 		log->prefix = strdup(data);
+		if (!log->prefix)
+			return -1;
 		break;
 	case NFTNL_EXPR_LOG_GROUP:
 		log->group = *((uint16_t *)data);
@@ -155,6 +157,8 @@ nftnl_expr_log_parse(struct nftnl_expr *e, struct nlattr *attr)
 			xfree(log->prefix);
 
 		log->prefix = strdup(mnl_attr_get_str(tb[NFTA_LOG_PREFIX]));
+		if (!log->prefix)
+			return -1;
 		e->flags |= (1 << NFTNL_EXPR_LOG_PREFIX);
 	}
 	if (tb[NFTA_LOG_GROUP]) {
diff --git a/src/expr/lookup.c b/src/expr/lookup.c
index ed32ba6..727c287 100644
--- a/src/expr/lookup.c
+++ b/src/expr/lookup.c
@@ -43,6 +43,8 @@ nftnl_expr_lookup_set(struct nftnl_expr *e, uint16_t type,
 		break;
 	case NFTNL_EXPR_LOOKUP_SET:
 		lookup->set_name = strdup((const char *)data);
+		if (!lookup->set_name)
+			return -1;
 		break;
 	case NFTNL_EXPR_LOOKUP_SET_ID:
 		lookup->set_id = *((uint32_t *)data);
@@ -137,6 +139,8 @@ nftnl_expr_lookup_parse(struct nftnl_expr *e, struct nlattr *attr)
 	if (tb[NFTA_LOOKUP_SET]) {
 		lookup->set_name =
 			strdup(mnl_attr_get_str(tb[NFTA_LOOKUP_SET]));
+		if (!lookup->set_name)
+			return -1;
 		e->flags |= (1 << NFTNL_EXPR_LOOKUP_SET);
 	}
 	if (tb[NFTA_LOOKUP_SET_ID]) {
diff --git a/src/rule.c b/src/rule.c
index 8ee8648..c5cf415 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -141,12 +141,16 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 			xfree(r->table);
 
 		r->table = strdup(data);
+		if (!r->table)
+			return;
 		break;
 	case NFTNL_RULE_CHAIN:
 		if (r->chain)
 			xfree(r->chain);
 
 		r->chain = strdup(data);
+		if (!r->chain)
+			return;
 		break;
 	case NFTNL_RULE_HANDLE:
 		r->handle = *((uint64_t *)data);
@@ -436,11 +440,15 @@ int nftnl_rule_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_rule *r)
 	if (tb[NFTA_RULE_TABLE]) {
 		xfree(r->table);
 		r->table = strdup(mnl_attr_get_str(tb[NFTA_RULE_TABLE]));
+		if (!r->table)
+			return -1;
 		r->flags |= (1 << NFTNL_RULE_TABLE);
 	}
 	if (tb[NFTA_RULE_CHAIN]) {
 		xfree(r->chain);
 		r->chain = strdup(mnl_attr_get_str(tb[NFTA_RULE_CHAIN]));
+		if (!r->chain)
+			return -1;
 		r->flags |= (1 << NFTNL_RULE_CHAIN);
 	}
 	if (tb[NFTA_RULE_HANDLE]) {
diff --git a/src/set.c b/src/set.c
index dbea93b..eac44e1 100644
--- a/src/set.c
+++ b/src/set.c
@@ -127,12 +127,16 @@ void nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
 			xfree(s->table);
 
 		s->table = strdup(data);
+		if (!s->table)
+			return;
 		break;
 	case NFTNL_SET_NAME:
 		if (s->name)
 			xfree(s->name);
 
 		s->name = strdup(data);
+		if (!s->name)
+			return;
 		break;
 	case NFTNL_SET_FLAGS:
 		s->set_flags = *((uint32_t *)data);
@@ -291,10 +295,16 @@ struct nftnl_set *nftnl_set_clone(const struct nftnl_set *set)
 
 	memcpy(newset, set, sizeof(*set));
 
-	if (set->flags & (1 << NFTNL_SET_TABLE))
+	if (set->flags & (1 << NFTNL_SET_TABLE)) {
 		newset->table = strdup(set->table);
-	if (set->flags & (1 << NFTNL_SET_NAME))
+		if (!newset->table)
+			goto err;
+	}
+	if (set->flags & (1 << NFTNL_SET_NAME)) {
 		newset->name = strdup(set->name);
+		if (!newset->name)
+			goto err;
+	}
 
 	INIT_LIST_HEAD(&newset->element_list);
 	list_for_each_entry(elem, &set->element_list, head) {
@@ -437,11 +447,15 @@ int nftnl_set_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_set *s)
 	if (tb[NFTA_SET_TABLE]) {
 		xfree(s->table);
 		s->table = strdup(mnl_attr_get_str(tb[NFTA_SET_TABLE]));
+		if (!s->table)
+			return -1;
 		s->flags |= (1 << NFTNL_SET_TABLE);
 	}
 	if (tb[NFTA_SET_NAME]) {
 		xfree(s->name);
 		s->name = strdup(mnl_attr_get_str(tb[NFTA_SET_NAME]));
+		if (!s->name)
+			return -1;
 		s->flags |= (1 << NFTNL_SET_NAME);
 	}
 	if (tb[NFTA_SET_FLAGS]) {
diff --git a/src/set_elem.c b/src/set_elem.c
index b9c7e1e..34c0e9b 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -116,6 +116,8 @@ void nftnl_set_elem_set(struct nftnl_set_elem *s, uint16_t attr,
 			xfree(s->data.chain);
 
 		s->data.chain = strdup(data);
+		if (!s->data.chain)
+			return;
 		break;
 	case NFTNL_SET_ELEM_DATA:	/* NFTA_SET_ELEM_DATA */
 		memcpy(s->data.val, data, data_len);
@@ -225,10 +227,16 @@ struct nftnl_set_elem *nftnl_set_elem_clone(struct nftnl_set_elem *elem)
 
 	memcpy(newelem, elem, sizeof(*elem));
 
-	if (elem->flags & (1 << NFTNL_SET_ELEM_CHAIN))
+	if (elem->flags & (1 << NFTNL_SET_ELEM_CHAIN)) {
 		newelem->data.chain = strdup(elem->data.chain);
+		if (!newelem->data.chain)
+			goto err;
+	}
 
 	return newelem;
+err:
+	nftnl_set_elem_free(newelem);
+	return NULL;
 }
 
 void nftnl_set_elem_nlmsg_build_payload(struct nlmsghdr *nlh,
@@ -474,12 +482,16 @@ int nftnl_set_elems_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_set *s)
 		xfree(s->table);
 		s->table =
 			strdup(mnl_attr_get_str(tb[NFTA_SET_ELEM_LIST_TABLE]));
+		if (!s->table)
+			return -1;
 		s->flags |= (1 << NFTNL_SET_TABLE);
 	}
 	if (tb[NFTA_SET_ELEM_LIST_SET]) {
 		xfree(s->name);
 		s->name =
 			strdup(mnl_attr_get_str(tb[NFTA_SET_ELEM_LIST_SET]));
+		if (!s->name)
+			return -1;
 		s->flags |= (1 << NFTNL_SET_NAME);
 	}
 	if (tb[NFTA_SET_ELEM_LIST_SET_ID]) {
-- 
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 v2 libnftnl] Check all strdup

[Pablo Neira Ayuso]

Carlos,

On Tue, May 31, 2016 at 12:08:32PM +0200, Carlos Falgueras García wrote:
> Check all strdup possible error and treat it consequently.

Please, manually apply these two patches in your local working copy:

http://patchwork.ozlabs.org/patch/631659/
http://patchwork.ozlabs.org/patch/631660/

Then, continue with the patch that I'm attaching.

As you can see, the idea is to return an integer for _set_data() and
_set_str(), so the caller can check if the internal string allocation
that the library performs has failed.

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

Re: [PATCH v2 libnftnl] Check all strdup

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

On Tue, Jun 07, 2016 at 05:08:10PM +0200, Pablo Neira Ayuso wrote:
> Carlos,
> 
> On Tue, May 31, 2016 at 12:08:32PM +0200, Carlos Falgueras García wrote:
> > Check all strdup possible error and treat it consequently.
> 
> Please, manually apply these two patches in your local working copy:
> 
> http://patchwork.ozlabs.org/patch/631659/
> http://patchwork.ozlabs.org/patch/631660/
> 
> Then, continue with the patch that I'm attaching.
> 
> As you can see, the idea is to return an integer for _set_data() and
> _set_str(), so the caller can check if the internal string allocation
> that the library performs has failed.

Forgot attachment, this is what I'm requesting you to continue.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 2969 bytes --]

diff --git a/include/libnftnl/chain.h b/include/libnftnl/chain.h
index 954b39f..ed21e48 100644
--- a/include/libnftnl/chain.h
+++ b/include/libnftnl/chain.h
@@ -37,13 +37,13 @@ enum nftnl_chain_attr {
 bool nftnl_chain_is_set(const struct nftnl_chain *c, uint16_t attr);
 void nftnl_chain_unset(struct nftnl_chain *c, uint16_t attr);
 void nftnl_chain_set(struct nftnl_chain *t, uint16_t attr, const void *data);
-void nftnl_chain_set_data(struct nftnl_chain *t, uint16_t attr,
+int nftnl_chain_set_data(struct nftnl_chain *t, uint16_t attr,
 			     const void *data, uint32_t data_len);
 void nftnl_chain_set_u8(struct nftnl_chain *t, uint16_t attr, uint8_t data);
 void nftnl_chain_set_u32(struct nftnl_chain *t, uint16_t attr, uint32_t data);
 void nftnl_chain_set_s32(struct nftnl_chain *t, uint16_t attr, int32_t data);
 void nftnl_chain_set_u64(struct nftnl_chain *t, uint16_t attr, uint64_t data);
-void nftnl_chain_set_str(struct nftnl_chain *t, uint16_t attr, const char *str);
+int nftnl_chain_set_str(struct nftnl_chain *t, uint16_t attr, const char *str);
 
 const void *nftnl_chain_get(const struct nftnl_chain *c, uint16_t attr);
 const void *nftnl_chain_get_data(const struct nftnl_chain *c, uint16_t attr,
diff --git a/src/chain.c b/src/chain.c
index 70daaf3..75ab840 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -165,11 +165,13 @@ static uint32_t nftnl_chain_validate[NFTNL_CHAIN_MAX + 1] = {
 	[NFTNL_CHAIN_FAMILY]		= sizeof(uint32_t),
 };
 
-void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
-			     const void *data, uint32_t data_len)
+int nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
+			 const void *data, uint32_t data_len)
 {
-	if (attr > NFTNL_CHAIN_MAX)
-		return;
+	if (attr > NFTNL_CHAIN_MAX) {
+		errno = -EOPNOTSUPP;
+		return -1;
+	}
 
 	nftnl_assert_validate(data, nftnl_chain_validate, attr, data_len);
 
@@ -182,6 +184,8 @@ void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
 			xfree(c->table);
 
 		c->table = strdup(data);
+		if (!c->table)
+			return -1;
 		break;
 	case NFTNL_CHAIN_HOOKNUM:
 		memcpy(&c->hooknum, data, sizeof(c->hooknum));
@@ -212,15 +216,20 @@ void nftnl_chain_set_data(struct nftnl_chain *c, uint16_t attr,
 			xfree(c->type);
 
 		c->type = strdup(data);
+		if (!c->type)
+			return -1;
 		break;
 	case NFTNL_CHAIN_DEV:
 		if (c->dev)
 			xfree(c->dev);
 
 		c->dev = strdup(data);
+		if (!c->type)
+			return -1;
 		break;
 	}
 	c->flags |= (1 << attr);
+	return 0;
 }
 EXPORT_SYMBOL(nftnl_chain_set_data);
 
@@ -254,9 +263,9 @@ void nftnl_chain_set_u8(struct nftnl_chain *c, uint16_t attr, uint8_t data)
 }
 EXPORT_SYMBOL(nftnl_chain_set_u8);
 
-void nftnl_chain_set_str(struct nftnl_chain *c, uint16_t attr, const char *str)
+int nftnl_chain_set_str(struct nftnl_chain *c, uint16_t attr, const char *str)
 {
-	nftnl_chain_set_data(c, attr, str, strlen(str));
+	return nftnl_chain_set_data(c, attr, str, strlen(str));
 }
 EXPORT_SYMBOL(nftnl_chain_set_str);
 

Re: [PATCH v2 libnftnl] Check all strdup

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> +	if (attr > NFTNL_CHAIN_MAX) {
> +		errno = -EOPNOTSUPP;

The negation should be dropped.

Re: [PATCH v2 libnftnl] Check all strdup

[Pablo Neira Ayuso]

On Wed, Jun 08, 2016 at 01:37:41PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +	if (attr > NFTNL_CHAIN_MAX) {
> > +		errno = -EOPNOTSUPP;
> 
> The negation should be dropped.

Right, this should be:

                errno = EOPNOTSUPP;