Re: [PATCH 1/9] fsck: do not reuse child_process structs

[Derrick Stolee]

On 11/12/2018 9:46 AM, Jeff King wrote:

The run-command API makes no promises about what is left in a struct
child_process after a command finishes, and it's not safe to simply
reuse it again for a similar command. In particular:

  - if you use child->args or child->env_array, they are cleared after
finish_command()

  - likewise, start_command() may point child->argv at child->args->argv;
reusing that would lead to accessing freed memory

  - the in/out/err may hold pipe descriptors from the previous run


Thanks! This is helpful information.


These two calls are _probably_ OK because they do not use any of those
features. But it's only by chance, and may break in the future; let's
reinitialize our struct for each program we run.

Signed-off-by: Jeff King 
---
  builtin/fsck.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06eb421720..b10f2b154c 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -841,6 +841,9 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
  
  		prepare_alt_odb(the_repository);

for (alt =  the_repository->objects->alt_odb_list; alt; alt = 
alt->next) {
+   child_process_init(_graph_verify);
+   commit_graph_verify.argv = verify_argv;
+   commit_graph_verify.git_cmd = 1;
verify_argv[2] = "--object-dir";
verify_argv[3] = alt->path;
if (run_command(_graph_verify))
@@ -859,6 +862,9 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
  
  		prepare_alt_odb(the_repository);

for (alt =  the_repository->objects->alt_odb_list; alt; alt = 
alt->next) {
+   child_process_init(_verify);
+   midx_verify.argv = midx_argv;
+   midx_verify.git_cmd = 1;
midx_argv[2] = "--object-dir";
midx_argv[3] = alt->path;
if (run_command(_verify))


Looks good to me.

-Stolee

[PATCH 1/9] fsck: do not reuse child_process structs

[Jeff King]

The run-command API makes no promises about what is left in a struct
child_process after a command finishes, and it's not safe to simply
reuse it again for a similar command. In particular:

 - if you use child->args or child->env_array, they are cleared after
   finish_command()

 - likewise, start_command() may point child->argv at child->args->argv;
   reusing that would lead to accessing freed memory

 - the in/out/err may hold pipe descriptors from the previous run

These two calls are _probably_ OK because they do not use any of those
features. But it's only by chance, and may break in the future; let's
reinitialize our struct for each program we run.

Signed-off-by: Jeff King 
---
 builtin/fsck.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 06eb421720..b10f2b154c 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -841,6 +841,9 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
 
prepare_alt_odb(the_repository);
for (alt =  the_repository->objects->alt_odb_list; alt; alt = 
alt->next) {
+   child_process_init(_graph_verify);
+   commit_graph_verify.argv = verify_argv;
+   commit_graph_verify.git_cmd = 1;
verify_argv[2] = "--object-dir";
verify_argv[3] = alt->path;
if (run_command(_graph_verify))
@@ -859,6 +862,9 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
 
prepare_alt_odb(the_repository);
for (alt =  the_repository->objects->alt_odb_list; alt; alt = 
alt->next) {
+   child_process_init(_verify);
+   midx_verify.argv = midx_argv;
+   midx_verify.git_cmd = 1;
midx_argv[2] = "--object-dir";
midx_argv[3] = alt->path;
if (run_command(_verify))
-- 
2.19.1.1577.g2c5b293d4f