From dd0b1e8cb643371326991428c7ad8fc9a61a5e1e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 09:13:25 +0100 Subject: [PATCH 1/6] openssl_stream: fix releasing OpenSSL locks The OpenSSL library may require multiple locks to work correctly, where it is the caller's responsibility to initialize and release the locks. While we correctly initialized up to `n` locks, as determined by `CRYPTO_num_locks`, we were repeatedly freeing the same mutex in our shutdown procedure. Fix the issue by freeing locks at the correct index. --- src/openssl_stream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/openssl_stream.c b/src/openssl_stream.c index bb9b32c67..c0a9c3c37 100644 --- a/src/openssl_stream.c +++ b/src/openssl_stream.c @@ -66,7 +66,7 @@ static void shutdown_ssl_locking(void) CRYPTO_set_locking_callback(NULL); for (i = 0; i < num_locks; ++i) - git_mutex_free(openssl_locks); + git_mutex_free(&openssl_locks[i]); git__free(openssl_locks); } From ff8d2eb15f145640c3b54755b460a06b24f0ca0e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 09:34:25 +0100 Subject: [PATCH 2/6] blame_git: check return value of object lookup The function `pass_whole_blame` performs an object lookup but does not check if the lookup actually succeeds. Convert the function to return an error code and check for it in the calling function. --- src/blame_git.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/blame_git.c b/src/blame_git.c index 96785c75b..735b62d95 100644 --- a/src/blame_git.c +++ b/src/blame_git.c @@ -478,14 +478,15 @@ cleanup: * The blobs of origin and porigin exactly match, so everything origin is * suspected for can be blamed on the parent. */ -static void pass_whole_blame(git_blame *blame, +static int pass_whole_blame(git_blame *blame, git_blame__origin *origin, git_blame__origin *porigin) { git_blame__entry *e; - if (!porigin->blob) - git_object_lookup((git_object**)&porigin->blob, blame->repository, - git_blob_id(origin->blob), GIT_OBJ_BLOB); + if (!porigin->blob && + git_object_lookup((git_object**)&porigin->blob, blame->repository, + git_blob_id(origin->blob), GIT_OBJ_BLOB) < 0) + return -1; for (e=blame->ent; e; e=e->next) { if (!same_suspect(e->suspect, origin)) continue; @@ -493,6 +494,8 @@ static void pass_whole_blame(git_blame *blame, origin_decref(e->suspect); e->suspect = porigin; } + + return 0; } static int pass_blame(git_blame *blame, git_blame__origin *origin, uint32_t opt) @@ -543,7 +546,8 @@ static int pass_blame(git_blame *blame, git_blame__origin *origin, uint32_t opt) } if (porigin->blob && origin->blob && !git_oid_cmp(git_blob_id(porigin->blob), git_blob_id(origin->blob))) { - pass_whole_blame(blame, origin, porigin); + error = pass_whole_blame(blame, origin, porigin); + goto finish; origin_decref(porigin); goto finish; } From 2cf48e13262921c2c6e38668c1ea54d93c2117c8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 09:34:41 +0100 Subject: [PATCH 3/6] config_file: check if section header buffer runs out of memory While parsing section headers, we use a buffer to store the actual section name. We do not check though if the buffer runs out of memory at any stage. Do so. --- src/config_file.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/config_file.c b/src/config_file.c index cd5727c05..50c5a3d82 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1041,8 +1041,9 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con GITERR_CHECK_ALLOC_ADD(&alloc_len, base_name_len, quoted_len); GITERR_CHECK_ALLOC_ADD(&alloc_len, alloc_len, 2); - git_buf_grow(&buf, alloc_len); - git_buf_printf(&buf, "%s.", base_name); + if (git_buf_grow(&buf, alloc_len) < 0 || + git_buf_printf(&buf, "%s.", base_name) < 0) + goto end_parse; rpos = 0; @@ -1082,6 +1083,11 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con } while (line + rpos < last_quote); end_parse: + if (git_buf_oom(&buf)) { + git_buf_free(&buf); + return -1; + } + if (line[rpos] != '"' || line[rpos + 1] != ']') { set_parse_error(reader, rpos, "Unexpected text after closing quotes"); git_buf_free(&buf); From 8d452448bb1d5da09966faad65bc5aa96ba3696c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 09:34:59 +0100 Subject: [PATCH 4/6] odb_pack: initialize `git_rawobj` structure The `pack_entry_find_prefix` function receives a `git_rawobj` structure as argument. While the function first initializes the structure to a sensible state, Coverity is unable to correctly detect this, resulting in a warning. Fix this warning by initializing the object to all-zeroes before passing it to the function. --- src/odb_pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/odb_pack.c b/src/odb_pack.c index b80d0337a..51770a88e 100644 --- a/src/odb_pack.c +++ b/src/odb_pack.c @@ -428,7 +428,7 @@ static int pack_backend__read_prefix( git_oid_cpy(out_oid, short_oid); } else { struct git_pack_entry e; - git_rawobj raw; + git_rawobj raw = {NULL}; if ((error = pack_entry_find_prefix( &e, (struct pack_backend *)backend, short_oid, len)) == 0 && From 723bdf48641736ece6a03032bae514ef28979dfe Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 09:35:23 +0100 Subject: [PATCH 5/6] patch_parse: check if advancing over header newline succeeds While parsing patch header lines, we iterate over each line and check if the line has trailing garbage. What we do not check though is that the line is actually a line ending with a trailing newline. Fix this by checking the return code of `parse_advance_expected_str`. --- src/patch_parse.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/patch_parse.c b/src/patch_parse.c index d993c0311..0a9edcd18 100644 --- a/src/patch_parse.c +++ b/src/patch_parse.c @@ -444,9 +444,9 @@ static int parse_header_git( goto done; parse_advance_ws(ctx); - parse_advance_expected_str(ctx, "\n"); - if (ctx->line_len > 0) { + if (parse_advance_expected_str(ctx, "\n") < 0 || + ctx->line_len > 0) { error = parse_err("trailing data at line %"PRIuZ, ctx->line_num); goto done; } From e7330016af5603021e9521c836b266295a1f2416 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 20 Mar 2017 12:38:28 +0100 Subject: [PATCH 6/6] diff_parse: check return value of `git_diff_init_options` --- src/diff_parse.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/diff_parse.c b/src/diff_parse.c index 93915683e..24a8a4af6 100644 --- a/src/diff_parse.c +++ b/src/diff_parse.c @@ -44,7 +44,11 @@ static git_diff_parsed *diff_parsed_alloc(void) diff->base.patch_fn = git_patch_parsed_from_diff; diff->base.free_fn = diff_parsed_free; - git_diff_init_options(&diff->base.opts, GIT_DIFF_OPTIONS_VERSION); + if (git_diff_init_options(&diff->base.opts, GIT_DIFF_OPTIONS_VERSION) < 0) { + git__free(&diff); + return NULL; + } + diff->base.opts.flags &= ~GIT_DIFF_IGNORE_CASE; git_pool_init(&diff->base.pool, 1);