[nftables 0/3] misc fixes

[nftables 0/3] misc fixes

[Eric Leblond]

Hello,

Here's a small patchset fixing some issues in nftables. The erec
patches are relatively significative (relatively as only applying
to debug mode) and the last one is just a small cleaning proposal.

BR,
--
Eric Leblond

[nftables 1/3] erec: fix buffer overflow

[Eric Leblond]

A static array was used to read data and to write information in
it without checking the limit of the array. The result was a buffer
overflow when the line was longer than 1024.

This patch now uses a allocated buffer to avoid the problem.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/erec.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/erec.c b/src/erec.c
index 810e9bf..75a3f74 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -12,6 +12,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdarg.h>
+#include <stdlib.h>
 
 #include <netlink.h>
 #include <gmputil.h>
@@ -82,6 +83,7 @@ void erec_print(FILE *f, const struct error_record *erec)
 	const struct input_descriptor *indesc = loc->indesc, *tmp;
 	const char *line = NULL; /* silence gcc */
 	char buf[1024];
+	char *pbuf = NULL;
 	unsigned int i, end;
 	int l, ret;
 
@@ -141,17 +143,24 @@ void erec_print(FILE *f, const struct error_record *erec)
 		if (indesc->type != INDESC_INTERNAL)
 			fprintf(f, "%s\n", line);
 
-		memset(buf, ' ', sizeof(buf));
 		end = 0;
 		for (l = erec->num_locations - 1; l >= 0; l--) {
 			loc = &erec->locations[l];
+			end = max(end, loc->last_column);
+		}
+		pbuf = malloc(end + 1);
+		if (pbuf == NULL)
+			return;
+		memset(pbuf, ' ', end + 1);
+		for (l = erec->num_locations - 1; l >= 0; l--) {
+			loc = &erec->locations[l];
 			for (i = loc->first_column ? loc->first_column - 1 : 0;
 			     i < loc->last_column; i++)
-				buf[i] = l ? '~' : '^';
-			end = max(end, loc->last_column);
+				pbuf[i] = l ? '~' : '^';
 		}
-		buf[end] = '\0';
-		fprintf(f, "%s", buf);
+		pbuf[end] = '\0';
+		fprintf(f, "%s", pbuf);
+		free(pbuf);
 	}
 	fprintf(f, "\n");
 }
-- 
2.1.4

Re: [nftables 1/3] erec: fix buffer overflow

[Pablo Neira Ayuso]

On Wed, Jun 24, 2015 at 09:51:49AM +0200, Eric Leblond wrote:
> A static array was used to read data and to write information in
> it without checking the limit of the array. The result was a buffer
> overflow when the line was longer than 1024.
> 
> This patch now uses a allocated buffer to avoid the problem.

Applied, thanks Eric.

I have replaced malloc and free by xmalloc and xfree respectively.

[nftables 2/3] erec: fix logic when reading from file

[Eric Leblond]

In case we are reading the rules from a file we need to reset the
file descriptor to the original position when calling erec_print.

This was not the case in previous code and was leading to valid
file to be seen as invalid when treated in debug mode.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/erec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/erec.c b/src/erec.c
index 75a3f74..b1456ee 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -86,6 +86,7 @@ void erec_print(FILE *f, const struct error_record *erec)
 	char *pbuf = NULL;
 	unsigned int i, end;
 	int l, ret;
+	int orig_offset = 0;
 
 	switch (indesc->type) {
 	case INDESC_BUFFER:
@@ -94,11 +95,13 @@ void erec_print(FILE *f, const struct error_record *erec)
 		break;
 	case INDESC_FILE:
 		memset(buf, 0, sizeof(buf));
+		orig_offset = lseek(indesc->fd, 0, SEEK_CUR);
 		lseek(indesc->fd, loc->line_offset, SEEK_SET);
 		ret = read(indesc->fd, buf, sizeof(buf) - 1);
 		if (ret > 0)
 			*strchrnul(buf, '\n') = '\0';
 		line = buf;
+		lseek(indesc->fd, orig_offset, SEEK_SET);
 		break;
 	case INDESC_INTERNAL:
 	case INDESC_NETLINK:
-- 
2.1.4

Re: [nftables 2/3] erec: fix logic when reading from file

[Pablo Neira Ayuso]

On Wed, Jun 24, 2015 at 09:51:50AM +0200, Eric Leblond wrote:
> In case we are reading the rules from a file we need to reset the
> file descriptor to the original position when calling erec_print.
> 
> This was not the case in previous code and was leading to valid
> file to be seen as invalid when treated in debug mode.

Also applied, thanks.

[nftables 3/3] mnl: improve select timeout logic

[Eric Leblond]

This patch explicitely set timeout for select in mnl_batch_talk.
Timeout used at the second call was the time spend on first select
and this was not looking correct. Instead this patch set a short
timeout as we are supposed to dequeue waiting kernel messages.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/mnl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/mnl.c b/src/mnl.c
index 76a9714..2da1074 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -246,7 +246,7 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
 	fd_set readfds;
 	struct timeval tv = {
-		.tv_sec		= 0,
+		.tv_sec		= 10,
 		.tv_usec	= 0
 	};
 
@@ -274,6 +274,8 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 		if (ret == -1)
 			mnl_err_list_node_add(err_list, errno, nlh->nlmsg_seq);
 
+		tv.tv_sec = 0;
+		tv.tv_usec = 1000;
 		ret = select(fd+1, &readfds, NULL, NULL, &tv);
 		if (ret == -1)
 			return -1;
-- 
2.1.4

Re: [nftables 3/3] mnl: improve select timeout logic

[Eric Leblond]

Hello,

Following live discussion with Pablo, please disregard this patch.
Sending a v2 in the following minutes.

BR,

On Wed, 2015-06-24 at 09:51 +0200, Eric Leblond wrote:
> This patch explicitely set timeout for select in mnl_batch_talk.
> Timeout used at the second call was the time spend on first select
> and this was not looking correct. Instead this patch set a short
> timeout as we are supposed to dequeue waiting kernel messages.
> 
> Signed-off-by: Eric Leblond <eric@regit.org>
> ---
>  src/mnl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mnl.c b/src/mnl.c
> index 76a9714..2da1074 100644
> --- a/src/mnl.c
> +++ b/src/mnl.c
> @@ -246,7 +246,7 @@ int mnl_batch_talk(struct mnl_socket *nl, struct 
> list_head *err_list)
>  	char rcv_buf[MNL_SOCKET_BUFFER_SIZE];
>  	fd_set readfds;
>  	struct timeval tv = {
> -		.tv_sec		= 0,
> +		.tv_sec		= 10,
>  		.tv_usec	= 0
>  	};
>  
> @@ -274,6 +274,8 @@ int mnl_batch_talk(struct mnl_socket *nl, struct 
> list_head *err_list)
>  		if (ret == -1)
>  			mnl_err_list_node_add(err_list, errno, nlh
> ->nlmsg_seq);
>  
> +		tv.tv_sec = 0;
> +		tv.tv_usec = 1000;
>  		ret = select(fd+1, &readfds, NULL, NULL, &tv);
>  		if (ret == -1)
>  			return -1;
-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/