[PATCH v2 1/2] Clean stale environment pointer in finish_command()

In start_command(), unset "env" fields are initialized via "env_array". In
finish_command(), the "env_array" is cleared, therefore the "env" field
will point to free()d data.

However, start_command() will set "env" to env_array.argv only if "env"
was unset to begin with, and if it was already set, the caller will need
the original value. Therefore, we need to be very careful only to reset
"env" in finish_command() when it has been initialized in start_command().

Signed-off-by: Johannes Schindelin <redacted>
---
 run-command.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/run-command.c b/run-command.c
index 79a0a76..85578da 100644
--- a/run-command.c
+++ b/run-command.c

@@ -555,6 +555,9 @@ int finish_command(struct child_process *cmd)
 {
 	int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
 	argv_array_clear(&cmd->args);
+	/* Avoid pointing to a stale environment */
+	if (cmd->env == cmd->env_array.argv)
+		cmd->env = NULL;
 	argv_array_clear(&cmd->env_array);
 	return ret;
 }

-- 
2.0.0.rc3.9669.g840d1f9

Hi,

On Mon, 10 Nov 2014, Johannes Schindelin wrote:

In start_command(), unset "env" fields are initialized via "env_array". In
finish_command(), the "env_array" is cleared, therefore the "env" field
will point to free()d data.

However, start_command() will set "env" to env_array.argv only if "env"
was unset to begin with, and if it was already set, the caller will need
the original value. Therefore, we need to be very careful only to reset
"env" in finish_command() when it has been initialized in start_command().

In case it was unclear: this is needed for the the suggested switch from the
previous method to construct the environment to the new env_array method
to work.

(The env_array method unfortunately requires the code to initialize the
environment twice because finish_command() insists on always releasing the
env_array, even if the caller may want to reuse the generated array).

Ciao,
Johannes

On Mon, Nov 10, 2014 at 03:41:09PM +0100, Johannes Schindelin wrote:

quoted

However, start_command() will set "env" to env_array.argv only if "env"
was unset to begin with, and if it was already set, the caller will need
the original value. Therefore, we need to be very careful only to reset
"env" in finish_command() when it has been initialized in start_command().

In case it was unclear: this is needed for the the suggested switch from the
previous method to construct the environment to the new env_array method
to work.

(The env_array method unfortunately requires the code to initialize the
environment twice because finish_command() insists on always releasing the
env_array, even if the caller may want to reuse the generated array).

I don't think this is "unfortunately"; freeing the memory was the entire
purpose in adding env_array. If you want to easily reuse the same
environment in multiple commands, it is still perfectly fine to use
"env" directly, like:

  struct argv_array env = ARGV_ARRAY_INIT;
  struct child_process one = CHILD_PROCESS_INIT;
  struct child_process two = CHILD_PROCESS_INIT;

  ... setup env with argv_array_push ...

  one.argv = foo;
  one.env = env.argv;
  run_command(&one);

  two.argv = bar;
  two.env = env.argv;
  run_command(&two);

  argv_array_clear(&env);

You do not get the benefit of the auto-cleanup (you have to call
argv_array_clear yourself), but that is less bad than repeating the
setup of "env" twice.

It may be that Documentation/technical/api-run-command.txt needs to
make this more explicit (I just read over it and it looks OK to me. But
since I am the one who designed the feature, I am not the best test of
whether it is sufficiently clear).

-Peff