From 345758ad45ffddb5d69ca2e97324127a20ab6827 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Mar 2016 14:24:09 +0100 Subject: [PATCH 1/8] describe: handle error code returned by git_pqueue_insert --- src/describe.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/describe.c b/src/describe.c index 48f04e858..13ddad5be 100644 --- a/src/describe.c +++ b/src/describe.c @@ -582,7 +582,8 @@ static int describe( best = (struct possible_tag *)git_vector_get(&all_matches, 0); if (gave_up_on) { - git_pqueue_insert(&list, gave_up_on); + if ((error = git_pqueue_insert(&list, gave_up_on)) < 0) + goto cleanup; seen_commits--; } if ((error = finish_depth_computation( From e126bc95cd296767ae6c372abb3d4c87ca359a57 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Mar 2016 14:40:17 +0100 Subject: [PATCH 2/8] config_file: handle missing quotation marks in section header When parsing a section header we expect something along the format of '[section "subsection"]'. When a section is mal-formated and is entirely missing its quotation marks we catch this case by observing that `strchr(line, '"') - strrchr(line, '"') = NULL - NULL = 0` and error out. Unfortunately, the error message is misleading though, as we state that we are missing the closing quotation mark while we in fact miss both quotation marks. Improve the error message by explicitly checking if the first quotation mark could be found and, if not, stating that quotation marks are completely missing. --- src/config_file.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/config_file.c b/src/config_file.c index 5f5e309e0..65971b930 100644 --- a/src/config_file.c +++ b/src/config_file.c @@ -1032,6 +1032,11 @@ static int parse_section_header_ext(struct reader *reader, const char *line, con */ first_quote = strchr(line, '"'); + if (first_quote == NULL) { + set_parse_error(reader, 0, "Missing quotation marks in section header"); + return -1; + } + last_quote = strrchr(line, '"'); quoted_len = last_quote - first_quote; From 61d7328dc373e80db17fbebe36fb11b32efc047a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Mar 2016 15:35:45 +0100 Subject: [PATCH 3/8] object: avoid call of memset with ouf of bounds pointer When computing a short OID we do this by first copying the leading parts into the new OID structure and then setting the trailing part to zero. In the case of the desired length being `GIT_OID_HEXSZ - 1` we will call `memset` with an out of bounds pointer and a length of 0. While this seems to cause no problems for common platforms the C89 standard does not explicitly state that calling `memset` with an out of bounds pointer and length of 0 is valid. Fix the potential issue by using the newly introduced `git_oid__cpy_prefix` function. --- src/object.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/object.c b/src/object.c index ebf77fb47..1d45f9f1b 100644 --- a/src/object.c +++ b/src/object.c @@ -12,6 +12,7 @@ #include "commit.h" #include "tree.h" #include "blob.h" +#include "oid.h" #include "tag.h" bool git_object__strict_input_validation = true; @@ -166,13 +167,9 @@ int git_object_lookup_prefix( error = git_odb_read(&odb_obj, odb, id); } } else { - git_oid short_oid; + git_oid short_oid = {{ 0 }}; - /* We copy the first len*4 bits from id and fill the remaining with 0s */ - memcpy(short_oid.id, id->id, (len + 1) / 2); - if (len % 2) - short_oid.id[len / 2] &= 0xF0; - memset(short_oid.id + (len + 1) / 2, 0, (GIT_OID_HEXSZ - len) / 2); + git_oid__cpy_prefix(&short_oid, id, len); /* If len < GIT_OID_HEXSZ (a strict short oid was given), we have * 2 options : From 80a834a5af6f8ea70b9f4e8657fa80da124c5693 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Mar 2016 16:00:49 +0100 Subject: [PATCH 4/8] index: assert required OID are non-NULL --- src/index.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/index.c b/src/index.c index b97f8091d..62aacf959 100644 --- a/src/index.c +++ b/src/index.c @@ -963,14 +963,20 @@ static int index_entry_reuc_init(git_index_reuc_entry **reuc_out, *reuc_out = reuc = reuc_entry_alloc(path); GITERR_CHECK_ALLOC(reuc); - if ((reuc->mode[0] = ancestor_mode) > 0) + if ((reuc->mode[0] = ancestor_mode) > 0) { + assert(ancestor_oid); git_oid_cpy(&reuc->oid[0], ancestor_oid); + } - if ((reuc->mode[1] = our_mode) > 0) + if ((reuc->mode[1] = our_mode) > 0) { + assert(our_oid); git_oid_cpy(&reuc->oid[1], our_oid); + } - if ((reuc->mode[2] = their_mode) > 0) + if ((reuc->mode[2] = their_mode) > 0) { + assert(their_oid); git_oid_cpy(&reuc->oid[2], their_oid); + } return 0; } From 3fe5768b061f319a4f8fa55c25614a31767d2208 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Mar 2016 17:55:40 +0100 Subject: [PATCH 5/8] pack-objects: fix memory leak on overflow --- src/pack-objects.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pack-objects.c b/src/pack-objects.c index 46fe8f3db..11e13f7d4 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -848,8 +848,10 @@ static int try_delta(git_packbuilder *pb, struct unpacked *trg, git_packbuilder__cache_unlock(pb); - if (overflow) + if (overflow) { + git__free(delta_buf); return -1; + } trg_object->delta_data = git__realloc(delta_buf, delta_size); GITERR_CHECK_ALLOC(trg_object->delta_data); From 486302d6af009d8c62fa1bd1d5b1ff2b36c31189 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Mar 2016 19:11:33 +0100 Subject: [PATCH 6/8] submodule: avoid passing NULL pointers to strncmp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In C89 it is undefined behavior to pass `NULL` pointers to `strncmp` and later on in C99 it has been explicitly stated that functions with an argument declared as `size_t nmemb` specifying the array length shall always have valid parameters, no matter if `nmemb` is 0 or not (see ISO 9899 ยง7.21.1.2). The function `str_equal_no_trailing_slash` always passes its parameters to `strncmp` if their lengths match. This means if one parameter is `NULL` and the other one either `NULL` or a string with length 0 we will pass the pointers to `strncmp` and cause undefined behavior. Fix this by explicitly handling the case when both lengths are 0. --- src/submodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/submodule.c b/src/submodule.c index 38db41529..3f39b9ef0 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -80,7 +80,8 @@ static kh_inline int str_equal_no_trailing_slash(const char *a, const char *b) if (blen > 0 && b[blen - 1] == '/') blen--; - return (alen == blen && strncmp(a, b, alen) == 0); + return (alen == 0 && blen == 0) || + (alen == blen && strncmp(a, b, alen) == 0); } __KHASH_IMPL( From 1a8c11f44356e7b1379b3bced5bbf86fce576c28 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 10 Mar 2016 10:40:47 +0100 Subject: [PATCH 7/8] diff_tform: fix potential NULL pointer access When the user passes in a diff which has no repository associated we may call `git_config__get_int_force` with a NULL-pointer configuration. Even though `git_config__get_int_force` is designed to swallow errors, it is not intended to be called with a NULL pointer configuration. Fix the issue by only calling `git_config__get_int_force` only when configuration could be retrieved from the repository. --- src/diff_tform.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 8577f06b8..6a6a62811 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -261,7 +261,7 @@ static int normalize_find_opts( if (!given || (given->flags & GIT_DIFF_FIND_ALL) == GIT_DIFF_FIND_BY_CONFIG) { - if (diff->repo) { + if (cfg) { char *rule = git_config__get_string_force(cfg, "diff.renames", "true"); int boolval; @@ -318,8 +318,10 @@ static int normalize_find_opts( #undef USE_DEFAULT if (!opts->rename_limit) { - opts->rename_limit = git_config__get_int_force( - cfg, "diff.renamelimit", DEFAULT_RENAME_LIMIT); + if (cfg) { + opts->rename_limit = git_config__get_int_force( + cfg, "diff.renamelimit", DEFAULT_RENAME_LIMIT); + } if (opts->rename_limit <= 0) opts->rename_limit = DEFAULT_RENAME_LIMIT; From 2615d0d6949c9f52e988ab649f10cf7a80c45186 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 2 Mar 2016 01:50:34 +0100 Subject: [PATCH 8/8] coverity: report errors when uploading tarball Curl by default does not report errors by setting the error code. As the upload can fail through several conditions (e.g. the rate limit, leading to unauthorized access) we should indicate this information in Travis CI. To improve upon the behavior, use `--write-out=%{http_code}` to write out the HTTP code in addition to the received body and return an error if the code does not equal 201. --- script/coverity.sh | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/script/coverity.sh b/script/coverity.sh index 8c826892f..7fe9eb4c7 100755 --- a/script/coverity.sh +++ b/script/coverity.sh @@ -49,10 +49,24 @@ COVERITY_UNSUPPORTED=1 \ # Upload results tar czf libgit2.tgz cov-int SHA=$(git rev-parse --short HEAD) -curl \ + +HTML="$(curl \ + --silent \ + --write-out "\n%{http_code}" \ --form token="$COVERITY_TOKEN" \ --form email=bs@github.com \ --form file=@libgit2.tgz \ --form version="$SHA" \ --form description="Travis build" \ - https://scan.coverity.com/builds?project=libgit2 + https://scan.coverity.com/builds?project=libgit2)" +# Body is everything up to the last line +BODY="$(echo "$HTML" | head -n-1)" +# Status code is the last line +STATUS_CODE="$(echo "$HTML" | tail -n1)" + +echo "${BODY}" + +if [ "${STATUS_CODE}" != "201" ]; then + echo "Received error code ${STATUS_CODE} from Coverity" + exit 1 +fi