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 v2: - renamed the `p` variables introduced by the patch series to the more explanatory `to_free`. - reworded incorrect commit message claiming that setup_discovered_git_dir() was using a static variable that is actually a singleton - reverted a code move that would have resulted in accessing uninitialized data of callers of mailinfo() that do not die() right away but clean up faithfully 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(): plug memory leak 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 | 10 ++++++++++ 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 | 9 ++++++++- 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, 135 insertions(+), 51 deletions(-) base-commit: d2bbb7c2bcf6e77ebfcabf4e12110fe6d5c91de6 Published-As: https://github.com/dscho/git/releases/tag/coverity-v3 Fetch-It-Via: git fetch https://github.com/dscho/git coverity-v3 Interdiff vs v2: diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c index 9b3efc8e987..664400b8169 100644 --- a/builtin/mailsplit.c +++ b/builtin/mailsplit.c @@ -232,9 +232,18 @@ 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)); - if (peek != EOF) - ungetc(peek, f); + ungetc(peek, f); if (strbuf_getwholeline(&buf, f, '\n')) { /* empty stdin is OK */ diff --git a/builtin/mktree.c b/builtin/mktree.c index f0354bc9718..da0fd8cd706 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, *p = NULL; + 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 = p = strbuf_detach(&p_uq, NULL); + path = to_free = strbuf_detach(&p_uq, NULL); } /* @@ -136,7 +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(p); + free(to_free); } int cmd_mktree(int ac, const char **av, const char *prefix) diff --git a/builtin/name-rev.c b/builtin/name-rev.c index a4ce73fb1e9..e7a3fe7ee70 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -28,7 +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 *p = NULL; + char *to_free = NULL; parse_commit(commit); @@ -36,7 +36,7 @@ static void name_rev(struct commit *commit, return; if (deref) { - tip_name = p = xstrfmt("%s^0", tip_name); + tip_name = to_free = xstrfmt("%s^0", tip_name); if (generation) die("generation: %d, but deref?", generation); @@ -55,7 +55,7 @@ static void name_rev(struct commit *commit, name->generation = generation; name->distance = distance; } else { - free(p); + free(to_free); return; } diff --git a/mailinfo.c b/mailinfo.c index a319911b510..f92cb9f729c 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -1097,6 +1097,9 @@ 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); if (peek == EOF) { @@ -1106,9 +1109,6 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *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/setup.c b/setup.c index 12efca85a41..e3f7699a902 100644 --- a/setup.c +++ b/setup.c @@ -703,15 +703,15 @@ 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 *p = NULL; + char *to_free = NULL; const char *ret; if (offset != cwd->len && !is_absolute_path(gitdir)) - gitdir = p = real_pathdup(gitdir, 1); + gitdir = to_free = real_pathdup(gitdir, 1); if (chdir(cwd->buf)) die_errno("Could not come back to cwd"); ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok); - free(p); + free(to_free); return ret; } -- 2.12.2.windows.2.800.gede8f145e06