From: Junio C Hamano Date: Tue, 19 Sep 2017 01:47:57 +0000 (+0900) Subject: Merge branch 'rs/strbuf-leakfix' X-Git-Tag: v2.15.0-rc0~78 X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/d811ba1897b723fd0146875201061e3603c16391?ds=inline;hp=-c Merge branch 'rs/strbuf-leakfix' Many leaks of strbuf have been fixed. * rs/strbuf-leakfix: (34 commits) wt-status: release strbuf after use in wt_longstatus_print_tracking() wt-status: release strbuf after use in read_rebase_todolist() vcs-svn: release strbuf after use in end_revision() utf8: release strbuf on error return in strbuf_utf8_replace() userdiff: release strbuf after use in userdiff_get_textconv() transport-helper: release strbuf after use in process_connect_service() sequencer: release strbuf after use in save_head() shortlog: release strbuf after use in insert_one_record() sha1_file: release strbuf on error return in index_path() send-pack: release strbuf on error return in send_pack() remote: release strbuf after use in set_url() remote: release strbuf after use in migrate_file() remote: release strbuf after use in read_remote_branches() refs: release strbuf on error return in write_pseudoref() notes: release strbuf after use in notes_copy_from_stdin() merge: release strbuf after use in write_merge_heads() merge: release strbuf after use in save_state() mailinfo: release strbuf on error return in handle_boundary() mailinfo: release strbuf after use in handle_from() help: release strbuf on error return in exec_woman_emacs() ... --- d811ba1897b723fd0146875201061e3603c16391 diff --combined builtin/merge.c index 66b718678e,7bc3fe4b6d..ab5ffe85e8 --- a/builtin/merge.c +++ b/builtin/merge.c @@@ -73,7 -73,6 +73,7 @@@ static int show_progress = -1 static int default_to_upstream = 1; static int signoff; static const char *sign_commit; +static int verify_msg = 1; static struct strategy all_strategy[] = { { "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL }, @@@ -237,7 -236,6 +237,7 @@@ static struct option builtin_merge_opti N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")), OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")), + OPT_BOOL(0, "verify", &verify_msg, N_("verify commit-msg hook")), OPT_END() }; @@@ -255,6 -253,7 +255,7 @@@ static int save_state(struct object_id struct child_process cp = CHILD_PROCESS_INIT; struct strbuf buffer = STRBUF_INIT; const char *argv[] = {"stash", "create", NULL}; + int rc = -1; cp.argv = argv; cp.out = -1; @@@ -268,11 -267,14 +269,14 @@@ if (finish_command(&cp) || len < 0) die(_("stash failed")); else if (!len) /* no changes */ - return -1; + goto out; strbuf_setlen(&buffer, buffer.len-1); if (get_oid(buffer.buf, stash)) die(_("not a valid object: %s"), buffer.buf); - return 0; + rc = 0; + out: + strbuf_release(&buffer); + return rc; } static void read_empty(unsigned const char *sha1, int verbose) @@@ -782,12 -784,6 +786,12 @@@ static void prepare_to_commit(struct co if (launch_editor(git_path_merge_msg(), NULL, NULL)) abort_commit(remoteheads, NULL); } + + if (verify_msg && run_commit_hook(0 < option_edit, get_index_file(), + "commit-msg", + git_path_merge_msg(), NULL)) + abort_commit(remoteheads, NULL); + read_merge_msg(&msg); strbuf_stripspace(&msg, 0 < option_edit); if (!msg.len) @@@ -942,6 -938,7 +946,7 @@@ static void write_merge_heads(struct co if (fast_forward == FF_NO) strbuf_addstr(&buf, "no-ff"); write_file_buf(git_path_merge_mode(), buf.buf, buf.len); + strbuf_release(&buf); } static void write_merge_state(struct commit_list *remoteheads) @@@ -1367,7 -1364,7 +1372,7 @@@ int cmd_merge(int argc, const char **ar * If head can reach all the merge then we are up to date. * but first the most common case of merging one remote. */ - finish_up_to_date(_("Already up-to-date.")); + finish_up_to_date(_("Already up to date.")); goto done; } else if (fast_forward != FF_NO && !remoteheads->next && !common->next && @@@ -1450,7 -1447,7 +1455,7 @@@ } } if (up_to_date) { - finish_up_to_date(_("Already up-to-date. Yeeah!")); + finish_up_to_date(_("Already up to date. Yeeah!")); goto done; } } diff --combined commit.c index 17a93d1e64,51f969fcbc..906298052d --- a/commit.c +++ b/commit.c @@@ -134,41 -134,35 +134,41 @@@ int register_commit_graft(struct commit return 0; } -struct commit_graft *read_graft_line(char *buf, int len) +struct commit_graft *read_graft_line(struct strbuf *line) { /* The format is just "Commit Parent1 Parent2 ...\n" */ - int i; + int i, phase; + const char *tail = NULL; struct commit_graft *graft = NULL; - const int entry_size = GIT_SHA1_HEXSZ + 1; + struct object_id dummy_oid, *oid; - while (len && isspace(buf[len-1])) - buf[--len] = '\0'; - if (buf[0] == '#' || buf[0] == '\0') + strbuf_rtrim(line); + if (!line->len || line->buf[0] == '#') return NULL; - if ((len + 1) % entry_size) - goto bad_graft_data; - i = (len + 1) / entry_size - 1; - graft = xmalloc(st_add(sizeof(*graft), st_mult(GIT_SHA1_RAWSZ, i))); - graft->nr_parent = i; - if (get_oid_hex(buf, &graft->oid)) - goto bad_graft_data; - for (i = GIT_SHA1_HEXSZ; i < len; i += entry_size) { - if (buf[i] != ' ') - goto bad_graft_data; - if (get_sha1_hex(buf + i + 1, graft->parent[i/entry_size].hash)) + /* + * phase 0 verifies line, counts hashes in line and allocates graft + * phase 1 fills graft + */ + for (phase = 0; phase < 2; phase++) { + oid = graft ? &graft->oid : &dummy_oid; + if (parse_oid_hex(line->buf, oid, &tail)) goto bad_graft_data; + for (i = 0; *tail != '\0'; i++) { + oid = graft ? &graft->parent[i] : &dummy_oid; + if (!isspace(*tail++) || parse_oid_hex(tail, oid, &tail)) + goto bad_graft_data; + } + if (!graft) { + graft = xmalloc(st_add(sizeof(*graft), + st_mult(sizeof(struct object_id), i))); + graft->nr_parent = i; + } } return graft; bad_graft_data: - error("bad graft data: %s", buf); - free(graft); + error("bad graft data: %s", line->buf); + assert(!graft); return NULL; } @@@ -180,7 -174,7 +180,7 @@@ static int read_graft_file(const char * return -1; while (!strbuf_getwholeline(&buf, fp, '\n')) { /* The format is just "Commit Parent1 Parent2 ...\n" */ - struct commit_graft *graft = read_graft_line(buf.buf, buf.len); + struct commit_graft *graft = read_graft_line(&buf); if (!graft) continue; if (register_commit_graft(graft, 1)) @@@ -1570,10 -1564,13 +1570,13 @@@ int commit_tree_extended(const char *ms if (encoding_is_utf8 && !verify_utf8(&buffer)) fprintf(stderr, _(commit_utf8_warn)); - if (sign_commit && do_sign_commit(&buffer, sign_commit)) - return -1; + if (sign_commit && do_sign_commit(&buffer, sign_commit)) { + result = -1; + goto out; + } result = write_sha1_file(buffer.buf, buffer.len, commit_type, ret); + out: strbuf_release(&buffer); return result; } diff --combined convert.c index 02962261c5,4a0ed8d3cb..a09935cb81 --- a/convert.c +++ b/convert.c @@@ -423,8 -423,10 +423,10 @@@ static int filter_buffer_or_fd(int in, child_process.in = -1; child_process.out = out; - if (start_command(&child_process)) + if (start_command(&child_process)) { + strbuf_release(&cmd); return error("cannot fork to run external filter '%s'", params->cmd); + } sigchain_push(SIGPIPE, SIG_IGN); @@@ -1040,6 -1042,7 +1042,6 @@@ static void convert_attrs(struct conv_a ca->crlf_action = git_path_check_crlf(ccheck + 4); if (ca->crlf_action == CRLF_UNDEFINED) ca->crlf_action = git_path_check_crlf(ccheck + 0); - ca->attr_action = ca->crlf_action; ca->ident = git_path_check_ident(ccheck + 1); ca->drv = git_path_check_convert(ccheck + 2); if (ca->crlf_action != CRLF_BINARY) { @@@ -1053,14 -1056,12 +1055,14 @@@ else if (eol_attr == EOL_CRLF) ca->crlf_action = CRLF_TEXT_CRLF; } - ca->attr_action = ca->crlf_action; } else { ca->drv = NULL; ca->crlf_action = CRLF_UNDEFINED; ca->ident = 0; } + + /* Save attr and make a decision for action */ + ca->attr_action = ca->crlf_action; if (ca->crlf_action == CRLF_TEXT) ca->crlf_action = text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT; if (ca->crlf_action == CRLF_UNDEFINED && auto_crlf == AUTO_CRLF_FALSE) diff --combined diff.c index 7df2227a3c,64cdcf2331..ea7e5978bc --- a/diff.c +++ b/diff.c @@@ -459,7 -459,7 +459,7 @@@ static struct diff_tempfile * If this diff_tempfile instance refers to a temporary file, * this tempfile object is used to manage its lifetime. */ - struct tempfile tempfile; + struct tempfile *tempfile; } diff_temp[2]; struct emit_callback { @@@ -1414,7 -1414,7 +1414,7 @@@ static void remove_tempfile(void { int i; for (i = 0; i < ARRAY_SIZE(diff_temp); i++) { - if (is_tempfile_active(&diff_temp[i].tempfile)) + if (is_tempfile_active(diff_temp[i].tempfile)) delete_tempfile(&diff_temp[i].tempfile); diff_temp[i].name = NULL; } @@@ -2583,6 -2583,7 +2583,7 @@@ static void show_stats(struct diffstat_ } print_stat_summary_inserts_deletes(options, total_files, adds, dels); + strbuf_release(&out); } static void show_shortstats(struct diffstat_t *data, struct diff_options *options) @@@ -3720,6 -3721,7 +3721,6 @@@ static void prep_temp_blob(const char * const struct object_id *oid, int mode) { - int fd; struct strbuf buf = STRBUF_INIT; struct strbuf template = STRBUF_INIT; char *path_dup = xstrdup(path); @@@ -3729,18 -3731,18 +3730,18 @@@ strbuf_addstr(&template, "XXXXXX_"); strbuf_addstr(&template, base); - fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1); - if (fd < 0) + temp->tempfile = mks_tempfile_ts(template.buf, strlen(base) + 1); + if (!temp->tempfile) die_errno("unable to create temp-file"); if (convert_to_working_tree(path, (const char *)blob, (size_t)size, &buf)) { blob = buf.buf; size = buf.len; } - if (write_in_full(fd, blob, size) != size) + if (write_in_full(temp->tempfile->fd, blob, size) != size || + close_tempfile_gently(temp->tempfile)) die_errno("unable to write temp-file"); - close_tempfile(&temp->tempfile); - temp->name = get_tempfile_path(&temp->tempfile); + temp->name = get_tempfile_path(temp->tempfile); oid_to_hex_r(temp->hex, oid); xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode); strbuf_release(&buf); @@@ -5288,6 -5290,7 +5289,7 @@@ static void show_rename_copy(struct dif emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY, sb.buf, sb.len, 0); show_mode_change(opt, p, 0); + strbuf_release(&sb); } static void diff_summary(struct diff_options *opt, struct diff_filepair *p) @@@ -5313,6 -5316,7 +5315,7 @@@ strbuf_addf(&sb, " (%d%%)\n", similarity_index(p)); emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY, sb.buf, sb.len, 0); + strbuf_release(&sb); } show_mode_change(opt, p, !p->score); break; diff --combined refs.c index a2b97fba78,d8dc86b1f5..c30f4c36be --- a/refs.c +++ b/refs.c @@@ -336,6 -336,12 +336,6 @@@ int for_each_tag_ref(each_ref_fn fn, vo return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data); } -int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) -{ - return refs_for_each_tag_ref(get_submodule_ref_store(submodule), - fn, cb_data); -} - int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data); @@@ -346,6 -352,12 +346,6 @@@ int for_each_branch_ref(each_ref_fn fn return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data); } -int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) -{ - return refs_for_each_branch_ref(get_submodule_ref_store(submodule), - fn, cb_data); -} - int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data); @@@ -356,6 -368,12 +356,6 @@@ int for_each_remote_ref(each_ref_fn fn return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data); } -int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) -{ - return refs_for_each_remote_ref(get_submodule_ref_store(submodule), - fn, cb_data); -} - int head_ref_namespaced(each_ref_fn fn, void *cb_data) { struct strbuf buf = STRBUF_INIT; @@@ -594,7 -612,7 +594,7 @@@ static int write_pseudoref(const char * if (fd < 0) { strbuf_addf(err, "could not open '%s' for writing: %s", filename, strerror(errno)); - return -1; + goto done; } if (old_sha1) { @@@ -1248,13 -1266,19 +1248,13 @@@ int refs_rename_ref_available(struct re return ok; } -int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) +int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { struct object_id oid; int flag; - if (submodule) { - if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0) - return fn("HEAD", &oid, 0, cb_data); - - return 0; - } - - if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag)) + if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING, + oid.hash, &flag)) return fn("HEAD", &oid, flag, cb_data); return 0; @@@ -1262,7 -1286,7 +1262,7 @@@ int head_ref(each_ref_fn fn, void *cb_data) { - return head_ref_submodule(NULL, fn, cb_data); + return refs_head_ref(get_main_ref_store(), fn, cb_data); } struct ref_iterator *refs_ref_iterator_begin( @@@ -1320,6 -1344,11 +1320,6 @@@ int for_each_ref(each_ref_fn fn, void * return refs_for_each_ref(get_main_ref_store(), fn, cb_data); } -int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data) -{ - return refs_for_each_ref(get_submodule_ref_store(submodule), fn, cb_data); -} - int refs_for_each_ref_in(struct ref_store *refs, const char *prefix, each_ref_fn fn, void *cb_data) { @@@ -1341,15 -1370,23 +1341,15 @@@ int for_each_fullref_in(const char *pre prefix, fn, 0, flag, cb_data); } -int for_each_ref_in_submodule(const char *submodule, const char *prefix, - each_ref_fn fn, void *cb_data) -{ - return refs_for_each_ref_in(get_submodule_ref_store(submodule), - prefix, fn, cb_data); -} - -int for_each_fullref_in_submodule(const char *submodule, const char *prefix, - each_ref_fn fn, void *cb_data, - unsigned int broken) +int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, + each_ref_fn fn, void *cb_data, + unsigned int broken) { unsigned int flag = 0; if (broken) flag = DO_FOR_EACH_INCLUDE_BROKEN; - return do_for_each_ref(get_submodule_ref_store(submodule), - prefix, fn, 0, flag, cb_data); + return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data); } int for_each_replace_ref(each_ref_fn fn, void *cb_data) @@@ -1484,10 -1521,25 +1484,10 @@@ const char *resolve_ref_unsafe(const ch int resolve_gitlink_ref(const char *submodule, const char *refname, unsigned char *sha1) { - size_t len = strlen(submodule); struct ref_store *refs; int flags; - while (len && submodule[len - 1] == '/') - len--; - - if (!len) - return -1; - - if (submodule[len]) { - /* We need to strip off one or more trailing slashes */ - char *stripped = xmemdupz(submodule, len); - - refs = get_submodule_ref_store(stripped); - free(stripped); - } else { - refs = get_submodule_ref_store(submodule); - } + refs = get_submodule_ref_store(submodule); if (!refs) return -1; @@@ -1602,32 -1654,31 +1602,32 @@@ struct ref_store *get_submodule_ref_sto { struct strbuf submodule_sb = STRBUF_INIT; struct ref_store *refs; - int ret; + char *to_free = NULL; + size_t len; - if (!submodule || !*submodule) { - /* - * FIXME: This case is ideally not allowed. But that - * can't happen until we clean up all the callers. - */ - return get_main_ref_store(); - } + if (!submodule) + return NULL; + + len = strlen(submodule); + while (len && is_dir_sep(submodule[len - 1])) + len--; + if (!len) + return NULL; + + if (submodule[len]) + /* We need to strip off one or more trailing slashes */ + submodule = to_free = xmemdupz(submodule, len); refs = lookup_ref_store_map(&submodule_ref_stores, submodule); if (refs) - return refs; + goto done; strbuf_addstr(&submodule_sb, submodule); - ret = is_nonbare_repository_dir(&submodule_sb); - strbuf_release(&submodule_sb); - if (!ret) - return NULL; + if (!is_nonbare_repository_dir(&submodule_sb)) + goto done; - ret = submodule_to_gitdir(&submodule_sb, submodule); - if (ret) { - strbuf_release(&submodule_sb); - return NULL; - } + if (submodule_to_gitdir(&submodule_sb, submodule)) + goto done; /* assume that add_submodule_odb() has been called */ refs = ref_store_init(submodule_sb.buf, @@@ -1635,10 -1686,7 +1635,10 @@@ register_ref_store_map(&submodule_ref_stores, "submodule", refs, submodule); +done: strbuf_release(&submodule_sb); + free(to_free); + return refs; } diff --combined sha1_file.c index 5f71bbac3e,7d9c9aed2f..b4a67bb838 --- a/sha1_file.c +++ b/sha1_file.c @@@ -30,7 -30,7 +30,7 @@@ #include "quote.h" #include "packfile.h" -const unsigned char null_sha1[20]; +const unsigned char null_sha1[GIT_MAX_RAWSZ]; const struct object_id null_oid; const struct object_id empty_tree_oid = { EMPTY_TREE_SHA1_BIN_LITERAL @@@ -1820,6 -1820,7 +1820,7 @@@ int index_path(struct object_id *oid, c { int fd; struct strbuf sb = STRBUF_INIT; + int rc = 0; switch (st->st_mode & S_IFMT) { case S_IFREG: @@@ -1836,8 -1837,7 +1837,7 @@@ if (!(flags & HASH_WRITE_OBJECT)) hash_sha1_file(sb.buf, sb.len, blob_type, oid->hash); else if (write_sha1_file(sb.buf, sb.len, blob_type, oid->hash)) - return error("%s: failed to insert into database", - path); + rc = error("%s: failed to insert into database", path); strbuf_release(&sb); break; case S_IFDIR: @@@ -1845,7 -1845,7 +1845,7 @@@ default: return error("%s: unsupported file type", path); } - return 0; + return rc; } int read_pack_header(int fd, struct pack_header *header) diff --combined transport-helper.c index f50b34df2d,519a244583..42b960ff86 --- a/transport-helper.c +++ b/transport-helper.c @@@ -604,6 -604,7 +604,7 @@@ static int process_connect_service(stru cmdbuf.buf); exit: + strbuf_release(&cmdbuf); fclose(input); return ret; } @@@ -1117,13 -1118,6 +1118,13 @@@ int transport_helper_init(struct transp __attribute__((format (printf, 1, 2))) static void transfer_debug(const char *fmt, ...) { + /* + * NEEDSWORK: This function is sometimes used from multiple threads, and + * we end up using debug_enabled racily. That "should not matter" since + * we always write the same value, but it's still wrong. This function + * is listed in .tsan-suppressions for the time being. + */ + va_list args; char msgbuf[PBUFFERSIZE]; static int debug_enabled = -1;