[nft PATCH 0/4] cli: Make valgrind (kind of) happy

[nft PATCH 0/4] cli: Make valgrind (kind of) happy

[Phil Sutter]

The following series is more or less a v2 of my previous single patch
adding nft_ctx_free() calls to cli.c, following a different path:
Eliminate any program exit points from cli.c so nft context deinit at
end of main() happens. This is nicer design as said function allocates
the context in the first place.

Patch 1 is minor cleanup, patch 2 updates main() to free the context in
all cases, too and patch 3 then changes CLI code as described above.
Patch 4 extends shell testsuite by a '-V' (valgrind) mode as present in
iptables' shell testsuite already which wraps all calls to $NFT by
valgrind and collects non-empty logs.

Sadly, I could not eliminate all valgrind complaints because each of the
three CLI backends leaves allocated memory in place at exit. None seem
to have sufficient deinit functions, except linenoise - but that code
runs only for terminals put in raw mode.

Phil Sutter (4):
  main: Make 'buf' variable branch-local
  main: Call nft_ctx_free() before exiting
  cli: Make cli_init() return to caller
  tests: shell: Introduce valgrind mode

 include/cli.h            |  2 +-
 src/cli.c                | 63 ++++++++++++++++++++++++++--------------
 src/main.c               | 42 +++++++++++++++------------
 tests/shell/run-tests.sh | 47 ++++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 41 deletions(-)

-- 
2.40.0

[nft PATCH 1/4] main: Make 'buf' variable branch-local

[Phil Sutter]

It is used only to linearize non-option argv for passing to
nft_run_cmd_from_buffer(), reduce its scope. Allows to safely move the
free() call there, too.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/main.c b/src/main.c
index cb20850b71c5b..a1592c1823f49 100644
--- a/src/main.c
+++ b/src/main.c
@@ -361,9 +361,9 @@ int main(int argc, char * const *argv)
 	const struct option *options = get_options();
 	bool interactive = false, define = false;
 	const char *optstring = get_optstring();
-	char *buf = NULL, *filename = NULL;
 	unsigned int output_flags = 0;
 	unsigned int debug_mask;
+	char *filename = NULL;
 	unsigned int len;
 	int i, val, rc;
 
@@ -514,6 +514,8 @@ int main(int argc, char * const *argv)
 	nft_ctx_output_set_flags(nft, output_flags);
 
 	if (optind != argc) {
+		char *buf;
+
 		for (len = 0, i = optind; i < argc; i++)
 			len += strlen(argv[i]) + strlen(" ");
 
@@ -529,6 +531,7 @@ int main(int argc, char * const *argv)
 				strcat(buf, " ");
 		}
 		rc = !!nft_run_cmd_from_buffer(nft, buf);
+		free(buf);
 	} else if (filename != NULL) {
 		rc = !!nft_run_cmd_from_filename(nft, filename);
 	} else if (interactive) {
@@ -543,7 +546,6 @@ int main(int argc, char * const *argv)
 		exit(EXIT_FAILURE);
 	}
 
-	free(buf);
 	nft_ctx_free(nft);
 
 	return rc;
-- 
2.40.0

[nft PATCH 2/4] main: Call nft_ctx_free() before exiting

[Phil Sutter]

Introduce labels for failure and regular exit so all direct exit() calls
after nft_ctx allocation may be replaced by a single goto statement.

Simply drop that return call in interactive branch, code will continue
at 'out' label naturally.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/main.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/main.c b/src/main.c
index a1592c1823f49..40dc60c2258cc 100644
--- a/src/main.c
+++ b/src/main.c
@@ -362,10 +362,10 @@ int main(int argc, char * const *argv)
 	bool interactive = false, define = false;
 	const char *optstring = get_optstring();
 	unsigned int output_flags = 0;
+	int i, val, rc = EXIT_SUCCESS;
 	unsigned int debug_mask;
 	char *filename = NULL;
 	unsigned int len;
-	int i, val, rc;
 
 	/* nftables cannot be used with setuid in a safe way. */
 	if (getuid() != geteuid())
