Merge branch 'jk/am-leakfix'
authorJunio C Hamano <gitster@pobox.com>
Tue, 16 May 2017 02:51:53 +0000 (11:51 +0900)
committerJunio C Hamano <gitster@pobox.com>
Tue, 16 May 2017 02:51:53 +0000 (11:51 +0900)
The codepath in "git am" that is used when running "git rebase"
leaked memory held for the log message of the commits being rebased.

* jk/am-leakfix:
am: shorten ident_split variable name in get_commit_info()
am: simplify allocations in get_commit_info()
am: fix commit buffer leak in get_commit_info()

1  2 
builtin/am.c
diff --combined builtin/am.c
index a95dd8b4e6c793d9e51658e2ccbe535cf5a5af45,5c747faa05957bddccd8340fa9faf4e0751479ee..17c80329c23beb2aa60621aa3dc10d95153c61e3
@@@ -134,15 -134,17 +134,15 @@@ struct am_state 
  };
  
  /**
 - * Initializes am_state with the default values. The state directory is set to
 - * dir.
 + * Initializes am_state with the default values.
   */
 -static void am_state_init(struct am_state *state, const char *dir)
 +static void am_state_init(struct am_state *state)
  {
        int gpgsign;
  
        memset(state, 0, sizeof(*state));
  
 -      assert(dir);
 -      state->dir = xstrdup(dir);
 +      state->dir = git_pathdup("rebase-apply");
  
        state->prec = 4;
  
@@@ -760,18 -762,14 +760,18 @@@ static int split_mail_conv(mail_conv_f
                mail = mkpath("%s/%0*d", state->dir, state->prec, i + 1);
  
                out = fopen(mail, "w");
 -              if (!out)
 +              if (!out) {
 +                      if (in != stdin)
 +                              fclose(in);
                        return error_errno(_("could not open '%s' for writing"),
                                           mail);
 +              }
  
                ret = fn(out, in, keep_cr);
  
                fclose(out);
 -              fclose(in);
 +              if (in != stdin)
 +                      fclose(in);
  
                if (ret)
                        return error(_("could not parse patch '%s'"), *paths);
@@@ -1051,7 -1049,7 +1051,7 @@@ static void am_setup(struct am_state *s
        } else {
                write_state_text(state, "abort-safety", "");
                if (!state->rebasing)
 -                      delete_ref("ORIG_HEAD", NULL, 0);
 +                      delete_ref(NULL, "ORIG_HEAD", NULL, 0);
        }
  
        /*
@@@ -1121,7 -1119,7 +1121,7 @@@ static void refresh_and_write_cache(voi
  {
        struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
  
 -      hold_locked_index(lock_file, 1);
 +      hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
        refresh_cache(REFRESH_QUIET);
        if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
                die(_("unable to write index file"));
@@@ -1183,39 -1181,42 +1183,39 @@@ static void NORETURN die_user_resolve(c
        exit(128);
  }
  
 -static void am_signoff(struct strbuf *sb)
 +/**
 + * Appends signoff to the "msg" field of the am_state.
 + */
 +static void am_append_signoff(struct am_state *state)
  {
        char *cp;
        struct strbuf mine = STRBUF_INIT;
 +      struct strbuf sb = STRBUF_INIT;
  
 -      /* Does it end with our own sign-off? */
 +      strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
 +
 +      /* our sign-off */
        strbuf_addf(&mine, "\n%s%s\n",
                    sign_off_header,
                    fmt_name(getenv("GIT_COMMITTER_NAME"),
                             getenv("GIT_COMMITTER_EMAIL")));
 -      if (mine.len < sb->len &&
 -          !strcmp(mine.buf, sb->buf + sb->len - mine.len))
 +
 +      /* Does sb end with it already? */
 +      if (mine.len < sb.len &&
 +          !strcmp(mine.buf, sb.buf + sb.len - mine.len))
                goto exit; /* no need to duplicate */
  
        /* Does it have any Signed-off-by: in the text */
 -      for (cp = sb->buf;
 +      for (cp = sb.buf;
             cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
             cp = strchr(cp, '\n')) {
 -              if (sb->buf == cp || cp[-1] == '\n')
 +              if (sb.buf == cp || cp[-1] == '\n')
                        break;
        }
  
 -      strbuf_addstr(sb, mine.buf + !!cp);
 +      strbuf_addstr(&sb, mine.buf + !!cp);
  exit:
        strbuf_release(&mine);
 -}
 -
 -/**
 - * Appends signoff to the "msg" field of the am_state.
 - */
 -static void am_append_signoff(struct am_state *state)
 -{
 -      struct strbuf sb = STRBUF_INIT;
 -
 -      strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
 -      am_signoff(&sb);
        state->msg = strbuf_detach(&sb, &state->msg_len);
  }
  
@@@ -1320,6 -1321,9 +1320,6 @@@ static int parse_mail(struct am_state *
        strbuf_addbuf(&msg, &mi.log_message);
        strbuf_stripspace(&msg, 0);
  
 -      if (state->signoff)
 -              am_signoff(&msg);
 -
        assert(!state->author_name);
        state->author_name = strbuf_detach(&author_name, NULL);
  
@@@ -1372,40 -1376,33 +1372,33 @@@ static int get_mail_commit_oid(struct o
   */
  static void get_commit_info(struct am_state *state, struct commit *commit)
  {
-       const char *buffer, *ident_line, *author_date, *msg;
+       const char *buffer, *ident_line, *msg;
        size_t ident_len;
-       struct ident_split ident_split;
-       struct strbuf sb = STRBUF_INIT;
+       struct ident_split id;
  
        buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());
  
        ident_line = find_commit_header(buffer, "author", &ident_len);
  
-       if (split_ident_line(&ident_split, ident_line, ident_len) < 0) {
-               strbuf_add(&sb, ident_line, ident_len);
-               die(_("invalid ident line: %s"), sb.buf);
-       }
+       if (split_ident_line(&id, ident_line, ident_len) < 0)
+               die(_("invalid ident line: %.*s"), (int)ident_len, ident_line);
  
        assert(!state->author_name);
-       if (ident_split.name_begin) {
-               strbuf_add(&sb, ident_split.name_begin,
-                       ident_split.name_end - ident_split.name_begin);
-               state->author_name = strbuf_detach(&sb, NULL);
-       } else
+       if (id.name_begin)
+               state->author_name =
+                       xmemdupz(id.name_begin, id.name_end - id.name_begin);
+       else
                state->author_name = xstrdup("");
  
        assert(!state->author_email);
-       if (ident_split.mail_begin) {
-               strbuf_add(&sb, ident_split.mail_begin,
-                       ident_split.mail_end - ident_split.mail_begin);
-               state->author_email = strbuf_detach(&sb, NULL);
-       } else
+       if (id.mail_begin)
+               state->author_email =
+                       xmemdupz(id.mail_begin, id.mail_end - id.mail_begin);
+       else
                state->author_email = xstrdup("");
  
-       author_date = show_ident_date(&ident_split, DATE_MODE(NORMAL));
-       strbuf_addstr(&sb, author_date);
        assert(!state->author_date);
-       state->author_date = strbuf_detach(&sb, NULL);
+       state->author_date = xstrdup(show_ident_date(&id, DATE_MODE(NORMAL)));
  
        assert(!state->msg);
        msg = strstr(buffer, "\n\n");
                die(_("unable to parse commit %s"), oid_to_hex(&commit->object.oid));
        state->msg = xstrdup(msg + 2);
        state->msg_len = strlen(state->msg);
+       unuse_commit_buffer(commit, buffer);
  }
  
  /**
@@@ -1844,9 -1842,6 +1838,9 @@@ static void am_run(struct am_state *sta
                        if (skip)
                                goto next; /* mail should be skipped */
  
 +                      if (state->signoff)
 +                              am_append_signoff(state);
 +
                        write_author_script(state);
                        write_commit_msg(state);
                }
@@@ -1975,7 -1970,7 +1969,7 @@@ static int fast_forward_to(struct tree 
                return -1;
  
        lock_file = xcalloc(1, sizeof(struct lock_file));
 -      hold_locked_index(lock_file, 1);
 +      hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
  
        refresh_cache(REFRESH_QUIET);
  
@@@ -2015,7 -2010,7 +2009,7 @@@ static int merge_tree(struct tree *tree
                return -1;
  
        lock_file = xcalloc(1, sizeof(struct lock_file));
 -      hold_locked_index(lock_file, 1);
 +      hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
  
        memset(&opts, 0, sizeof(opts));
        opts.head_idx = 1;
@@@ -2171,7 -2166,7 +2165,7 @@@ static void am_abort(struct am_state *s
                                has_curr_head ? &curr_head : NULL, 0,
                                UPDATE_REFS_DIE_ON_ERR);
        else if (curr_branch)
 -              delete_ref(curr_branch, NULL, REF_NODEREF);
 +              delete_ref(NULL, curr_branch, NULL, REF_NODEREF);
  
        free(curr_branch);
        am_destroy(state);
@@@ -2321,7 -2316,7 +2315,7 @@@ int cmd_am(int argc, const char **argv
  
        git_config(git_am_config, NULL);
  
 -      am_state_init(&state, git_path("rebase-apply"));
 +      am_state_init(&state);
  
        in_progress = am_in_progress(&state);
        if (in_progress)