Skip to content

Commit

Permalink
Merge branch 'coverity'
Browse files Browse the repository at this point in the history
Coverity is a tool to analyze code statically, trying to find common (or
not so common) problems before they occur in production.

Coverity offers its services to Open Source software, and just like
upstream Git, Git for Windows applied and was granted the use.

While Coverity reports a lot of false positives due to Git's (ab-)use of
the FLEX_ARRAY feature (where it declares a 0-byte or 1-byte array at the
end of a struct, and then allocates a variable-length data structure
holding a variable-length string at the end, so that the struct as well as
the string can be released with a single free()), there were a few issues
reported that are true positives, and not all of them were resource leaks
in builtins (for which it is considered kind of okay to not release memory
just before exit() is called anyway).

This topic branch tries to address a couple of those issues.

Note: there are a couple more issues left, either because they are tricky
to resolve (in some cases, the custody of occasionally-allocated memory is
very unclear) or because it is unclear whether they are false positives
(due to the hard-to-reason-about nature of the code). It's a start,
though.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
  • Loading branch information
dscho committed Apr 26, 2017
2 parents 92d3e5a + 50cb324 commit 7a2040a
Show file tree
Hide file tree
Showing 24 changed files with 87 additions and 31 deletions.
19 changes: 12 additions & 7 deletions builtin/am.c
Expand Up @@ -762,14 +762,18 @@ static int split_mail_conv(mail_conv_fn fn, struct am_state *state,
mail = mkpath("%s/%0*d", state->dir, state->prec, i + 1);

out = fopen(mail, "w");
if (!out)
if (!out) {
if (in != stdin)
fclose(in);
return error_errno(_("could not open '%s' for writing"),
mail);
}

ret = fn(out, in, keep_cr);

fclose(out);
fclose(in);
if (in != stdin)
fclose(in);

if (ret)
return error(_("could not parse patch '%s'"), *paths);
Expand Down Expand Up @@ -1355,15 +1359,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;
ret = -1;

if (!skip_prefix(sb.buf, "From ", &x))
return -1;
if (!ret && !skip_prefix(sb.buf, "From ", &x))
ret = -1;

if (get_oid_hex(x, commit_id) < 0)
return -1;
if (!ret && get_oid_hex(x, commit_id) < 0)
ret = -1;

strbuf_release(&sb);
fclose(fp);
Expand Down
1 change: 1 addition & 0 deletions builtin/cat-file.c
Expand Up @@ -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;
}

Expand Down
1 change: 1 addition & 0 deletions builtin/checkout.c
Expand Up @@ -232,6 +232,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;
}

Expand Down
2 changes: 2 additions & 0 deletions builtin/difftool.c
Expand Up @@ -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);
Expand Down Expand Up @@ -497,6 +498,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
}
}

fclose(fp);
if (finish_command(&child)) {
ret = error("error occurred running diff --raw");
goto finish;
Expand Down
1 change: 1 addition & 0 deletions builtin/fast-export.c
Expand Up @@ -765,6 +765,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)
Expand Down
2 changes: 1 addition & 1 deletion builtin/mailsplit.c
Expand Up @@ -232,7 +232,7 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,

do {
peek = fgetc(f);
} while (isspace(peek));
} while (peek >= 0 && isspace(peek));
ungetc(peek, f);

if (strbuf_getwholeline(&buf, f, '\n')) {
Expand Down
5 changes: 3 additions & 2 deletions builtin/mktree.c
Expand Up @@ -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, *p = NULL;
unsigned char sha1[20];

ptr = buf;
Expand Down Expand Up @@ -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 = p = strbuf_detach(&p_uq, NULL);
}