@@ -384,20 +384,20 @@ int main(int argc, char * const *argv)
 		switch (val) {
 		case OPT_HELP:
 			show_help(argv[0]);
-			exit(EXIT_SUCCESS);
+			goto out;
 		case OPT_VERSION:
 			printf("%s v%s (%s)\n",
 			       PACKAGE_NAME, PACKAGE_VERSION, RELEASE_NAME);
-			exit(EXIT_SUCCESS);
+			goto out;
 		case OPT_VERSION_LONG:
 			show_version();
-			exit(EXIT_SUCCESS);
+			goto out;
 		case OPT_DEFINE:
 			if (nft_ctx_add_var(nft, optarg)) {
 				fprintf(stderr,
 					"Failed to define variable '%s'\n",
 					optarg);
-				exit(EXIT_FAILURE);
+				goto out_fail;
 			}
 			define = true;
 			break;
@@ -408,7 +408,7 @@ int main(int argc, char * const *argv)
 			if (interactive) {
 				fprintf(stderr,
 					"Error: -i/--interactive and -f/--file options cannot be combined\n");
-				exit(EXIT_FAILURE);
+				goto out_fail;
 			}
 			filename = optarg;
 			break;
@@ -416,7 +416,7 @@ int main(int argc, char * const *argv)
 			if (filename) {
 				fprintf(stderr,
 					"Error: -i/--interactive and -f/--file options cannot be combined\n");
-				exit(EXIT_FAILURE);
+				goto out_fail;
 			}
 			interactive = true;
 			break;
@@ -425,7 +425,7 @@ int main(int argc, char * const *argv)
 				fprintf(stderr,
 					"Failed to add include path '%s'\n",
 					optarg);
-				exit(EXIT_FAILURE);
+				goto out_fail;
 			}
 			break;
 		case OPT_NUMERIC:
@@ -460,7 +460,7 @@ int main(int argc, char * const *argv)
 				if (i == array_size(debug_param)) {
 					fprintf(stderr, "invalid debug parameter `%s'\n",
 						optarg);
-					exit(EXIT_FAILURE);
+					goto out_fail;
 				}
 
 				if (end == NULL)
@@ -480,7 +480,7 @@ int main(int argc, char * const *argv)
 			output_flags |= NFT_CTX_OUTPUT_JSON;
 #else
 			fprintf(stderr, "JSON support not compiled-in\n");
-			exit(EXIT_FAILURE);
+			goto out_fail;
 #endif
 			break;
 		case OPT_GUID:
@@ -502,13 +502,13 @@ int main(int argc, char * const *argv)
 			nft_ctx_set_optimize(nft, 0x1);
 			break;
 		case OPT_INVALID:
-			exit(EXIT_FAILURE);
+			goto out_fail;
 		}
 	}
 
 	if (!filename && define) {
 		fprintf(stderr, "Error: -D/--define can only be used with -f/--filename\n");
-		exit(EXIT_FAILURE);
+		goto out_fail;
 	}
 
 	nft_ctx_output_set_flags(nft, output_flags);
@@ -523,7 +523,7 @@ int main(int argc, char * const *argv)
 		if (buf == NULL) {
 			fprintf(stderr, "%s:%u: Memory allocation failure\n",
 				__FILE__, __LINE__);
-			exit(EXIT_FAILURE);
+			goto out_fail;
 		}
 		for (i = optind; i < argc; i++) {
 			strcat(buf, argv[i]);
@@ -538,15 +538,17 @@ int main(int argc, char * const *argv)
 		if (cli_init(nft) < 0) {
 			fprintf(stderr, "%s: interactive CLI not supported in this build\n",
 				argv[0]);
-			exit(EXIT_FAILURE);
+			goto out_fail;
 		}
-		return EXIT_SUCCESS;
 	} else {
 		fprintf(stderr, "%s: no command specified\n", argv[0]);
-		exit(EXIT_FAILURE);
+		goto out_fail;
 	}
 
+out:
 	nft_ctx_free(nft);
-
 	return rc;
+out_fail:
+	nft_ctx_free(nft);
+	return EXIT_FAILURE;
 }
-- 
2.40.0

[nft PATCH 3/4] cli: Make cli_init() return to caller

[Phil Sutter]

Avoid direct exit() calls as that leaves the caller-allocated nft_ctx
object in place. Making sure it is freed helps with valgrind-analyses at
least.

To signal desired exit from CLI, introduce global cli_quit boolean and
make all cli_exit() implementations also set cli_rc variable to the
appropriate return code.

