[nft PATCH 0/4] Memleak fixes and some minor cleanups

[nft PATCH 0/4] Memleak fixes and some minor cleanups

[Phil Sutter]

The following series fixes two sources of memleaks as reported by
valgrind (see patches 1 and 4). Patches 2 and 3 contain some minor
cleanups I found while searching for the leaks.

Phil Sutter (4):
  scanner: Fix for memleak due to unclosed file pointer
  scanner: Fix for wrong parameter type of scanner_destroy()
  scanner: Make use of yylex_init_extra()
  parser: Fix for memleak when commands fail

 include/nftables.h |  2 +-
 include/parser.h   |  2 +-
 src/erec.c         | 11 +++++------
 src/main.c         |  2 +-
 src/parser_bison.y |  2 ++
 src/scanner.l      |  8 ++++----
 6 files changed, 14 insertions(+), 13 deletions(-)

-- 
2.13.1

[nft PATCH 1/4] scanner: Fix for memleak due to unclosed file pointer

[Phil Sutter]

When including a file, it is opened by fopen() and therefore needs to be
closed after scanning has finished using fclose(), otherwise valgrind
will report a memleak.

This patch changes struct input_descriptor to track the opened FILE
pointer instead of the file descriptor so the pointer is available for
closing in scanner_destroy().

While at it, change erec_print() to work on the open FILE pointer so it
doesn't have to call fileno() in beforehand. And as a little bonus, use
C99 initializer of the buffer to get rid of the call to memset().

Note that it is necessary to call erec_print_list() prior to destroying
the scanner, otherwise it will start manipulating an already freed FILE
pointer (and therefore crash the program).

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/nftables.h |  2 +-
 src/erec.c         | 11 +++++------
 src/main.c         |  2 +-
 src/scanner.l      |  3 ++-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/nftables.h b/include/nftables.h