/*
Expand Down Expand Up @@ -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(p);
}

int cmd_mktree(int ac, const char **av, const char *prefix)
Expand Down
7 changes: 5 additions & 2 deletions builtin/name-rev.c
Expand Up @@ -28,14 +28,15 @@ 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;

parse_commit(commit);

if (commit->date < cutoff)
return;

if (deref) {
tip_name = xstrfmt("%s^0", tip_name);
tip_name = p = xstrfmt("%s^0", tip_name);

if (generation)
die("generation: %d, but deref?", generation);
Expand All @@ -53,8 +54,10 @@ static void name_rev(struct commit *commit,
name->taggerdate = taggerdate;
name->generation = generation;
name->distance = distance;
} else
} else {
free(p);
return;
}

for (parents = commit->parents;
parents;
Expand Down
1 change: 1 addition & 0 deletions builtin/pack-redundant.c
Expand Up @@ -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;
}

Expand Down
4 changes: 3 additions & 1 deletion builtin/receive-pack.c
Expand Up @@ -984,7 +984,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;
unsigned char *old_sha1 = cmd->old_sha1;
unsigned char *new_sha1 = cmd->new_sha1;

Expand All @@ -995,6 +996,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)) {
Expand Down
8 changes: 5 additions & 3 deletions builtin/worktree.c
Expand Up @@ -408,9 +408,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);
Expand Down
4 changes: 3 additions & 1 deletion compat/mingw.c
Expand Up @@ -1172,8 +1172,10 @@ static char **get_path_split(void)
++n;
}
}
if (!n)
if (!n) {
free(envpath);
return NULL;
}

ALLOC_ARRAY(path, n + 1);
p = envpath;
Expand Down
7 changes: 7 additions & 0 deletions compat/winansi.c
Expand Up @@ -100,6 +100,8 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, &mode))
return 0;
sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
FOREGROUND_RED;
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;

Expand Down Expand Up @@ -128,6 +130,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);
Expand Down
5 changes: 4 additions & 1 deletion config.c
Expand Up @@ -2426,7 +2426,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)) {
Expand Down Expand Up @@ -2508,11 +2508,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);
Expand Down
6 changes: 5 additions & 1 deletion http-backend.c
Expand Up @@ -681,8 +681,10 @@ int cmd_main(int argc, const char **argv)
if (!regexec(&re, dir, 1, out, 0)) {
size_t n;

if (strcmp(method, c->method))
if (strcmp(method, c->method)) {
free(dir);
return bad_request(&hdr, c);
}

cmd = c;
n = out[0].rm_eo - out[0].rm_so;
Expand All @@ -708,5 +710,7 @@ int cmd_main(int argc, const char **argv)
max_request_buffer);

cmd->imp(&hdr, cmd_arg);
free(dir);
free(cmd_arg);
return 0;
}
1 change: 1 addition & 0 deletions line-log.c
Expand Up @@ -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(parent_range);
return changed;
}

Expand Down
2 changes: 1 addition & 1 deletion mailinfo.c
Expand Up @@ -1094,7 +1094,7 @@ int mailinfo(struct mailinfo *mi, const char *msg, const char *patch)

do {
peek = fgetc(mi->input);
} while (isspace(peek));
} while (peek >= 0 && isspace(peek));
ungetc(peek, mi->input);

/* process the email header */
Expand Down
3 changes: 2 additions & 1 deletion patch-ids.c
Expand Up @@ -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;
Expand Down
5 changes: 4 additions & 1 deletion reflog-walk.c
Expand Up @@ -183,7 +183,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (!reflogs || reflogs->nr == 0) {
unsigned char sha1[20];
char *b;
if (dwim_log(branch, strlen(branch), sha1, &b) == 1) {
int ret = dwim_log(branch, strlen(branch), sha1, &b);
if (ret > 1)
free(b);
else if (ret == 1) {
if (reflogs) {
free(reflogs->ref);
free(reflogs);
Expand Down
5 changes: 3 additions & 2 deletions remote.c
Expand Up @@ -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"
Expand Down
11 changes: 8 additions & 3 deletions setup.c
Expand Up @@ -697,11 +697,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 *p = NULL;
const char *ret;

if (offset != cwd->len && !is_absolute_path(gitdir))
gitdir = real_pathdup(gitdir, 1);
gitdir = p = 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(p);
return ret;
}

/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
Expand Down Expand Up @@ -740,7 +745,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))
Expand Down
8 changes: 6 additions & 2 deletions shallow.c
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion worktree.c
Expand Up @@ -396,6 +396,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
Expand All @@ -415,7 +416,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;
Expand Down

0 comments on commit 7a2040a

Please sign in to comment.