[nft PATCH 0/7] some memory leak fixes

[nft PATCH 0/7] some memory leak fixes

[Eric Leblond]

Hi,

Here's a small patchset fixing some memory leaks in nftables. Most
of them have been found using ASAN.

There is still a problem in memory handling due to the max_errors
system that stack errors to avoid an exit on first error. The
consequence is that the bison parser is loosing track of its
internal stacks and can not call the destructors when there
is an error in the command.

If we do set max_errors to 1: 

 diff --git a/src/main.c b/src/main.c
 index 7fbf00a..183bd0e 100644
 --- a/src/main.c
 +++ b/src/main.c
 @@ -29,7 +29,7 @@
  #include <cli.h>
  
  static struct nft_ctx nft;
 -unsigned int max_errors = 10;
 +unsigned int max_errors = 1;
  #ifdef DEBUG
  unsigned int debug_level;
  #endif

Then there is no more memory leak in case of an invalid command
but we loose the display of multiple errors.

A possibleway to fix that would be to be able to set max_errors
via a configuration function. It would be set to 1 by default.
So users of libnftables will not experiment memleak but we
could keep the same behavior in nft by setting it to 10
explicetely.

BR,
--
Eric

[nft PATCH 1/7] src: fix memory leak when listing rules

[Eric Leblond]

When listing rules we were calling strdup on the table name
but variable was just used locally.

Found via `nft list ruleset` run with ASAN:

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0x45cca0 in __interceptor_strdup (/usr/local/sbin/nft+0x45cca0)
    #1 0x593c71 in xstrdup /home/eric/git/netfilter/nftables/src/utils.c:75:8
    #2 0x513b34 in do_list_ruleset /home/eric/git/netfilter/nftables/src/rule.c:1388:23
    #3 0x50e178 in do_command_list /home/eric/git/netfilter/nftables/src/rule.c:1500:10
    #4 0x50d3ea in do_command /home/eric/git/netfilter/nftables/src/rule.c:1696:10
    #5 0x5061ae in nft_netlink /home/eric/git/netfilter/nftables/src/main.c:207:9
    #6 0x505b87 in nft_run /home/eric/git/netfilter/nftables/src/main.c:255:8
    #7 0x50771f in main /home/eric/git/netfilter/nftables/src/main.c:392:6
    #8 0x7fa1f326d2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

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

diff --git a/src/rule.c b/src/rule.c
index f65674c..58d640e 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1385,12 +1385,14 @@ static int do_list_ruleset(struct netlink_ctx *ctx, struct cmd *cmd)
 			continue;
 
 		cmd->handle.family = table->handle.family;
-		cmd->handle.table = xstrdup(table->handle.table);
+		cmd->handle.table = table->handle.table;
 
 		if (do_list_table(ctx, cmd, table) < 0)
 			return -1;
 	}
 
+	cmd->handle.table = NULL;
+
 	return 0;
 }
 
-- 
2.13.2

[nft PATCH 2/7] parser: fix memory leak in set creation

[Eric Leblond]

sudo  ASAN_SYMBOLIZER_PATH=/usr/lib/llvm-3.9/bin/llvm-symbolizer  nft add set inet filter blacklisddddddddddddddddddddt {type inet_service \;}

=================================================================
==25152==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 13 byte(s) in 1 object(s) allocated from:
    #0 0x45cca0 in __interceptor_strdup (/usr/local/sbin/nft+0x45cca0)
    #1 0x593cb1 in xstrdup /home/eric/git/netfilter/nftables/src/utils.c:75:8
    #2 0x5bccb2 in nft_lex /home/eric/git/netfilter/nftables/src/scanner.l:566:22
    #3 0x5cb363 in nft_parse /home/eric/git/netfilter/nftables/src/parser_bison.c:4366:16
    #4 0x505a37 in nft_run /home/eric/git/netfilter/nftables/src/main.c:246:8
    #5 0x50771f in main /home/eric/git/netfilter/nftables/src/main.c:392:6
    #6 0x7ff7befdb2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

SUMMARY: AddressSanitizer: 13 byte(s) leaked in 1 allocation(s).
Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/parser_bison.y | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index a8448e1..c505a04 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1452,8 +1452,10 @@ type_identifier_list	:	type_identifier
 				if (dtype == NULL) {
 					erec_queue(error(&@1, "unknown datatype %s", $1),
 						   state->msgs);
+					xfree($1);
 					YYERROR;
 				}
