some memory leak fixes

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

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)

---
 src/rule.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index f65674c..58d640e 100644
@@ -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

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

---
 src/parser_bison.y | 2 ++
 1 file changed, 2 insertions(+)

index a8448e1..c505a04 100644
@@ -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

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

---
 src/parser_bison.y | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index c505a04..b898e1c 100644
@@ -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

---
 src/parser_bison.y | 2 ++
 1 file changed, 2 insertions(+)

index b898e1c..85cf131 100644
@@ -756,6 +756,8 @@ line			:	common_block			{ $$ = NULL; }
 					} else
 						list_splice_tail(&list, &state->cmds);
 				}
+				if (state->nerrs)
+					YYABORT;
 				$$ = NULL;
 
 				YYACCEPT;

2.13.2

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;
||                                              ^~

---
 src/evaluate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ca8b63b..86eea19 100644
@@ -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

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

---
 src/erec.c    |  5 +++++
 src/scanner.l | 11 +++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

index eacdd97..439add9 100644
@@ -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;
 }
 

index 86a03f3..7d5437f 100644
@@ -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

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)

---
 src/cli.c | 4 ++++
 1 file changed, 4 insertions(+)

index 7cd2f45..9876d06 100644
@@ -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

Series applied, thanks Eric.

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.

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

Let me know, thanks!