The logic is to finish CLI only if cli_quit is true which asserts proper
cleanup as it is set only by the respective cli_exit() function.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/cli.h |  2 +-
 src/cli.c     | 63 ++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/include/cli.h b/include/cli.h
index 609ed2ed0e0aa..c854948e4a16d 100644
--- a/include/cli.h
+++ b/include/cli.h
@@ -12,6 +12,6 @@ static inline int cli_init(struct nft_ctx *nft)
         return -1;
 }
 #endif
-extern void cli_exit(void);
+extern void cli_exit(int rc);
 
 #endif
diff --git a/src/cli.c b/src/cli.c
index 11fc85abeaa2b..5f7e01ff96314 100644
--- a/src/cli.c
+++ b/src/cli.c
@@ -37,6 +37,15 @@
 #define CMDLINE_PROMPT		"nft> "
 #define CMDLINE_QUIT		"quit"
 
+static bool cli_quit;
+static int cli_rc;
+
+static void __cli_exit(int rc)
+{
+	cli_quit = true;
+	cli_rc = rc;
+}
+
 static char histfile[PATH_MAX];
 
 static void
@@ -100,8 +109,8 @@ static char *cli_append_multiline(char *line)
 		if (!s) {
 			fprintf(stderr, "%s:%u: Memory allocation failure\n",
 				__FILE__, __LINE__);
-			cli_exit();
-			exit(EXIT_FAILURE);
+			cli_exit(EXIT_FAILURE);
+			return NULL;
 		}
 		snprintf(s, len + 1, "%s%s", multiline, line);
 		free(multiline);
@@ -125,8 +134,7 @@ static void cli_complete(char *line)
 
 	if (line == NULL) {
 		printf("\n");
-		cli_exit();
-		exit(0);
+		return cli_exit(0);
 	}
 
 	line = cli_append_multiline(line);
@@ -139,10 +147,8 @@ static void cli_complete(char *line)
 	if (*c == '\0')
 		return;
 
-	if (!strcmp(line, CMDLINE_QUIT)) {
-		cli_exit();
-		exit(0);
-	}
+	if (!strcmp(line, CMDLINE_QUIT))
+		return cli_exit(0);
 
 	/* avoid duplicate history entries */
 	hist = history_get(history_length);
@@ -176,16 +182,19 @@ int cli_init(struct nft_ctx *nft)
 	read_history(histfile);
 	history_set_pos(history_length);
 
-	while (true)
+	while (!cli_quit)
 		rl_callback_read_char();
-	return 0;
+
+	return cli_rc;
 }
 
-void cli_exit(void)
+void cli_exit(int rc)
 {
 	rl_callback_handler_remove();
 	rl_deprep_terminal();
 	write_history(histfile);
+
+	__cli_exit(rc);
 }
 
 #elif defined(HAVE_LIBEDIT)
@@ -205,51 +214,63 @@ int cli_init(struct nft_ctx *nft)
 	history_set_pos(history_length);
 
 	rl_set_prompt(CMDLINE_PROMPT);
-	while ((line = readline(rl_prompt)) != NULL) {
+	while (!cli_quit) {
+		line = readline(rl_prompt);
+		if (!line) {
+			cli_exit(0);
+			break;
+		}
 		line = cli_append_multiline(line);
 		if (!line)
 			continue;
 
 		cli_complete(line);
 	}
-	cli_exit();
 
-	return 0;
+	return cli_rc;
 }
 
