[PATCH] hush: Fix a memory leak in run_command()

[PATCH] hush: Fix a memory leak in run_command()

[Uwe Kleine-König]

parse_string_outer() calls initialize_context(), too. As the latter
allocates memory make sure to only call it once.

This fixes

	automount -d /mnt/foo true
	ls -l /mnt/foo

dying with a failure to allocate memory. Now it results in an endless loop,
which admittedly is only a little bit better :-)

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 common/hush.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/common/hush.c b/common/hush.c
index 6a089fabf11d..117b273ea4e2 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -1887,8 +1887,6 @@ int run_command(const char *cmd)
 	struct p_context ctx = {};
 	int ret;
 
-	initialize_context(&ctx);
-
 	ret = parse_string_outer(&ctx, cmd, FLAG_PARSE_SEMICOLON);
 	release_context(&ctx);
 

base-commit: 298727bc7931fe878fd72a1fa6f35c18bd7c593c
-- 
2.36.1

Re: [PATCH] hush: Fix a memory leak in run_command()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4840 bytes --]

Hello,

On Thu, Jul 14, 2022 at 06:41:18PM +0200, Uwe Kleine-König wrote:
> parse_string_outer() calls initialize_context(), too. As the latter
> allocates memory make sure to only call it once.
> 
> This fixes
> 
> 	automount -d /mnt/foo true
> 	ls -l /mnt/foo
> 
> dying with a failure to allocate memory. Now it results in an endless loop,
> which admittedly is only a little bit better :-)
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  common/hush.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/common/hush.c b/common/hush.c
> index 6a089fabf11d..117b273ea4e2 100644
> --- a/common/hush.c
> +++ b/common/hush.c
> @@ -1887,8 +1887,6 @@ int run_command(const char *cmd)
>  	struct p_context ctx = {};
>  	int ret;
>  
> -	initialize_context(&ctx);
> -
>  	ret = parse_string_outer(&ctx, cmd, FLAG_PARSE_SEMICOLON);
>  	release_context(&ctx);

Just a heads-up, looking at 6bd5212b3d431d4fdc2a0022a06c68939b145c52: I
tried to evaluate the alternative approach, i.e.  relying in
parse_string_outer that ctx is initialized. Then a call to
initialize_context() is required in run_shell().

However dropping the initialize_context() from parse_stream_outer()
breaks things. So adding release_context(ctx); before it would be an
option.

Also shell_expand() should get a call of release_context(&ctx);

So the prototype patch with some printfs looks as follows:

diff --git a/common/hush.c b/common/hush.c
index 5138a1a45ae9..1c654b126305 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -1177,6 +1177,7 @@ static void initialize_context(struct p_context *ctx)
 	ctx->pipe = NULL;
 	ctx->child = NULL;
 	ctx->list_head = new_pipe();
+	printf("%s:%d: ctx=%p, list_head=%p\n", __func__, __LINE__, ctx, ctx->list_head);
 	ctx->pipe = ctx->list_head;
 	ctx->w = RES_NONE;
 	ctx->stack = NULL;
@@ -1188,6 +1189,7 @@ static void initialize_context(struct p_context *ctx)
 
 static void release_context(struct p_context *ctx)
 {
+	printf("%s:%d: ctx=%p, list_head=%p\n", __func__, __LINE__, ctx, ctx->list_head);
 #ifdef CONFIG_CMD_GETOPT
 	struct option *opt, *tmp;
 
@@ -1701,6 +1703,8 @@ char *shell_expand(char *str)
 	free_pipe_list(ctx.list_head, 0);
 	b_free(&o);
 
+	release_context(&ctx);
+
 	return res;
 }
 
@@ -1714,6 +1718,13 @@ static int parse_stream_outer(struct p_context *ctx, struct in_str *inp, int fla
 
 	do {
 		ctx->type = flag;
+
+		/*
+		 * parse_stream_outer() is always called with an initialized
+		 * *ctx.  Before reinitializing it, release the old state to not
+		 * leak memory.
+		 */
+		release_context(ctx);
 		initialize_context(ctx);
 		update_ifs_map();
 
@@ -1954,6 +1965,7 @@ int run_shell(void)
 	do {
 		ctrlc_handled();
 		setup_file_in_str(&input);
+		initialize_context(&ctx);
 		rcode = parse_stream_outer(&ctx, &input, FLAG_PARSE_SEMICOLON);
 		if (rcode < -1) {
 			exit = 1;


Then I get in the sandbox:

	barebox@Sandbox:/ for i in 1; do echo $i; done
	initialize_context:1180: ctx=00007ffcde6bfb90, list_head=00007ff63c7dfc90
	initialize_context:1180: ctx=00007ffcde6bf920, list_head=00007ff63c7e13b0
	release_context:1192: ctx=00007ffcde6bf920, list_head=00007ff63c7e13b0
	initialize_context:1180: ctx=00007ffcde6bf920, list_head=00007ff63c7e1470
	1
	release_context:1192: ctx=00007ffcde6bf920, list_head=00007ff63c7e1470
	release_context:1192: ctx=00007ffcde6bfb90, list_head=00007ff63c7e0660
	initialize_context:1180: ctx=00007ffcde6bfb90, list_head=00007ff63c7e0660
	barebox@Sandbox:/ for i in 1; do echo $i; done
	initialize_context:1180: ctx=00007ffcde6bfb90, list_head=00007ff63c7dfc90
	initialize_context:1180: ctx=00007ffcde6bf920, list_head=00007ff63c7dfc30
	release_context:1192: ctx=00007ffcde6bf920, list_head=00007ff63c7dfc30
	initialize_context:1180: ctx=00007ffcde6bf920, list_head=00007ff63c7dfc60
	1
	release_context:1192: ctx=00007ffcde6bf920, list_head=00007ff63c7dfc60
	release_context:1192: ctx=00007ffcde6bfb90, list_head=00007ff63c7e0660
	initialize_context:1180: ctx=00007ffcde6bfb90, list_head=00007ff63c7e0660

So there is still some inbalance, each command triggers 4x
initialize_context but only 3x release_context.

The further details are unclear to me, e.g. how and when a ctx changes
the list_head and how list_head is freed (that doesn't happen in
release_context()).

So while I think parts of the above patch are correct, I don't
understand the whole picture. The time that I planned to take looking
into this is over now. So here come just a few notes for someone (maybe
a future me) to pick it up later.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]