rebase: use child_process_clear() to clean

In the run_am() function, we set up a child_process struct to run "git-am", allocating memory for its args and env strvecs. These are normally cleaned up when we call run_command(). But if we encounter certain errors, we exit the function early and try to clean up ourselves by clearing the am.args field. This leaks the "env" strvec. We should use child_process_clear() instead, which covers both. And more importantly, it future proofs us against the struct ever growing more allocated fields. These are unlikely errors to happen in practice, so they don't actually trigger the leak sanitizer in the tests. But we can add a new test which does exercise one of the paths (and fails SANITIZE=leak without this patch). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Mar 22, 2024 at 06:35 UTC 647e870a08e31984f5f42703f3618ff378a72932
2 files changed +12 -3
builtin/rebase.c
+3 -3
@@ -653,7 +653,7 @@ static int run_am(struct rebase_options *opts)
653 status = error_errno(_("could not open '%s' for writing"),
654 rebased_patches);
655 free(rebased_patches);
656 - strvec_clear(&am.args);
656 + child_process_clear(&am);
657 return status;
658 }
659
@@ -676,7 +676,7 @@ static int run_am(struct rebase_options *opts)
676 struct reset_head_opts ropts = { 0 };
677 unlink(rebased_patches);
678 free(rebased_patches);
679 - strvec_clear(&am.args);
679 + child_process_clear(&am);
680
681 ropts.oid = &opts->orig_head->object.oid;
682 ropts.branch = opts->head_name;
@@ -699,7 +699,7 @@ static int run_am(struct rebase_options *opts)
699 status = error_errno(_("could not open '%s' for reading"),
700 rebased_patches);
701 free(rebased_patches);
702 - strvec_clear(&am.args);
702 + child_process_clear(&am);
703 return status;
704 }
705
t/t3438-rebase-broken-files.sh
+9
@@ -58,4 +58,13 @@ test_expect_success 'unknown key in author-script' '
58 check_resolve_fails
59 '
60
61 +test_expect_success POSIXPERM,SANITY 'unwritable rebased-patches does not leak' '
62 + >.git/rebased-patches &&
63 + chmod a-w .git/rebased-patches &&
64 +
65 + git checkout -b side HEAD^ &&
66 + test_commit unrelated &&
67 + test_must_fail git rebase --apply --onto tmp HEAD^
68 +'
69 +
70 test_done