-void cli_exit(void)
+void cli_exit(int rc)
 {
 	rl_deprep_terminal();
 	write_history(histfile);
+
+	__cli_exit(rc);
 }
 
 #else /* HAVE_LINENOISE */
 
 int cli_init(struct nft_ctx *nft)
 {
-	int quit = 0;
 	char *line;
 
 	init_histfile();
 	linenoiseHistoryLoad(histfile);
 	linenoiseSetMultiLine(1);
 
-	while (!quit && (line = linenoise(CMDLINE_PROMPT)) != NULL) {
+	while (!cli_quit) {
+		line = linenoise(CMDLINE_PROMPT);
+		if (!line) {
+			cli_exit(0);
+			break;
+		}
 		if (strcmp(line, CMDLINE_QUIT) == 0) {
-			quit = 1;
+			cli_exit(0);
 		} else if (line[0] != '\0') {
 			linenoiseHistoryAdd(line);
 			nft_run_cmd_from_buffer(nft, line);
 		}
 		linenoiseFree(line);
 	}
-	cli_exit();
-	exit(0);
+
+	return cli_rc;
 }
 
-void cli_exit(void)
+void cli_exit(int rc)
 {
 	linenoiseHistorySave(histfile);
+
+	__cli_exit(rc);
 }
 
 #endif /* HAVE_LINENOISE */
-- 
2.40.0

[nft PATCH 4/4] tests: shell: Introduce valgrind mode

[Phil Sutter]

Pass flag '-V' to run-tests.sh to run all 'nft' invocations in valgrind
leak checking environment. Code copied from iptables' shell-testsuite
where it proved to be useful already.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/shell/run-tests.sh | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh
index 931bba967b370..1a69987598314 100755
--- a/tests/shell/run-tests.sh
+++ b/tests/shell/run-tests.sh
@@ -69,6 +69,11 @@ if [ "$1" == "-g" ] ; then
 	shift
 fi
 
+if [ "$1" == "-V" ] ; then
+	VALGRIND=y
+	shift
+fi
+
 for arg in "$@"; do
 	SINGLE+=" $arg"
 	VERBOSE=y
@@ -106,6 +111,48 @@ find_tests() {
 	${FIND} ${TESTDIR} -type f -executable | sort
 }
 
+printscript() { # (cmd, tmpd)
+	cat <<EOF
+#!/bin/bash
+
+CMD="$1"
+
+# note: valgrind man page warns about --log-file with --trace-children, the
+# last child executed overwrites previous reports unless %p or %q is used.
+# Since libtool wrapper calls exec but none of the iptables tools do, this is
+# perfect for us as it effectively hides bash-related errors
+
+valgrind --log-file=$2/valgrind.log --trace-children=yes \
+	 --leak-check=full --show-leak-kinds=all \$CMD "\$@"
+RC=\$?
+
+# don't keep uninteresting logs
+if grep -q 'no leaks are possible' $2/valgrind.log; then
+	rm $2/valgrind.log
+else
+	mv $2/valgrind.log $2/valgrind_\$\$.log
+fi
+
+# drop logs for failing commands for now
+[ \$RC -eq 0 ] || rm $2/valgrind_\$\$.log
+
+exit \$RC
+EOF
+}
+
+if [ "$VALGRIND" == "y" ]; then
+	tmpd=$(mktemp -d)
+	chmod 755 $tmpd
+
+	msg_info "writing valgrind logs to $tmpd"
+
+	printscript "$NFT" "$tmpd" >${tmpd}/nft
+	trap "rm ${tmpd}/nft" EXIT
+	chmod a+x ${tmpd}/nft
+
+	NFT="${tmpd}/nft"
+fi
+
 echo ""
 ok=0
 failed=0
-- 
2.40.0

Re: [nft PATCH 0/4] cli: Make valgrind (kind of) happy

[Phil Sutter]

On Thu, Jun 22, 2023 at 05:46:30PM +0200, Phil Sutter wrote:
> The following series is more or less a v2 of my previous single patch
> adding nft_ctx_free() calls to cli.c, following a different path:
> Eliminate any program exit points from cli.c so nft context deinit at
> end of main() happens. This is nicer design as said function allocates
> the context in the first place.
> 
> Patch 1 is minor cleanup, patch 2 updates main() to free the context in
> all cases, too and patch 3 then changes CLI code as described above.
> Patch 4 extends shell testsuite by a '-V' (valgrind) mode as present in
> iptables' shell testsuite already which wraps all calls to $NFT by
> valgrind and collects non-empty logs.
> 
> Sadly, I could not eliminate all valgrind complaints because each of the
> three CLI backends leaves allocated memory in place at exit. None seem
> to have sufficient deinit functions, except linenoise - but that code
> runs only for terminals put in raw mode.
> 
> Phil Sutter (4):
>   main: Make 'buf' variable branch-local
>   main: Call nft_ctx_free() before exiting
>   cli: Make cli_init() return to caller
>   tests: shell: Introduce valgrind mode

Series applied.