mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: barebox@lists.infradead.org
Subject: Re: [PATCH] hush: Fix a memory leak in run_command()
Date: Wed, 12 Apr 2023 13:29:45 +0200	[thread overview]
Message-ID: <20230412112945.2fvnj7d7wnf7ybzv@pengutronix.de> (raw)
In-Reply-To: <20220714164118.56761-1-u.kleine-koenig@pengutronix.de>

[-- 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 --]

      reply	other threads:[~2023-04-12 11:33 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-14 16:41 Uwe Kleine-König
2023-04-12 11:29 ` Uwe Kleine-König [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230412112945.2fvnj7d7wnf7ybzv@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox