[PATCH] libnftnl: tests: Free nftnl_udata_buf before exit

[PATCH] libnftnl: tests: Free nftnl_udata_buf before exit

[Carlos Falgueras García]

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 tests/nft-rule-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/nft-rule-test.c b/tests/nft-rule-test.c
index 2f6e35f..dee3530 100644
--- a/tests/nft-rule-test.c
+++ b/tests/nft-rule-test.c
@@ -88,6 +88,7 @@ int main(int argc, char *argv[])
 	nftnl_rule_set_data(a, NFTNL_RULE_USERDATA,
 			    nftnl_udata_buf_data(udata),
 			    nftnl_udata_buf_len(udata));
+	nftnl_udata_buf_free(udata);
 
 	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234);
 	nftnl_rule_nlmsg_build_payload(nlh, a);
-- 
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 1/2] libnfntl: Fix segfault due to invalid free of rule user data

[Carlos Falgueras García]

If the user allocates a nftnl_udata_buf and then passes the TLV data to
nftnl_rule_set_data, the pointer stored in rule.user.data is not the begining of
the allocated block. In this situation, if it calls to nftnl_rule_free, it tries
to free this pointer and segfault is thrown.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 src/rule.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/rule.c b/src/rule.c
index c299548..3f276f8 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -167,7 +167,12 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 		if (r->user.data != NULL)
 			xfree(r->user.data);
 
-		r->user.data = (void *)data;
+		r->user.data = malloc(data_len);
+		if (!r->user.data) {
+			perror("libnftnl: " __FILE__ ": nftnl_rule_set_data()");
+			return;
+		}
+		memcpy(r->user.data, data, data_len);
 		r->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] libnfntl: Fix segfault due to invalid free of rule user data

[Pablo Neira Ayuso]

On Tue, May 17, 2016 at 06:00:15PM +0200, Carlos Falgueras García wrote:
> If the user allocates a nftnl_udata_buf and then passes the TLV data to
> nftnl_rule_set_data, the pointer stored in rule.user.data is not the begining of
> the allocated block. In this situation, if it calls to nftnl_rule_free, it tries
> to free this pointer and segfault is thrown.
> 
> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> ---
>  src/rule.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/rule.c b/src/rule.c
> index c299548..3f276f8 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -167,7 +167,12 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
>  		if (r->user.data != NULL)
>  			xfree(r->user.data);
>  
> -		r->user.data = (void *)data;
> +		r->user.data = malloc(data_len);
> +		if (!r->user.data) {
> +			perror("libnftnl: " __FILE__ ": nftnl_rule_set_data()");

We should spot this error messages from the library. The only
exception is when netlink ABI gets broken. So I'm removing this line.

We should add a new version of these setters at some point so we can
return an error (instead of void), so the client may check if the
memory allocation has failed, later.
--
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] nftables: Fix memory leak linearizing user data

[Carlos Falgueras García]

nftnl_rule_set_data makes a copy of the user data which receives, it is not
necessary make a copy before call it.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 src/netlink_linearize.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 62bb25c..98c22d8 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -1127,8 +1127,6 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
 
 	if (rule->comment) {
 		struct nftnl_udata_buf *udata;
-		uint32_t udlen;
-		void *ud;
 
 		udata = nftnl_udata_buf_alloc(NFT_USERDATA_MAXLEN);
 		if (!udata)
@@ -1137,12 +1135,9 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
 		if (!nftnl_udata_put_strz(udata, UDATA_TYPE_COMMENT,
 					  rule->comment))
 			memory_allocation_error();
-
-		udlen = nftnl_udata_buf_len(udata);
-		ud = xmalloc(udlen);
-		memcpy(ud, nftnl_udata_buf_data(udata), udlen);
-
-		nftnl_rule_set_data(nlr, NFTNL_RULE_USERDATA, ud, udlen);
+		nftnl_rule_set_data(nlr, NFTNL_RULE_USERDATA,
+				    nftnl_udata_buf_data(udata),
+				    nftnl_udata_buf_len(udata));
 
 		nftnl_udata_buf_free(udata);
 	}
-- 
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] nftables: Fix memory leak linearizing user data

[Pablo Neira Ayuso]

On Tue, May 17, 2016 at 06:00:16PM +0200, Carlos Falgueras García wrote:
> nftnl_rule_set_data makes a copy of the user data which receives, it is not
> necessary make a copy before call it.

Applied.

Please, don't include the name of the project you're targeting the
patch to in the subject, so instead of:

        [PATCH 2/2] nftables: Fix memory leak linearizing user data

this should be something like:

        [PATCH 2/2 nft] netlink_linearize: Fix memory leak in user data

For reference, have a look at what we have in the repository and
trying to make it look similar.

Anyway, I have reworded this subject. This leak is happening after the
change you made in the library, so yes there is now a leak, but we are
actually adapting this to the new interface before we release nftables
0.6.
--
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] libnftnl: tests: Free nftnl_udata_buf before exit

[Pablo Neira Ayuso]

Applied, thanks.