+				xfree($1);
 				$$ = dtype->type;
 			}
 			|	type_identifier_list	DOT	type_identifier
-- 
2.13.2

[nft PATCH 3/7] parser: fix bison warnings

[Eric Leblond]

We had the following warnings

parser_bison.y:1089:10: warning: variable 'cmd' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
                                        if (erec != NULL) {
                                            ^~~~~~~~~~~~
parser_bison.y:1095:39: note: uninitialized use occurs here
                                (yyval.cmd) = cmd_alloc(CMD_LIST, cmd, &(yyvsp[0].handle), &(yyloc), NULL);
                                                                  ^~~
parser_bison.y:1089:6: note: remove the 'if' if its condition is always true
                                        if (erec != NULL) {
                                        ^~~~~~~~~~~~~~~~~~
parser_bison.y:1080:12: note: initialize the variable 'cmd' to silence this warning
                                int cmd;
                                       ^
                                        = 0

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

diff --git a/src/parser_bison.y b/src/parser_bison.y
index c505a04..b898e1c 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1089,7 +1089,8 @@ list_cmd		:	TABLE		table_spec
 					if (erec != NULL) {
 						erec_queue(erec, state->msgs);
 						YYERROR;
-					}
+					} else
+						YYERROR;
 				}
 
 				$$ = cmd_alloc(CMD_LIST, cmd, &$4, &@$, NULL);
-- 
2.13.2

[nft PATCH 4/7] parser: error if needed at EOF

[Eric Leblond]

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

diff --git a/src/parser_bison.y b/src/parser_bison.y
index b898e1c..85cf131 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -756,6 +756,8 @@ line			:	common_block			{ $$ = NULL; }
 					} else
 						list_splice_tail(&list, &state->cmds);
 				}
+				if (state->nerrs)
+					YYABORT;
 				$$ = NULL;
 
 				YYACCEPT;
-- 
2.13.2

[nft PATCH 5/7] evaluate: fix build with clang

[Eric Leblond]

Building with a recent clang was failing due to the following error:

src/evaluate.c|450 col 45| error: initializer element is not constant
||    static const unsigned int max_tcpoptlen = 15 * 4 * BITS_PER_BYTE - tcphdrlen;
||                                              ^~

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

diff --git a/src/evaluate.c b/src/evaluate.c
index ca8b63b..86eea19 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -446,8 +446,7 @@ static int __expr_evaluate_exthdr(struct eval_ctx *ctx, struct expr **exprp)
 
 	switch (expr->exthdr.op) {
 	case NFT_EXTHDR_OP_TCPOPT: {
-		static const uint8_t tcphdrlen = 20 * BITS_PER_BYTE;
-		static const unsigned int max_tcpoptlen = 15 * 4 * BITS_PER_BYTE - tcphdrlen;
+		static const unsigned int max_tcpoptlen = (15 * 4 - 20) * BITS_PER_BYTE;
 		unsigned int totlen = 0;
 
 		totlen += expr->exthdr.tmpl->offset;
-- 
2.13.2

[nft PATCH 6/7] scanner: free filename when destroying scanner

[Eric Leblond]

To be able to do so we duplicate the name in the indesc if it is
set.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/erec.c    |  5 +++++
 src/scanner.l | 11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/erec.c b/src/erec.c
index eacdd97..439add9 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -39,6 +39,8 @@ static void input_descriptor_destroy(const struct input_descriptor *indesc)
 	    indesc->location.indesc->type != INDESC_INTERNAL) {
 		input_descriptor_destroy(indesc->location.indesc);
 	}
+	if (indesc->name)
+		xfree(indesc->name);
 	xfree(indesc);
 }
 
@@ -53,6 +55,9 @@ static struct input_descriptor *input_descriptor_dup(const struct input_descript
 	    indesc->location.indesc->type != INDESC_INTERNAL)
 		dup_indesc->location.indesc = input_descriptor_dup(indesc->location.indesc);
 
+	if (indesc->name)
+		dup_indesc->name = xstrdup(indesc->name);
+
 	return dup_indesc;
 }
 
