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@xxxxxxxxxxxxxx>
> ---
> 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:
signature.asc
Description: PGP signature
