From 5b34ba414d105a8219e55babd0780a935a0c0c20 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:45 +0200 Subject: get_mail_commit_oid(): avoid resource leak When we fail to read, or parse, the file, we still want to close the file descriptor and release the strbuf. Reported via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/am.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) (limited to 'builtin') diff --git a/builtin/am.c b/builtin/am.c index a95dd8b4e6..9c5c2c778e 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1351,19 +1351,16 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail) struct strbuf sb = STRBUF_INIT; FILE *fp = xfopen(mail, "r"); const char *x; + int ret = 0; - if (strbuf_getline_lf(&sb, fp)) - return -1; - - if (!skip_prefix(sb.buf, "From ", &x)) - return -1; - - if (get_oid_hex(x, commit_id) < 0) - return -1; + if (strbuf_getline_lf(&sb, fp) || + !skip_prefix(sb.buf, "From ", &x) || + get_oid_hex(x, commit_id) < 0) + ret = -1; strbuf_release(&sb); fclose(fp); - return 0; + return ret; } /** -- cgit v1.2.3 From 5f3296c069bfe2872592f96b29bdcc8f0edd5eb8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:48 +0200 Subject: difftool: address a couple of resource/memory leaks This change plugs a couple of memory leaks and makes sure that the file descriptor is closed in run_dir_diff(). Spotted by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/difftool.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) (limited to 'builtin') diff --git a/builtin/difftool.c b/builtin/difftool.c index 1354d0e462..b9a892f269 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const char *index_path, hashmap_entry_init(entry, strhash(buf.buf)); hashmap_add(result, entry); } + fclose(fp); if (finish_command(&diff_files)) die("diff-files did not exit properly"); strbuf_release(&index_env); @@ -439,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } if (lmode && status != 'C') { - if (checkout_path(lmode, &loid, src_path, &lstate)) - return error("could not write '%s'", src_path); + if (checkout_path(lmode, &loid, src_path, &lstate)) { + ret = error("could not write '%s'", src_path); + goto finish; + } } if (rmode && !S_ISLNK(rmode)) { @@ -456,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, hashmap_add(&working_tree_dups, entry); if (!use_wt_file(workdir, dst_path, &roid)) { - if (checkout_path(rmode, &roid, dst_path, &rstate)) - return error("could not write '%s'", - dst_path); + if (checkout_path(rmode, &roid, dst_path, + &rstate)) { + ret = error("could not write '%s'", + dst_path); + goto finish; + } } else if (!is_null_oid(&roid)) { /* * Changes in the working tree need special @@ -473,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, ADD_CACHE_JUST_APPEND); add_path(&rdir, rdir_len, dst_path); - if (ensure_leading_directories(rdir.buf)) - return error("could not create " - "directory for '%s'", - dst_path); + if (ensure_leading_directories(rdir.buf)) { + ret = error("could not create " + "directory for '%s'", + dst_path); + goto finish; + } add_path(&wtdir, wtdir_len, dst_path); if (symlinks) { if (symlink(wtdir.buf, rdir.buf)) { @@ -497,13 +505,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } } + fclose(fp); + fp = NULL; if (finish_command(&child)) { ret = error("error occurred running diff --raw"); goto finish; } if (!i) - return 0; + goto finish; /* * Changes to submodules require special treatment.This loop writes a @@ -626,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, exit_cleanup(tmpdir, rc); finish: + if (fp) + fclose(fp); + free(lbase_dir); free(rbase_dir); strbuf_release(&ldir); -- cgit v1.2.3 From f0733c13ed8b79bb10e240c4b4a6630784c7d258 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:14 +0200 Subject: mailinfo & mailsplit: check for EOF while parsing While POSIX states that it is okay to pass EOF to isspace() (and it seems to be implied that EOF should *not* be treated as whitespace), and also to pass EOF to ungetc() (which seems to be intended to fail without buffering the character), it is much better to handle these cases explicitly. Not only does it reduce head-scratching (and helps static analysis avoid reporting false positives), it also lets us handle files containing nothing but whitespace by erroring out. Reported via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/mailsplit.c | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'builtin') diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index 30681681c1..664400b816 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -232,6 +232,16 @@ static int split_mbox(const char *file, const char *dir, int allow_bare, do { peek = fgetc(f); + if (peek == EOF) { + if (f == stdin) + /* empty stdin is OK */ + ret = skip; + else { + fclose(f); + error(_("empty mbox: '%s'"), file); + } + goto out; + } } while (isspace(peek)); ungetc(peek, f); -- cgit v1.2.3 From 05c2b7ba49e36a6beff30204948dba79451c4233 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:17 +0200 Subject: cat-file: fix memory leak Discovered by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/cat-file.c | 1 + 1 file changed, 1 insertion(+) (limited to 'builtin') diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 1890d7a639..9af863e791 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, die("git cat-file %s: bad file", obj_name); write_or_die(1, buf, size); + free(buf); return 0; } -- cgit v1.2.3 From 514e8039444565f7806e065579090167db5e489c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:40 +0200 Subject: checkout: fix memory leak This change addresses part of the NEEDSWORK comment above the code, therefore the comment needs to be adjusted, too. Discovered via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/checkout.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'builtin') diff --git a/builtin/checkout.c b/builtin/checkout.c index bfa5419f33..5faea3a05f 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout *state) /* * NEEDSWORK: * There is absolutely no reason to write this as a blob object - * and create a phony cache entry just to leak. This hack is - * primarily to get to the write_entry() machinery that massages - * the contents to work-tree format and writes out which only - * allows it for a cache entry. The code in write_entry() needs - * to be refactored to allow us to feed a - * instead of a cache entry. Such a refactoring would help - * merge_recursive as well (it also writes the merge result to the - * object database even when it may contain conflicts). + * and create a phony cache entry. This hack is primarily to get + * to the write_entry() machinery that massages the contents to + * work-tree format and writes out which only allows it for a + * cache entry. The code in write_entry() needs to be refactored + * to allow us to feed a instead of a cache + * entry. Such a refactoring would help merge_recursive as well + * (it also writes the merge result to the object database even + * when it may contain conflicts). */ if (write_sha1_file(result_buf.ptr, result_buf.size, blob_type, oid.hash)) @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout *state) if (!ce) die(_("make_cache_entry failed for path '%s'"), path); status = checkout_entry(ce, state, NULL); + free(ce); return status; } -- cgit v1.2.3 From 077a34e0b9401f9bcea991d850f1111a70e7d3d1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:54 +0200 Subject: pack-redundant: plug memory leak Identified via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/pack-redundant.c | 1 + 1 file changed, 1 insertion(+) (limited to 'builtin') diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index 72c815844d..cb1df1c761 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -442,6 +442,7 @@ static void minimize(struct pack_list **min) /* return if there are no objects missing from the unique set */ if (missing->size == 0) { *min = unique; + free(missing); return; } -- cgit v1.2.3 From 43e61e715283ba3e6cfe5ebf9e831e1d96cec399 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:57:28 +0200 Subject: mktree: plug memory leaks reported by Coverity Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/mktree.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'builtin') diff --git a/builtin/mktree.c b/builtin/mktree.c index de9b40fc63..da0fd8cd70 100644 --- a/builtin/mktree.c +++ b/builtin/mktree.c @@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss unsigned mode; enum object_type mode_type; /* object type derived from mode */ enum object_type obj_type; /* object type derived from sha */ - char *path; + char *path, *to_free = NULL; unsigned char sha1[20]; ptr = buf; @@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss struct strbuf p_uq = STRBUF_INIT; if (unquote_c_style(&p_uq, path, NULL)) die("invalid quoting"); - path = strbuf_detach(&p_uq, NULL); + path = to_free = strbuf_detach(&p_uq, NULL); } /* @@ -136,6 +136,7 @@ static void mktree_line(char *buf, size_t len, int nul_term_line, int allow_miss } append_to_tree(mode, sha1, path); + free(to_free); } int cmd_mktree(int ac, const char **av, const char *prefix) -- cgit v1.2.3 From 1efb1e9a21d16efddd1440bd2684bf19dc9546e0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:57:33 +0200 Subject: fast-export: avoid leaking memory in handle_tag() Reported by, you guessed it, Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/fast-export.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'builtin') diff --git a/builtin/fast-export.c b/builtin/fast-export.c index e0220630d0..64617ad8e3 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag) oid_to_hex(&tag->object.oid)); case DROP: /* Ignore this tag altogether */ + free(buf); return; case REWRITE: if (tagged->type != OBJ_COMMIT) { @@ -765,6 +766,7 @@ static void handle_tag(const char *name, struct tag *tag) (int)(tagger_end - tagger), tagger, tagger == tagger_end ? "" : "\n", (int)message_size, (int)message_size, message ? message : ""); + free(buf); } static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name) -- cgit v1.2.3 From bda6e82801e3c01d51634e6c722783295fd079ae Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:57:55 +0200 Subject: receive-pack: plug memory leak in update() Reported via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index f96834f42c..48c07ddb65 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -986,7 +986,8 @@ static const char *update(struct command *cmd, struct shallow_info *si) { const char *name = cmd->ref_name; struct strbuf namespaced_name_buf = STRBUF_INIT; - const char *namespaced_name, *ret; + static char *namespaced_name; + const char *ret; struct object_id *old_oid = &cmd->old_oid; struct object_id *new_oid = &cmd->new_oid; @@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) } strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name); + free(namespaced_name); namespaced_name = strbuf_detach(&namespaced_name_buf, NULL); if (is_ref_checked_out(namespaced_name)) { -- cgit v1.2.3 From 5308224633cf138f436357b2a8a87a546373af72 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:59:06 +0200 Subject: name-rev: avoid leaking memory in the `deref` case When the `name_rev()` function is asked to dereference the tip name, it allocates memory. But when it turns out that another tip already described the commit better than the current one, we forgot to release the memory. Pointed out by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/name-rev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'builtin') diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 92a5d8a5d2..e7a3fe7ee7 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit, struct rev_name *name = (struct rev_name *)commit->util; struct commit_list *parents; int parent_number = 1; + char *to_free = NULL; parse_commit(commit); @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit, return; if (deref) { - tip_name = xstrfmt("%s^0", tip_name); + tip_name = to_free = xstrfmt("%s^0", tip_name); if (generation) die("generation: %d, but deref?", generation); @@ -53,8 +54,10 @@ copy_data: name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; - } else + } else { + free(to_free); return; + } for (parents = commit->parents; parents; -- cgit v1.2.3 From 2e11f58fa6a0c904d9b00e51cfb2d8a3ee77b360 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:59:13 +0200 Subject: show_worktree(): plug memory leak The buffer allocated by shorten_unambiguous_ref() needs to be released. Discovered by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/worktree.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'builtin') diff --git a/builtin/worktree.c b/builtin/worktree.c index 1722a9bdc2..ff5dfd2b10 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV)); if (wt->is_detached) strbuf_addstr(&sb, "(detached HEAD)"); - else if (wt->head_ref) - strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0)); - else + else if (wt->head_ref) { + char *ref = shorten_unambiguous_ref(wt->head_ref, 0); + strbuf_addf(&sb, "[%s]", ref); + free(ref); + } else strbuf_addstr(&sb, "(error)"); } printf("%s\n", sb.buf); -- cgit v1.2.3 From 443a12f37be1c5967785b83bf04935fe357afb9b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 8 May 2017 13:21:06 +0900 Subject: checkout: fix memory leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When "git checkout -m" does an in-core three-way merge to carry local modifications forward to check out a different branch, the code forgot to free the updated contents it has in-core. Noticed-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/checkout.c | 1 + 1 file changed, 1 insertion(+) (limited to 'builtin') diff --git a/builtin/checkout.c b/builtin/checkout.c index 5faea3a05f..94849cf250 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -247,6 +247,7 @@ static int checkout_merged(int pos, const struct checkout *state) if (write_sha1_file(result_buf.ptr, result_buf.size, blob_type, oid.hash)) die(_("Unable to add merge result for '%s'"), path); + free(result_buf.ptr); ce = make_cache_entry(mode, oid.hash, path, 2, 0); if (!ce) die(_("make_cache_entry failed for path '%s'"), path); -- cgit v1.2.3