diff --git a/src/scanner.l b/src/scanner.l
index 86a03f3..7d5437f 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -835,6 +835,7 @@ void scanner_push_buffer(void *scanner, const struct input_descriptor *indesc,
 	state->indesc = &state->indescs[state->indesc_idx++];
 	memcpy(state->indesc, indesc, sizeof(*state->indesc));
 	state->indesc->data = buffer;
+	state->indesc->name = NULL;
 
 	b = yy_scan_string(buffer, scanner);
 	assert(b != NULL);
@@ -858,9 +859,15 @@ void scanner_destroy(struct parser_state *scanner)
 {
 	struct parser_state *state = yyget_extra(scanner);
 
-	/* Can't free indesc name - locations might still be in use */
-	while (state->indesc_idx--)
+	do {
+		struct input_descriptor *inpdesc =
+			&state->indescs[state->indesc_idx];
+		if (inpdesc && inpdesc->name) {
+			xfree(inpdesc->name);
+			inpdesc->name = NULL;
+		}
 		yypop_buffer_state(scanner);
+	} while (state->indesc_idx--);
 
 	yylex_destroy(scanner);
 }
-- 
2.13.2

[nft PATCH 7/7] cli: fix heap buffer overflow

[Eric Leblond]

This patch fixes an invalid read when an empty command was sent.

Found via nft running ASAN and entering an empty command:

nft>
=================================================================
==19540==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000008c6f at pc 0x55e3b561704d bp 0x7fffe9a33ac0 sp 0x7fffe9a33ab8
READ of size 1 at 0x602000008c6f thread T0
    #0 0x55e3b561704c in cli_append_multiline /home/eric/git/netfilter/nftables/src/cli.c:65
    #1 0x55e3b561725b in cli_complete /home/eric/git/netfilter/nftables/src/cli.c:109
    #2 0x7f6e0c2ccac2 in rl_callback_read_char (/lib/x86_64-linux-gnu/libreadline.so.7+0x2fac2)
    #3 0x55e3b5617ba6 in cli_init /home/eric/git/netfilter/nftables/src/cli.c:199
    #4 0x55e3b5573b75 in main /home/eric/git/netfilter/nftables/src/main.c:381
    #5 0x7f6e0bc9b2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    #6 0x55e3b55725a9 in _start (/usr/local/sbin/nft+0x445a9)

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

diff --git a/src/cli.c b/src/cli.c
index 7cd2f45..9876d06 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -58,6 +58,10 @@ static char *cli_append_multiline(char *line)
 	}
 
 	len = strlen(line);
+
+	if (len == 0)
+		return NULL;
+
 	if (line[len - 1] == '\\') {
 		line[len - 1] = '\0';
 		len--;
-- 
2.13.2

Re: [nft PATCH 0/7] some memory leak fixes

[Pablo Neira Ayuso]

On Tue, Jul 11, 2017 at 12:32:48AM +0200, Eric Leblond wrote:
> 
> Hi,
> 
> Here's a small patchset fixing some memory leaks in nftables. Most
> of them have been found using ASAN.

Series applied, thanks Eric.

> There is still a problem in memory handling due to the max_errors
> system that stack errors to avoid an exit on first error. The
> consequence is that the bison parser is loosing track of its
> internal stacks and can not call the destructors when there
> is an error in the command.

Probably we need explicit object tracking via list insertion, then
rewind and release them? Would that be possible? I would expect this
triggers a large patchset to do this right.

> If we do set max_errors to 1: 
> 
>  diff --git a/src/main.c b/src/main.c
>  index 7fbf00a..183bd0e 100644
>  --- a/src/main.c
>  +++ b/src/main.c
>  @@ -29,7 +29,7 @@
>   #include <cli.h>
>   
>   static struct nft_ctx nft;
>  -unsigned int max_errors = 10;
>  +unsigned int max_errors = 1;
>   #ifdef DEBUG
>   unsigned int debug_level;
>   #endif
> 
> Then there is no more memory leak in case of an invalid command
> but we loose the display of multiple errors.
> 
> A possibleway to fix that would be to be able to set max_errors
> via a configuration function. It would be set to 1 by default.
> So users of libnftables will not experiment memleak but we
> could keep the same behavior in nft by setting it to 10
> explicetely.

I would prefer we find a way to fix this without adding this
limitation.

Let me know, thanks!