I recently registered the git-for-windows fork with Coverity to ensure that even the Windows-specific patches get some static analysis love.
While at it, I squashed a couple of obvious issues in the part that is not Windows-specific. Note: while this patch series squashes some of those issues, the remaining issues are not necessarily all false positives (e.g. Coverity getting fooled by our FLEX_ARRAY trick into believing that the last member of the struct is indeed a 0 or 1 size array) or intentional (e.g. builtins not releasing memory because exit() is called right after returning from the function that leaks memory). Notable examples of the remaining issues are: - a couple of callers of shorten_unambiguous_ref() assume that they do not have to release the memory returned from that function, often assigning the pointer to a `const` variable that typically does not hold a pointer that needs to be free()d. My hunch is that we will want to convert that function to have a fixed number of static buffers and use those in a round robin fashion à la sha1_to_hex(). - pack-redundant.c seems to have hard-to-reason-about code paths that may eventually leak memory. Essentially, the custody of the allocated memory is not clear at all. - fast-import.c calls strbuf_detach() on the command_buf without any obvious rationale. Turns out that *some* code paths assign command_buf.buf to a `recent_command` which releases the buffer later. However, from a cursory look it appears to me as if there are some other code paths that skip that assignment, effectively causing a memory leak once strbuf_detach() is called. Sadly, I lack the time to pursue those remaining issues further. Changes since v1: - clarified in the commit messages for the setup_*git_dir() functions that we are not really plugging a memory leak, but marking singletons as `static` to help Coverity not to complain about this again - dropped the patch to http-backend, as it is supposedly a one-shot program using exit() as "garbage collector". - the difftool patch does a more thorough job of fixing leaks now - reworded the commit subject of the mailinfo/mailsplit patch to sport a prefix indicating the overall area of the fix - changed the mailinfo/mailsplit patch to *really* handle EOF correctly - simplified the patch to get_mail_commit_oid(), and now it even returns the correct error value! - adjusted the comment in builtin/checkout.c that talked about leaking the cache_entry (which is not leaked anymore) - add forgotten free(buf) in fast-export.c in an early return - line-log data is now released properly - a couple more memory leaks in add_reflog_for_walk() have been found and addressed (this one *really* needs a couple more eyeballs, though, it is just a much bigger change than I originally anticipated) Johannes Schindelin (25): mingw: avoid memory leak when splitting PATH winansi: avoid use of uninitialized value winansi: avoid buffer overrun add_commit_patch_id(): avoid allocating memory unnecessarily git_config_rename_section_in_file(): avoid resource leak get_mail_commit_oid(): avoid resource leak difftool: address a couple of resource/memory leaks status: close file descriptor after reading git-rebase-todo mailinfo & mailsplit: check for EOF while parsing cat-file: fix memory leak checkout: fix memory leak split_commit_in_progress(): fix memory leak setup_bare_git_dir(): help static analysis setup_discovered_git_dir(): help static analysis pack-redundant: plug memory leak mktree: plug memory leaks reported by Coverity fast-export: avoid leaking memory in handle_tag() receive-pack: plug memory leak in update() line-log: avoid memory leak shallow: avoid memory leak add_reflog_for_walk: avoid memory leak remote: plug memory leak in match_explicit() name-rev: avoid leaking memory in the `deref` case show_worktree(): plug memory leak submodule_uses_worktrees(): plug memory leak builtin/am.c | 15 ++++++--------- builtin/cat-file.c | 1 + builtin/checkout.c | 17 +++++++++-------- builtin/difftool.c | 33 +++++++++++++++++++++++---------- builtin/fast-export.c | 2 ++ builtin/mailsplit.c | 3 ++- builtin/mktree.c | 5 +++-- builtin/name-rev.c | 7 +++++-- builtin/pack-redundant.c | 1 + builtin/receive-pack.c | 4 +++- builtin/worktree.c | 8 +++++--- compat/mingw.c | 4 +++- compat/winansi.c | 7 +++++++ config.c | 5 ++++- line-log.c | 1 + mailinfo.c | 15 +++++++++++---- patch-ids.c | 3 ++- reflog-walk.c | 20 +++++++++++++++++--- remote.c | 5 +++-- setup.c | 11 ++++++++--- shallow.c | 8 ++++++-- worktree.c | 2 +- wt-status.c | 8 +++++++- 23 files changed, 130 insertions(+), 55 deletions(-) base-commit: 027a3b943b444a3e3a76f9a89803fc10245b858f Published-As: https://github.com/dscho/git/releases/tag/coverity-v2 Fetch-It-Via: git fetch https://github.com/dscho/git coverity-v2 Interdiff vs v1: diff --git a/builtin/am.c b/builtin/am.c index 067dd4fc20d..9c5c2c778e2 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1353,18 +1353,14 @@ static int get_mail_commit_oid(struct object_id *commit_id, const char *mail) const char *x; int ret = 0; - if (strbuf_getline_lf(&sb, fp)) - ret = -1; - - if (!ret && !skip_prefix(sb.buf, "From ", &x)) - ret = -1; - - if (!ret && get_oid_hex(x, commit_id) < 0) + 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; } /** diff --git a/builtin/checkout.c b/builtin/checkout.c index 98f98256608..5faea3a05fa 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 <buffer, size, mode> - * 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 <buffer, size, mode> 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)) diff --git a/builtin/difftool.c b/builtin/difftool.c index a4f1d117ef6..b9a892f2693 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -440,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)) { @@ -457,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 @@ -474,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)) { @@ -499,13 +506,14 @@ 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 @@ -628,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); diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 828d41c0c11..64617ad8e36 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) { diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index c0d88f97512..9b3efc8e987 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -232,8 +232,9 @@ static int split_mbox(const char *file, const char *dir, int allow_bare, do { peek = fgetc(f); - } while (peek >= 0 && isspace(peek)); - ungetc(peek, f); + } while (isspace(peek)); + if (peek != EOF) + ungetc(peek, f); if (strbuf_getwholeline(&buf, f, '\n')) { /* empty stdin is OK */ diff --git a/http-backend.c b/http-backend.c index d12572fda10..eef0a361f4f 100644 --- a/http-backend.c +++ b/http-backend.c @@ -681,10 +681,8 @@ int cmd_main(int argc, const char **argv) if (!regexec(&re, dir, 1, out, 0)) { size_t n; - if (strcmp(method, c->method)) { - free(dir); + if (strcmp(method, c->method)) return bad_request(&hdr, c); - } cmd = c; n = out[0].rm_eo - out[0].rm_so; @@ -710,7 +708,5 @@ int cmd_main(int argc, const char **argv) max_request_buffer); cmd->imp(&hdr, cmd_arg); - free(dir); - free(cmd_arg); return 0; } diff --git a/line-log.c b/line-log.c index 19d46e9ea2c..b9087814b8c 100644 --- a/line-log.c +++ b/line-log.c @@ -1125,7 +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(parent_range); + free_line_log_data(parent_range); return changed; } diff --git a/mailinfo.c b/mailinfo.c index 60dcad7b714..a319911b510 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)) @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch) return -1; } - mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data))); - mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data))); - do { peek = fgetc(mi->input); - } while (peek >= 0 && isspace(peek)); + if (peek == EOF) { + fclose(cmitmsg); + return error("empty patch: '%s'", patch); + } + } while (isspace(peek)); ungetc(peek, mi->input); + mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data))); + mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data))); + /* process the email header */ while (read_one_header_line(&line, mi->input)) check_header(mi, &line, mi->p_hdr_data, 1); diff --git a/reflog-walk.c b/reflog-walk.c index ec66f2b16e6..c63eb1a3fd7 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -197,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; } -- 2.12.2.windows.2.800.gede8f145e06
