sequencer: fix leaking string buffer in `commit_staged_changes()`

We're leaking the `rev` string buffer in various call paths. Refactor
the function to have a common exit path so that we can release its
memory reliably.

This fixes a subset of tests failing with the memory sanitizer in t3404.
But as there are more failures, we cannot yet mark the whole test suite
as passing.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index 475646671a8fdaf6a6056b51461895b606ddfafd..c581061b6db22a3a8d042488ca922b46e6e1db8f 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -5146,33 +5146,47 @@ static int commit_staged_changes(struct repository *r,
        struct replay_ctx *ctx = opts->ctx;
        unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
        unsigned int final_fixup = 0, is_clean;
+       struct strbuf rev = STRBUF_INIT;
+       int ret;
 
-       if (has_unstaged_changes(r, 1))
-               return error(_("cannot rebase: You have unstaged changes."));
+       if (has_unstaged_changes(r, 1)) {
+               ret = error(_("cannot rebase: You have unstaged changes."));
+               goto out;
+       }
 
        is_clean = !has_uncommitted_changes(r, 0);
 
        if (!is_clean && !file_exists(rebase_path_message())) {
                const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
-               return error(_(staged_changes_advice), gpg_opt, gpg_opt);
+               ret = error(_(staged_changes_advice), gpg_opt, gpg_opt);
+               goto out;
        }
+
        if (file_exists(rebase_path_amend())) {
-               struct strbuf rev = STRBUF_INIT;
                struct object_id head, to_amend;
 
-               if (repo_get_oid(r, "HEAD", &head))
-                       return error(_("cannot amend non-existing commit"));
-               if (!read_oneliner(&rev, rebase_path_amend(), 0))
-                       return error(_("invalid file: '%s'"), rebase_path_amend());
-               if (get_oid_hex(rev.buf, &to_amend))
-                       return error(_("invalid contents: '%s'"),
-                               rebase_path_amend());
-               if (!is_clean && !oideq(&head, &to_amend))
-                       return error(_("\nYou have uncommitted changes in your "
-                                      "working tree. Please, commit them\n"
-                                      "first and then run 'git rebase "
-                                      "--continue' again."));
+               if (repo_get_oid(r, "HEAD", &head)) {
+                       ret = error(_("cannot amend non-existing commit"));
+                       goto out;
+               }
+
+               if (!read_oneliner(&rev, rebase_path_amend(), 0)) {
+                       ret = error(_("invalid file: '%s'"), rebase_path_amend());
+                       goto out;
+               }
+
+               if (get_oid_hex(rev.buf, &to_amend)) {
+                       ret = error(_("invalid contents: '%s'"),
+                                   rebase_path_amend());
+                       goto out;
+               }
+               if (!is_clean && !oideq(&head, &to_amend)) {
+                       ret = error(_("\nYou have uncommitted changes in your "
+                                     "working tree. Please, commit them\n"
+                                     "first and then run 'git rebase "
+                                     "--continue' again."));
+                       goto out;
+               }
                /*
                 * When skipping a failed fixup/squash, we need to edit the
                 * commit message, the current fixup list and count, and if it
@@ -5204,9 +5218,11 @@ static int commit_staged_changes(struct repository *r,
                                len--;
                        strbuf_setlen(&ctx->current_fixups, len);
                        if (write_message(p, len, rebase_path_current_fixups(),
-                                         0) < 0)
-                               return error(_("could not write file: '%s'"),
-                                            rebase_path_current_fixups());
+                                         0) < 0) {
+                               ret = error(_("could not write file: '%s'"),
+                                           rebase_path_current_fixups());
+                               goto out;
+                       }
 
                        /*
                         * If a fixup/squash in a fixup/squash chain failed, the
@@ -5236,35 +5252,38 @@ static int commit_staged_changes(struct repository *r,
                                 * We need to update the squash message to skip
                                 * the latest commit message.
                                 */
-                               int res = 0;
                                struct commit *commit;
                                const char *msg;
                                const char *path = rebase_path_squash_msg();
                                const char *encoding = get_commit_output_encoding();
 
-                               if (parse_head(r, &commit))
-                                       return error(_("could not parse HEAD"));
+                               if (parse_head(r, &commit)) {
+                                       ret = error(_("could not parse HEAD"));
+                                       goto out;
+                               }
 
                                p = repo_logmsg_reencode(r, commit, NULL, encoding);
                                if (!p)  {
-                                       res = error(_("could not parse commit %s"),
+                                       ret = error(_("could not parse commit %s"),
                                                    oid_to_hex(&commit->object.oid));
                                        goto unuse_commit_buffer;
                                }
                                find_commit_subject(p, &msg);
                                if (write_message(msg, strlen(msg), path, 0)) {
-                                       res = error(_("could not write file: "
+                                       ret = error(_("could not write file: "
                                                       "'%s'"), path);
                                        goto unuse_commit_buffer;
                                }
+
+                               ret = 0;
+
                        unuse_commit_buffer:
                                repo_unuse_commit_buffer(r, commit, p);
-                               if (res)
-                                       return res;
+                               if (ret)
+                                       goto out;
                        }
                }
 
-               strbuf_release(&rev);
                flags |= AMEND_MSG;
        }
 
@@ -5272,18 +5291,29 @@ static int commit_staged_changes(struct repository *r,
                if (refs_ref_exists(get_main_ref_store(r),
                                    "CHERRY_PICK_HEAD") &&
                    refs_delete_ref(get_main_ref_store(r), "",
-                                   "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF))
-                       return error(_("could not remove CHERRY_PICK_HEAD"));
-               if (unlink(git_path_merge_msg(r)) && errno != ENOENT)
-                       return error_errno(_("could not remove '%s'"),
-                                          git_path_merge_msg(r));
-               if (!final_fixup)
-                       return 0;
+                                   "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF)) {
+                       ret = error(_("could not remove CHERRY_PICK_HEAD"));
+                       goto out;
+               }
+
+               if (unlink(git_path_merge_msg(r)) && errno != ENOENT) {
+                       ret = error_errno(_("could not remove '%s'"),
+                                         git_path_merge_msg(r));
+                       goto out;
+               }
+
+               if (!final_fixup) {
+                       ret = 0;
+                       goto out;
+               }
        }
 
        if (run_git_commit(final_fixup ? NULL : rebase_path_message(),
-                          opts, flags))
-               return error(_("could not commit staged changes."));
+                          opts, flags)) {
+               ret = error(_("could not commit staged changes."));
+               goto out;
+       }
+
        unlink(rebase_path_amend());
        unlink(git_path_merge_head(r));
        refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
@@ -5301,7 +5331,12 @@ static int commit_staged_changes(struct repository *r,
                strbuf_reset(&ctx->current_fixups);
                ctx->current_fixup_count = 0;
        }
-       return 0;
+
+       ret = 0;
+
+out:
+       strbuf_release(&rev);
+       return ret;
 }
 
 int sequencer_continue(struct repository *r, struct replay_opts *opts)