From 1a845a2c88e5b72acce33c9719ec36fcd0f9c877 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:24 +0200 Subject: [PATCH 01/26] mingw: avoid memory leak when splitting PATH In the (admittedly, concocted) case that PATH consists only of path delimiters, we would leak the duplicated string. Reported by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/mingw.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index 3fbfda5978..fe0e3ccd24 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -961,8 +961,10 @@ static char **get_path_split(void) ++n; } } - if (!n) + if (!n) { + free(envpath); return NULL; + } ALLOC_ARRAY(path, n + 1); p = envpath; From 3057da5ad51b3256a6f6b94d034e6a5571ce5ece Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:29 +0200 Subject: [PATCH 02/26] winansi: avoid use of uninitialized value To initialize the foreground color attributes of "plain text", our ANSI emulation tries to infer them from the currently attached console while running the is_console() function. This function first tries to detect any console attached to stdout, then it is called with stderr. If neither stdout nor stderr has any console attached, it does not actually matter what we use for "plain text" attributes, as we never need to output any text to any console in that case. However, after working on stdout and stderr, is_console() is called with stdin, and it still tries to initialize the "plain text" attributes if they had not been initialized earlier. In this case, we cannot detect any attributes, and we used an uninitialized value for them. Naturally, Coverity complained about this use case because it could not reason about the code deeply enough to figure out that we do not even use those attributes in that case. Let's just initialize the value to 0 in that case, both to avoid future Coverity reports, and to help catch future regressions in case anybody changes the order of the is_console() calls (which would make the text black on black). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/winansi.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/compat/winansi.c b/compat/winansi.c index 793420f9d0..a551de90eb 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -105,6 +105,13 @@ static int is_console(int fd) if (!fd) { if (!GetConsoleMode(hcon, &mode)) return 0; + /* + * This code path is only reached if there is no console + * attached to stdout/stderr, i.e. we will not need to output + * any text to any console, therefore we might just as well + * use black as foreground color. + */ + sbi.wAttributes = 0; } else if (!GetConsoleScreenBufferInfo(hcon, &sbi)) return 0; From b6b066adf9e1e970a6d8295db630ab1e1f3bc71c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:34 +0200 Subject: [PATCH 03/26] winansi: avoid buffer overrun When we could not convert the UTF-8 sequence into Unicode for writing to the Console, we should not try to write an insanely-long sequence of invalid wide characters (mistaking the negative return value for an unsigned length). Reported by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/winansi.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/compat/winansi.c b/compat/winansi.c index a551de90eb..a11a0f16d2 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -140,6 +140,11 @@ static void write_console(unsigned char *str, size_t len) /* convert utf-8 to utf-16 */ int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len); + if (wlen < 0) { + wchar_t *err = L"[invalid]"; + WriteConsoleW(console, err, wcslen(err), &dummy, NULL); + return; + } /* write directly to console */ WriteConsoleW(console, wbuf, wlen, &dummy, NULL); From 5748693b9123f55363211d18e8074db5e1b8384e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:38 +0200 Subject: [PATCH 04/26] add_commit_patch_id(): avoid allocating memory unnecessarily It would appear that we allocate (and forget to release) memory if the patch ID is not even defined. Reported by the Coverity tool. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- patch-ids.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/patch-ids.c b/patch-ids.c index fa8f11de82..92eba7a059 100644 --- a/patch-ids.c +++ b/patch-ids.c @@ -99,11 +99,12 @@ struct patch_id *has_commit_patch_id(struct commit *commit, struct patch_id *add_commit_patch_id(struct commit *commit, struct patch_ids *ids) { - struct patch_id *key = xcalloc(1, sizeof(*key)); + struct patch_id *key; if (!patch_id_defined(commit)) return NULL; + key = xcalloc(1, sizeof(*key)); if (init_patch_id_entry(key, commit, ids)) { free(key); return NULL; From 4db7dbdb4ae5426ab133b3844ea2a6889c6897d7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:41 +0200 Subject: [PATCH 05/26] git_config_rename_section_in_file(): avoid resource leak In case of errors, we really want the file descriptor to be closed. Discovered by a Coverity scan. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- config.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index b4a3205da3..a30056ec7e 100644 --- a/config.c +++ b/config.c @@ -2621,7 +2621,7 @@ int git_config_rename_section_in_file(const char *config_filename, struct lock_file *lock; int out_fd; char buf[1024]; - FILE *config_file; + FILE *config_file = NULL; struct stat st; if (new_name && !section_name_is_ok(new_name)) { @@ -2703,11 +2703,14 @@ int git_config_rename_section_in_file(const char *config_filename, } } fclose(config_file); + config_file = NULL; commit_and_out: if (commit_lock_file(lock) < 0) ret = error_errno("could not write config file %s", config_filename); out: + if (config_file) + fclose(config_file); rollback_lock_file(lock); out_no_rollback: free(filename_buf); From 5b34ba414d105a8219e55babd0780a935a0c0c20 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:45 +0200 Subject: [PATCH 06/26] 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(-) 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; } /** From 5f3296c069bfe2872592f96b29bdcc8f0edd5eb8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:48 +0200 Subject: [PATCH 07/26] 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(-) 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); From e7b65e205af88755567c3283ee29ca9fb9af11a9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:55:52 +0200 Subject: [PATCH 08/26] status: close file descriptor after reading git-rebase-todo Reported via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- wt-status.c | 1 + 1 file changed, 1 insertion(+) diff --git a/wt-status.c b/wt-status.c index 0375484962..0a6e16dbe0 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1168,6 +1168,7 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines) abbrev_sha1_in_line(&line); string_list_append(lines, line.buf); } + fclose(f); return 0; } From f0733c13ed8b79bb10e240c4b4a6630784c7d258 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:14 +0200 Subject: [PATCH 09/26] 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 ++++++++++ mailinfo.c | 9 ++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) 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); diff --git a/mailinfo.c b/mailinfo.c index 68037758f2..f92cb9f729 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE *in) for (;;) { int peek; - peek = fgetc(in); ungetc(peek, in); + peek = fgetc(in); + if (peek == EOF) + break; + ungetc(peek, in); if (peek != ' ' && peek != '\t') break; if (strbuf_getline_lf(&continuation, in)) @@ -1099,6 +1102,10 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) do { peek = fgetc(mi->input); + if (peek == EOF) { + fclose(cmitmsg); + return error("empty patch: '%s'", patch); + } } while (isspace(peek)); ungetc(peek, mi->input); From 05c2b7ba49e36a6beff30204948dba79451c4233 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:17 +0200 Subject: [PATCH 10/26] 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(+) 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; } From 514e8039444565f7806e065579090167db5e489c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:40 +0200 Subject: [PATCH 11/26] 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(-) 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; } From 41fc6b33fc0cef748f84906810bb36fcd491f0a1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:44 +0200 Subject: [PATCH 12/26] split_commit_in_progress(): simplify & fix memory leak This function did a whole lot of unnecessary work, such as reading in four files just to figure out that, oh, hey, we do not need to look at them after all because the HEAD is not detached. Simplify the entire function to return early when possible, to read in the files only when necessary, and to release the allocated memory always (there was a leak, reported via Coverity, where we failed to release the allocated strings if the HEAD is not detached). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- wt-status.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/wt-status.c b/wt-status.c index 0a6e16dbe0..117ac8cfb0 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1082,29 +1082,29 @@ static char *read_line_from_git_path(const char *filename) static int split_commit_in_progress(struct wt_status *s) { int split_in_progress = 0; - char *head = read_line_from_git_path("HEAD"); - char *orig_head = read_line_from_git_path("ORIG_HEAD"); - char *rebase_amend = read_line_from_git_path("rebase-merge/amend"); - char *rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head"); + char *head, *orig_head, *rebase_amend, *rebase_orig_head; - if (!head || !orig_head || !rebase_amend || !rebase_orig_head || + if ((!s->amend && !s->nowarn && !s->workdir_dirty) || !s->branch || strcmp(s->branch, "HEAD")) - return split_in_progress; + return 0; - if (!strcmp(rebase_amend, rebase_orig_head)) { - if (strcmp(head, rebase_amend)) - split_in_progress = 1; - } else if (strcmp(orig_head, rebase_orig_head)) { + head = read_line_from_git_path("HEAD"); + orig_head = read_line_from_git_path("ORIG_HEAD"); + rebase_amend = read_line_from_git_path("rebase-merge/amend"); + rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head"); + + if (!head || !orig_head || !rebase_amend || !rebase_orig_head) + ; /* fall through, no split in progress */ + else if (!strcmp(rebase_amend, rebase_orig_head)) + split_in_progress = !!strcmp(head, rebase_amend); + else if (strcmp(orig_head, rebase_orig_head)) split_in_progress = 1; - } - - if (!s->amend && !s->nowarn && !s->workdir_dirty) - split_in_progress = 0; free(head); free(orig_head); free(rebase_amend); free(rebase_orig_head); + return split_in_progress; } From da6f847559d8794f25da9f7c090929baebce7751 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:47 +0200 Subject: [PATCH 13/26] setup_bare_git_dir(): help static analysis Coverity reported a memory leak in this function. However, it can only be called once, as setup_git_directory() changes global state and hence is not reentrant. Mark the variable as static to indicate that this is a singleton. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.c b/setup.c index 0309c27821..0320a9ad14 100644 --- a/setup.c +++ b/setup.c @@ -748,7 +748,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset, /* --work-tree is set without --git-dir; use discovered one */ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) { - const char *gitdir; + static const char *gitdir; gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset); if (chdir(cwd->buf)) From 2d4dcf210e156153a9c1b11bc60d647d5a327624 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:51 +0200 Subject: [PATCH 14/26] setup_discovered_git_dir(): plug memory leak The setup_explicit_git_dir() function does not take custody of the string passed as first parameter; we have to release it if we turned the value of git_dir into an absolute path. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- setup.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/setup.c b/setup.c index 0320a9ad14..e3f7699a90 100644 --- a/setup.c +++ b/setup.c @@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char *gitdir, /* --work-tree is set without --git-dir; use discovered one */ if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) { + char *to_free = NULL; + const char *ret; + if (offset != cwd->len && !is_absolute_path(gitdir)) - gitdir = real_pathdup(gitdir, 1); + gitdir = to_free = real_pathdup(gitdir, 1); if (chdir(cwd->buf)) die_errno("Could not come back to cwd"); - return setup_explicit_git_dir(gitdir, cwd, nongit_ok); + ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok); + free(to_free); + return ret; } /* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */ From 077a34e0b9401f9bcea991d850f1111a70e7d3d1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:56:54 +0200 Subject: [PATCH 15/26] 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(+) 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; } From 43e61e715283ba3e6cfe5ebf9e831e1d96cec399 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:57:28 +0200 Subject: [PATCH 16/26] 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(-) 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) From 1efb1e9a21d16efddd1440bd2684bf19dc9546e0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:57:33 +0200 Subject: [PATCH 17/26] 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(+) 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) From bda6e82801e3c01d51634e6c722783295fd079ae Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:57:55 +0200 Subject: [PATCH 18/26] 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(-) 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)) { From 6cf034e7ed3ec0692535416d9d339b60a830bf23 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:58:01 +0200 Subject: [PATCH 19/26] line-log: avoid memory leak Discovered by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- line-log.c | 1 + 1 file changed, 1 insertion(+) diff --git a/line-log.c b/line-log.c index a23b910471..b9087814b8 100644 --- a/line-log.c +++ b/line-log.c @@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c changed = process_all_files(&parent_range, rev, &queue, range); if (parent) add_line_range(rev, parent, parent_range); + free_line_log_data(parent_range); return changed; } From 7c565a6b2d8bf7fe989c85dc75df7fabc8113f40 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:58:35 +0200 Subject: [PATCH 20/26] shallow: avoid memory leak Reported by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- shallow.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/shallow.c b/shallow.c index 25b6db989b..f9370961f9 100644 --- a/shallow.c +++ b/shallow.c @@ -473,11 +473,15 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1, struct commit_list *head = NULL; int bitmap_nr = (info->nr_bits + 31) / 32; size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr); - uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */ - uint32_t *bitmap = paint_alloc(info); struct commit *c = lookup_commit_reference_gently(sha1, 1); + uint32_t *tmp; /* to be freed before return */ + uint32_t *bitmap; + if (!c) return; + + tmp = xmalloc(bitmap_size); + bitmap = paint_alloc(info); memset(bitmap, 0, bitmap_size); bitmap[id / 32] |= (1U << (id % 32)); commit_list_insert(c, &head); From 5026b471751092ab971f3d4ae46320bc8ce40ff5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:58:42 +0200 Subject: [PATCH 21/26] add_reflog_for_walk: avoid memory leak We free()d the `log` buffer when dwim_log() returned 1, but not when it returned a larger value (which meant that it still allocated the buffer but we simply ignored it). While in the vicinity, make sure that the `reflogs` structure as well as the `branch` variable are released properly, too. Identified by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- reflog-walk.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index 99679f5825..c63eb1a3fd 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info, if (!reflogs || reflogs->nr == 0) { struct object_id oid; char *b; - if (dwim_log(branch, strlen(branch), oid.hash, &b) == 1) { + int ret = dwim_log(branch, strlen(branch), + oid.hash, &b); + if (ret > 1) + free(b); + else if (ret == 1) { if (reflogs) { free(reflogs->ref); free(reflogs); @@ -193,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info, reflogs = read_complete_reflog(branch); } } - if (!reflogs || reflogs->nr == 0) + if (!reflogs || reflogs->nr == 0) { + if (reflogs) { + free(reflogs->ref); + free(reflogs); + } + free(branch); return -1; + } string_list_insert(&info->complete_reflogs, branch)->util = reflogs; } + free(branch); commit_reflog = xcalloc(1, sizeof(struct commit_reflog)); if (recno < 0) { commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp); if (commit_reflog->recno < 0) { - free(branch); + if (reflogs) { + free(reflogs->ref); + free(reflogs); + } free(commit_reflog); return -1; } From 3dc7ea91dab9959b6bec82c05df18d6d1bb70619 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:59:01 +0200 Subject: [PATCH 22/26] remote: plug memory leak in match_explicit() The `guess_ref()` returns an allocated buffer of which `make_linked_ref()` does not take custody (`alloc_ref()` makes a copy), therefore we need to release the buffer afterwards. Noticed via Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- remote.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/remote.c b/remote.c index 801137c72e..72b4591b98 100644 --- a/remote.c +++ b/remote.c @@ -1191,9 +1191,10 @@ static int match_explicit(struct ref *src, struct ref *dst, else if (is_null_oid(&matched_src->new_oid)) error("unable to delete '%s': remote ref does not exist", dst_value); - else if ((dst_guess = guess_ref(dst_value, matched_src))) + else if ((dst_guess = guess_ref(dst_value, matched_src))) { matched_dst = make_linked_ref(dst_guess, dst_tail); - else + free(dst_guess); + } else error("unable to push to unqualified destination: %s\n" "The destination refspec neither matches an " "existing ref on the remote nor\n" From 5308224633cf138f436357b2a8a87a546373af72 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:59:06 +0200 Subject: [PATCH 23/26] 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(-) 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 @@ static void name_rev(struct commit *commit, name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; - } else + } else { + free(to_free); return; + } for (parents = commit->parents; parents; From 2e11f58fa6a0c904d9b00e51cfb2d8a3ee77b360 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:59:13 +0200 Subject: [PATCH 24/26] 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(-) 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); From d32de66a07c600c14f70e6d435f8f9c496b0d625 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 May 2017 15:59:19 +0200 Subject: [PATCH 25/26] submodule_uses_worktrees(): plug memory leak There is really no reason why we would need to hold onto the allocated string longer than necessary. Reported by Coverity. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- worktree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/worktree.c b/worktree.c index bae787cf8d..89a81b13de 100644 --- a/worktree.c +++ b/worktree.c @@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path) /* The env would be set for the superproject. */ get_common_dir_noenv(&sb, submodule_gitdir); + free(submodule_gitdir); /* * The check below is only known to be good for repository format @@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path) /* See if there is any file inside the worktrees directory. */ dir = opendir(sb.buf); strbuf_release(&sb); - free(submodule_gitdir); if (!dir) return 0; From 443a12f37be1c5967785b83bf04935fe357afb9b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 8 May 2017 13:21:06 +0900 Subject: [PATCH 26/26] 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(+) 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);