index c992d30235670..b55e144021870 100644
--- a/include/nftables.h
+++ b/include/nftables.h
@@ -113,7 +113,7 @@ struct input_descriptor {
 	const char			*name;
 	union {
 		const char		*data;
-		int			fd;
+		FILE			*fp;
 	};
 	unsigned int			lineno;
 	unsigned int			column;
diff --git a/src/erec.c b/src/erec.c
index b5964465fbf3d..f62bc78ccdfab 100644
--- a/src/erec.c
+++ b/src/erec.c
@@ -118,7 +118,7 @@ void erec_print(FILE *f, const struct error_record *erec,
 	const struct location *loc = erec->locations, *iloc;
 	const struct input_descriptor *indesc = loc->indesc, *tmp;
 	const char *line = NULL; /* silence gcc */
-	char buf[1024];
+	char buf[1024] = {};
 	char *pbuf = NULL;
 	unsigned int i, end;
 	int l, ret;
@@ -131,14 +131,13 @@ void erec_print(FILE *f, const struct error_record *erec,
 		*strchrnul(line, '\n') = '\0';
 		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);
+		orig_offset = ftell(indesc->fp);
+		fseek(indesc->fp, loc->line_offset, SEEK_SET);
+		ret = fread(buf, 1, sizeof(buf) - 1, indesc->fp);
 		if (ret > 0)
 			*strchrnul(buf, '\n') = '\0';
 		line = buf;
-		lseek(indesc->fd, orig_offset, SEEK_SET);
+		fseek(indesc->fp, orig_offset, SEEK_SET);
 		break;
 	case INDESC_INTERNAL:
 	case INDESC_NETLINK:
diff --git a/src/main.c b/src/main.c
index 3519377b6e2c6..21bd74aa5fcf1 100644
--- a/src/main.c
+++ b/src/main.c
@@ -428,8 +428,8 @@ int main(int argc, char * const *argv)
 	if (nft_run(&nft, nf_sock, scanner, &state, &msgs) != 0)
 		rc = NFT_EXIT_FAILURE;
 out:
-	scanner_destroy(scanner);
 	erec_print_list(stderr, &msgs, nft.debug_mask);
+	scanner_destroy(scanner);
 	xfree(buf);
 	cache_release(&nft.cache);
 	iface_cache_release();
diff --git a/src/scanner.l b/src/scanner.l
index d50e2b6710654..25e4eb1c70ec1 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -634,7 +634,7 @@ static struct error_record *scanner_push_file(void *scanner, const char *filenam
 		state->indesc->location = *loc;
 	state->indesc->type	= INDESC_FILE;
 	state->indesc->name	= xstrdup(filename);
-	state->indesc->fd	= fileno(f);
+	state->indesc->fp	= f;
 	init_pos(state);
 	return NULL;
 }
@@ -866,6 +866,7 @@ void scanner_destroy(struct parser_state *scanner)
 		if (inpdesc && inpdesc->name) {
 			xfree(inpdesc->name);
 			inpdesc->name = NULL;
+			fclose(inpdesc->fp);
 		}
 		yypop_buffer_state(scanner);
 	} while (state->indesc_idx--);
-- 
2.13.1

[nft PATCH 2/4] scanner: Fix for wrong parameter type of scanner_destroy()

[Phil Sutter]

The function takes the scanner as argument, not the state. This wasn't a
real issue since scanner is a void pointer, which means it's only casted
around without need. So this fix is a rather cosmetic one.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/parser.h | 2 +-
 src/scanner.l    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/parser.h b/include/parser.h
index 0e266d60b8a31..431edfb3d4e34 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -37,7 +37,7 @@ extern void parser_init(struct mnl_socket *nf_sock, struct nft_cache *cache,
 extern int nft_parse(struct nft_ctx *ctx, void *, struct parser_state *state);
 
 extern void *scanner_init(struct parser_state *state);
-extern void scanner_destroy(struct parser_state *state);
+extern void scanner_destroy(void *scanner);
 
 extern int scanner_read_file(void *scanner, const char *filename,
 			     const struct location *loc);
diff --git a/src/scanner.l b/src/scanner.l
index 25e4eb1c70ec1..7d57cc1465d3a 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -856,7 +856,7 @@ void *scanner_init(struct parser_state *state)
 	return scanner;
 }
 
-void scanner_destroy(struct parser_state *scanner)
+void scanner_destroy(void *scanner)
 {
 	struct parser_state *state = yyget_extra(scanner);
 
-- 
2.13.1

[nft PATCH 3/4] scanner: Make use of yylex_init_extra()

[Phil Sutter]

This combines the calls to yylex_init() and yyset_extra().

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/scanner.l | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/scanner.l b/src/scanner.l
index 7d57cc1465d3a..f0021c5c0fbae 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -849,8 +849,7 @@ void *scanner_init(struct parser_state *state)
 
 	state->indesc = state->indescs;
 
-	yylex_init(&scanner);
-	yyset_extra(state, scanner),
+	yylex_init_extra(state, &scanner);
 	yyset_out(NULL, scanner);
 
 	return scanner;
-- 
2.13.1

[nft PATCH 4/4] parser: Fix for memleak when commands fail

[Phil Sutter]

In case of failing command evaluation, commands need to be freed as
their memory becomes orphaned afterwards.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_bison.y | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index a8b71cddc1920..d149178c2679b 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -692,6 +692,7 @@ input			:	/* empty */
 
 					list_add_tail(&$2->list, &list);
 					if (cmd_evaluate(&state->ectx, $2) < 0) {
+						cmd_free($2);
 						if (++state->nerrs == nft->parser_max_errors)
 							YYABORT;
 					} else
@@ -758,6 +759,7 @@ line			:	common_block			{ $$ = NULL; }
 
 					list_add_tail(&$1->list, &list);
 					if (cmd_evaluate(&state->ectx, $1) < 0) {
+						cmd_free($1);
 						if (++state->nerrs == nft->parser_max_errors)
 							YYABORT;
 					} else
-- 
2.13.1

Re: [nft PATCH 0/4] Memleak fixes and some minor cleanups

[Pablo Neira Ayuso]

On Thu, Aug 24, 2017 at 07:14:09PM +0200, Phil Sutter wrote:
> The following series fixes two sources of memleaks as reported by
> valgrind (see patches 1 and 4). Patches 2 and 3 contain some minor
> cleanups I found while searching for the leaks.

Series applied, thanks.