From f2a554b45e52d7e682f26796e492cd64d8b9a6f4 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 14:43:28 +0100 Subject: [PATCH 01/20] coverity: hint git_vector_foreach does not deref NULL contents Coverity does not comprehend the connection between a vector's size and the contents pointer, that is that the vector's pointer is non-NULL when its size is positive. As the vector code should be reasonably well tested and users are expected to not manually modify a vector's contents it seems save to assume that the macros will never dereference a NULL pointer. Fix Coverity warnings by overriding the foreach macros with macros that explicitly aborting when (v)->contents is NULL. --- script/user_nodefs.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/script/user_nodefs.h b/script/user_nodefs.h index 3d25d92ec..5b0be81a3 100644 --- a/script/user_nodefs.h +++ b/script/user_nodefs.h @@ -25,3 +25,9 @@ #nodef GITERR_CHECK_VERSION(S,V,N) if (giterr__check_version(S,V,N) < 0) { __coverity_panic__(); } #nodef LOOKS_LIKE_DRIVE_PREFIX(S) (strlen(S) >= 2 && git__isalpha((S)[0]) && (S)[1] == ':') + +#nodef git_vector_foreach(v, iter, elem) \ + for ((iter) = 0; (v)->contents != NULL && (iter) < (v)->length && ((elem) = (v)->contents[(iter)], 1); (iter)++ ) + +#nodef git_vector_rforeach(v, iter, elem) \ + for ((iter) = (v)->length - 1; (v)->contents != NULL && (iter) < SIZE_MAX && ((elem) = (v)->contents[(iter)], 1); (iter)-- ) From 859ed5ddc7dc0e208215079265fb012eb8b48048 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 09:54:26 +0100 Subject: [PATCH 02/20] common: introduce GITERR_CHECK_ALLOC_BUF We commonly have to check if a git_buf has been allocated correctly or if we ran out of memory. Introduce a new macro similar to `GITERR_CHECK_ALLOC` which checks if we ran OOM and if so returns an error. Provide a `#nodef` for Coverity to mark the error case as an abort path. --- script/user_nodefs.h | 1 + src/common.h | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/script/user_nodefs.h b/script/user_nodefs.h index 5b0be81a3..3c06a706d 100644 --- a/script/user_nodefs.h +++ b/script/user_nodefs.h @@ -6,6 +6,7 @@ */ #nodef GITERR_CHECK_ALLOC(ptr) if (ptr == NULL) { __coverity_panic__(); } +#nodef GITERR_CHECK_ALLOC_BUF(buf) if (buf == NULL || git_buf_oom(buf)) { __coverity_panic__(); } #nodef GITERR_CHECK_ALLOC_ADD(out, one, two) \ if (GIT_ADD_SIZET_OVERFLOW(out, one, two)) { __coverity_panic__(); } diff --git a/src/common.h b/src/common.h index bc4bdd856..9abd605cb 100644 --- a/src/common.h +++ b/src/common.h @@ -89,6 +89,11 @@ */ #define GITERR_CHECK_ALLOC(ptr) if (ptr == NULL) { return -1; } +/** + * Check a buffer allocation result, returning -1 if it failed. + */ +#define GITERR_CHECK_ALLOC_BUF(buf) if ((void *)(buf) == NULL || git_buf_oom(buf)) { return -1; } + /** * Check a return value and propagate result if non-zero. */ From 42c05ed56b3bc4f5963a8c68ec6fec803b7e22e3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 10:02:44 +0100 Subject: [PATCH 03/20] path: use GITERR_CHECK_ALLOC_BUF to verify passed in buffer --- src/path.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/path.c b/src/path.c index 852ef576a..1fd14fcb9 100644 --- a/src/path.c +++ b/src/path.c @@ -705,8 +705,7 @@ int git_path_resolve_relative(git_buf *path, size_t ceiling) char *base, *to, *from, *next; size_t len; - if (!path || git_buf_oom(path)) - return -1; + GITERR_CHECK_ALLOC_BUF(path); if (ceiling > path->size) ceiling = path->size; From 6e2a37556d4793101b71ac83747a3acb31ee3c5b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 11:45:43 +0100 Subject: [PATCH 04/20] smart_pkt: check buffer with GITERR_CHECK_ALLOC_BUF --- src/transports/smart_pkt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c index 870f08497..2c33916c7 100644 --- a/src/transports/smart_pkt.c +++ b/src/transports/smart_pkt.c @@ -543,7 +543,9 @@ static int buffer_want_with_caps(const git_remote_head *head, transport_smart_ca "%04xwant %s %s\n", (unsigned int)len, oid, git_buf_cstr(&str)); git_buf_free(&str); - return git_buf_oom(buf); + GITERR_CHECK_ALLOC_BUF(buf); + + return 0; } /* From c5bd70d13843cc213ec57b10168dd94ed362843b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 11:48:30 +0100 Subject: [PATCH 05/20] revwalk: use GITERR_CHECK_ALLOC_BUF --- src/revwalk.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/revwalk.c b/src/revwalk.c index 89279ed1f..4815a1089 100644 --- a/src/revwalk.c +++ b/src/revwalk.c @@ -223,8 +223,7 @@ static int push_glob(git_revwalk *walk, const char *glob, int hide) git_buf_joinpath(&buf, GIT_REFS_DIR, glob); else git_buf_puts(&buf, glob); - if (git_buf_oom(&buf)) - return -1; + GITERR_CHECK_ALLOC_BUF(&buf); /* If no '?', '*' or '[' exist, we append '/ *' to the glob */ wildcard = strcspn(glob, "?*["); From b9f28b8d52d595943eb416fec8f5bd3f5ec21f5b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 10:09:03 +0100 Subject: [PATCH 06/20] refspec: check buffer with GITERR_CHECK_ALLOC_BUF --- src/refspec.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/refspec.c b/src/refspec.c index f92a6d2b6..debde8692 100644 --- a/src/refspec.c +++ b/src/refspec.c @@ -323,8 +323,8 @@ int git_refspec__dwim_one(git_vector *out, git_refspec *spec, git_vector *refs) if (git__prefixcmp(spec->src, GIT_REFS_DIR)) { for (j = 0; formatters[j]; j++) { git_buf_clear(&buf); - if (git_buf_printf(&buf, formatters[j], spec->src) < 0) - return -1; + git_buf_printf(&buf, formatters[j], spec->src); + GITERR_CHECK_ALLOC_BUF(&buf); key.name = (char *) git_buf_cstr(&buf); if (!git_vector_search(&pos, refs, &key)) { @@ -348,8 +348,8 @@ int git_refspec__dwim_one(git_vector *out, git_refspec *spec, git_vector *refs) git_buf_puts(&buf, GIT_REFS_HEADS_DIR); } - if (git_buf_puts(&buf, spec->dst) < 0) - return -1; + git_buf_puts(&buf, spec->dst); + GITERR_CHECK_ALLOC_BUF(&buf); cur->dst = git_buf_detach(&buf); } From 2129d6df93ba0ad5b2f7cf15d4e3cfa39487d5a0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 13:33:48 +0100 Subject: [PATCH 07/20] crlf: do not ignore GIT_PASSTHROUGH error When no payload is set for `crlf_apply` we try to compute the crlf attributes ourselves with `crlf_check`. When the function determines that the current file does not require any treatment we return the GIT_PASSTHROUGH error code without actually allocating the out-pointer, which indicates the file should not be passed through the filter. The `crlf_apply` function explicitly checks for the GIT_PASSTHROUGH return code and ignores it. This means we will try to apply the crlf-filter to the current file, leading us to dereference the unallocated payload-pointer. Fix this obviously incorrect behavior by not treating GIT_PASSTHROUGH in any special way. This is the correct thing to do anyway, as the code indicates that the file should not be passed through the filter. --- src/crlf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crlf.c b/src/crlf.c index f391137c1..5d7510ac7 100644 --- a/src/crlf.c +++ b/src/crlf.c @@ -346,7 +346,7 @@ static int crlf_apply( /* initialize payload in case `check` was bypassed */ if (!*payload) { int error = crlf_check(self, payload, src, NULL); - if (error < 0 && error != GIT_PASSTHROUGH) + if (error < 0) return error; } From d1c9a48df667d6c83cca2ad21b1200fb65d7a1c6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 10:45:09 +0100 Subject: [PATCH 08/20] pack-objects: check realloc in try_delta with GITERR_CHECK_ALLOC --- src/pack-objects.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/pack-objects.c b/src/pack-objects.c index 0afa28e62..5d9c09dd7 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -850,9 +850,11 @@ static int try_delta(git_packbuilder *pb, struct unpacked *trg, git_packbuilder__cache_unlock(pb); - if (overflow || - !(trg_object->delta_data = git__realloc(delta_buf, delta_size))) + if (overflow) return -1; + + trg_object->delta_data = git__realloc(delta_buf, delta_size); + GITERR_CHECK_ALLOC(trg_object->delta_data); } else { /* create delta when writing the pack */ git_packbuilder__cache_unlock(pb); From bac52ab0f2e8d09de1f98590f177e9e847cdb11b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 13:48:45 +0100 Subject: [PATCH 09/20] pack-objects: return early when computing write order fails The function `compute_write_order` may return a `NULL`-pointer when an error occurs. In such cases we jump to the `done`-label where we try to clean up allocated memory. Unfortunately we try to deallocate the `write_order` array, though, which may be NULL here. Fix this error by returning early instead of jumping to the `done` label. There is no data to be cleaned up anyway. --- src/pack-objects.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/pack-objects.c b/src/pack-objects.c index 5d9c09dd7..46fe8f3db 100644 --- a/src/pack-objects.c +++ b/src/pack-objects.c @@ -629,10 +629,8 @@ static int write_pack(git_packbuilder *pb, int error = 0; write_order = compute_write_order(pb); - if (write_order == NULL) { - error = -1; - goto done; - } + if (write_order == NULL) + return -1; /* Write pack header */ ph.hdr_signature = htonl(PACK_SIGNATURE); From be8479c9873315afcf6b86f0b1bb79052a23b363 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 14:01:50 +0100 Subject: [PATCH 10/20] diff_print: assert patch is non-NULL When invoking `diff_print_info_init_frompatch` it is obvious that the patch should be non-NULL. We explicitly check if the variable is set and continue afterwards, happily dereferencing the potential NULL-pointer. Fix this by instead asserting that patch is set. This also silences Coverity. --- src/diff_print.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/diff_print.c b/src/diff_print.c index bc2d6fab0..dae9e341d 100644 --- a/src/diff_print.c +++ b/src/diff_print.c @@ -92,7 +92,11 @@ static int diff_print_info_init_frompatch( git_diff_line_cb cb, void *payload) { - git_repository *repo = patch && patch->diff ? patch->diff->repo : NULL; + git_repository *repo; + + assert(patch); + + repo = patch->diff ? patch->diff->repo : NULL; memset(pi, 0, sizeof(diff_print_info)); From 793e0855365bbabfeffbd730fd9ee5ee9a011546 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 14:06:48 +0100 Subject: [PATCH 11/20] refdb_fs: remove unnecessary check for NULL The fail-label of `reflog_parse` explicitly checks the entry poitner for NULL before freeing it. When we jump to the label the variable has to be set to a non-NULL and valid pointer though: if the allocation fails we immediately return with an error code and if the loop was not entered we return with a success code, withouth executing the label's code. Remove the useless NULL-check to silence Coverity. --- src/refdb_fs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 1348c67a1..f6ed7201a 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -1512,8 +1512,7 @@ static int reflog_parse(git_reflog *log, const char *buf, size_t buf_size) #undef seek_forward fail: - if (entry) - git_reflog_entry__free(entry); + git_reflog_entry__free(entry); return -1; } From 003c5e46a84ec7ecee134b9471e6c84ba673847d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 15:52:49 +0100 Subject: [PATCH 12/20] transports: smart_pkt: fix memory leaks on error paths --- src/transports/smart_pkt.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/transports/smart_pkt.c b/src/transports/smart_pkt.c index 2c33916c7..2ea57bb64 100644 --- a/src/transports/smart_pkt.c +++ b/src/transports/smart_pkt.c @@ -296,13 +296,12 @@ static int ng_pkt(git_pkt **out, const char *line, size_t len) pkt = git__malloc(sizeof(*pkt)); GITERR_CHECK_ALLOC(pkt); + pkt->ref = NULL; pkt->type = GIT_PKT_NG; line += 3; /* skip "ng " */ - if (!(ptr = strchr(line, ' '))) { - giterr_set(GITERR_NET, "Invalid packet line"); - return -1; - } + if (!(ptr = strchr(line, ' '))) + goto out_err; len = ptr - line; GITERR_CHECK_ALLOC_ADD(&alloclen, len, 1); @@ -313,12 +312,8 @@ static int ng_pkt(git_pkt **out, const char *line, size_t len) pkt->ref[len] = '\0'; line = ptr + 1; - if (!(ptr = strchr(line, '\n'))) { - giterr_set(GITERR_NET, "Invalid packet line"); - git__free(pkt->ref); - git__free(pkt); - return -1; - } + if (!(ptr = strchr(line, '\n'))) + goto out_err; len = ptr - line; GITERR_CHECK_ALLOC_ADD(&alloclen, len, 1); @@ -330,6 +325,12 @@ static int ng_pkt(git_pkt **out, const char *line, size_t len) *out = (git_pkt *)pkt; return 0; + +out_err: + giterr_set(GITERR_NET, "Invalid packet line"); + git__free(pkt->ref); + git__free(pkt); + return -1; } static int unpack_pkt(git_pkt **out, const char *line, size_t len) From 7808c93797b3fa9f552bd2e24672089b8d27ad2a Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 15:59:15 +0100 Subject: [PATCH 13/20] index: plug memory leak in `read_conflict_names` --- src/index.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/index.c b/src/index.c index d0a0da2c5..85c2f8ea8 100644 --- a/src/index.c +++ b/src/index.c @@ -2193,9 +2193,10 @@ static int read_conflict_names(git_index *index, const char *buffer, size_t size #define read_conflict_name(ptr) \ len = p_strnlen(buffer, size) + 1; \ - if (size < len) \ - return index_error_invalid("reading conflict name entries"); \ - \ + if (size < len) { \ + index_error_invalid("reading conflict name entries"); \ + goto out_err; \ + } \ if (len == 1) \ ptr = NULL; \ else { \ @@ -2216,7 +2217,16 @@ static int read_conflict_names(git_index *index, const char *buffer, size_t size read_conflict_name(conflict_name->theirs); if (git_vector_insert(&index->names, conflict_name) < 0) - return -1; + goto out_err; + + continue; + +out_err: + git__free(conflict_name->ancestor); + git__free(conflict_name->ours); + git__free(conflict_name->theirs); + git__free(conflict_name); + return -1; } #undef read_conflict_name From 0f1e2d2066115e62fd7396e0f436b4a5dd8384cd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 11:23:26 +0100 Subject: [PATCH 14/20] index: fix contradicting comparison The overflow check in `read_reuc` tries to verify if the `git__strtol32` parses an integer bigger than UINT_MAX. The `tmp` variable is casted to an unsigned int for this and then checked for being greater than UINT_MAX, which obviously can never be true. Fix this by instead fixing the `mode` field's size in `struct git_index_reuc_entry` to `uint32_t`. We can now parse the int with `git__strtol64`, which can never return a value bigger than `UINT32_MAX`, and additionally checking if the returned value is smaller than zero. We do not need to handle overflows explicitly here, as `git__strtol64` returns an error when the returned value would overflow. --- include/git2/sys/index.h | 2 +- src/index.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/git2/sys/index.h b/include/git2/sys/index.h index 29a99f798..2e2b87e68 100644 --- a/include/git2/sys/index.h +++ b/include/git2/sys/index.h @@ -25,7 +25,7 @@ typedef struct git_index_name_entry { /** Representation of a resolve undo entry in the index. */ typedef struct git_index_reuc_entry { - unsigned int mode[3]; + uint32_t mode[3]; git_oid oid[3]; char *path; } git_index_reuc_entry; diff --git a/src/index.c b/src/index.c index 85c2f8ea8..483f7af7c 100644 --- a/src/index.c +++ b/src/index.c @@ -2135,11 +2135,11 @@ static int read_reuc(git_index *index, const char *buffer, size_t size) /* read 3 ASCII octal numbers for stage entries */ for (i = 0; i < 3; i++) { - int tmp; + int64_t tmp; - if (git__strtol32(&tmp, buffer, &endptr, 8) < 0 || + if (git__strtol64(&tmp, buffer, &endptr, 8) < 0 || !endptr || endptr == buffer || *endptr || - (unsigned)tmp > UINT_MAX) { + tmp < 0) { index_entry_reuc_free(lost); return index_error_invalid("reading reuc entry stage"); } From d0cb11e794de0dbf3998ad88357c17f5d8bf843c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 16:01:03 +0100 Subject: [PATCH 15/20] remote: set error code in `create_internal` Set the error code when an error occurs in any of the called functions. This ensures we pass the error up to callers and actually free the remote when an error occurs. --- src/remote.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/remote.c b/src/remote.c index 2f8ffcb37..8b7203ee2 100644 --- a/src/remote.c +++ b/src/remote.c @@ -208,8 +208,8 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n remote->repo = repo; - if (git_vector_init(&remote->refs, 32, NULL) < 0 || - canonicalize_url(&canonical_url, url) < 0) + if ((error = git_vector_init(&remote->refs, 32, NULL)) < 0 || + (error = canonicalize_url(&canonical_url, url)) < 0) goto on_error; remote->url = apply_insteadof(repo->_config, canonical_url.ptr, GIT_DIRECTION_FETCH); From 2afb6fa46df25ef77a166b92304cc6e725103c7c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 16:05:13 +0100 Subject: [PATCH 16/20] rebase: plug memory leak in `rebase_alloc` Convert `rebase_alloc` to use our usual error propagation patterns, that is accept an out-parameter and return an error code that is to be checked by the caller. This allows us to use the GITERR_CHECK_ALLOC macro, which helps static analysis. --- src/rebase.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/rebase.c b/src/rebase.c index b9d1d4fc5..bcad9b7cd 100644 --- a/src/rebase.c +++ b/src/rebase.c @@ -257,12 +257,12 @@ done: return error; } -static git_rebase *rebase_alloc(const git_rebase_options *rebase_opts) +static int rebase_alloc(git_rebase **out, const git_rebase_options *rebase_opts) { git_rebase *rebase = git__calloc(1, sizeof(git_rebase)); + GITERR_CHECK_ALLOC(rebase); - if (!rebase) - return NULL; + *out = NULL; if (rebase_opts) memcpy(&rebase->options, rebase_opts, sizeof(git_rebase_options)); @@ -270,14 +270,16 @@ static git_rebase *rebase_alloc(const git_rebase_options *rebase_opts) git_rebase_init_options(&rebase->options, GIT_REBASE_OPTIONS_VERSION); if (rebase_opts && rebase_opts->rewrite_notes_ref) { - if ((rebase->options.rewrite_notes_ref = git__strdup(rebase_opts->rewrite_notes_ref)) == NULL) - return NULL; + rebase->options.rewrite_notes_ref = git__strdup(rebase_opts->rewrite_notes_ref); + GITERR_CHECK_ALLOC(rebase->options.rewrite_notes_ref); } if ((rebase->options.checkout_options.checkout_strategy & (GIT_CHECKOUT_SAFE | GIT_CHECKOUT_FORCE)) == 0) rebase->options.checkout_options.checkout_strategy = GIT_CHECKOUT_SAFE; - return rebase; + *out = rebase; + + return 0; } static int rebase_check_versions(const git_rebase_options *given_opts) @@ -305,8 +307,8 @@ int git_rebase_open( if ((error = rebase_check_versions(given_opts)) < 0) return error; - rebase = rebase_alloc(given_opts); - GITERR_CHECK_ALLOC(rebase); + if (rebase_alloc(&rebase, given_opts) < 0) + return -1; rebase->repo = repo; @@ -708,8 +710,8 @@ int git_rebase_init( branch = head_branch; } - rebase = rebase_alloc(given_opts); - GITERR_CHECK_ALLOC(rebase); + if (rebase_alloc(&rebase, given_opts) < 0) + return -1; rebase->repo = repo; rebase->inmemory = inmemory; From 2baf854e975267eb560b48f1ead0641ec676d637 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 16:08:56 +0100 Subject: [PATCH 17/20] openssl_stream: fix memory leak when creating new stream --- src/openssl_stream.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/openssl_stream.c b/src/openssl_stream.c index 1dad5f637..840e7dc3f 100644 --- a/src/openssl_stream.c +++ b/src/openssl_stream.c @@ -545,6 +545,7 @@ int git_openssl_stream_new(git_stream **out, const char *host, const char *port) st = git__calloc(1, sizeof(openssl_stream)); GITERR_CHECK_ALLOC(st); + st->io = NULL; #ifdef GIT_CURL error = git_curl_stream_new(&st->io, host, port); #else @@ -552,12 +553,13 @@ int git_openssl_stream_new(git_stream **out, const char *host, const char *port) #endif if (error < 0) - return error; + goto out_err; st->ssl = SSL_new(git__ssl_ctx); if (st->ssl == NULL) { giterr_set(GITERR_SSL, "failed to create ssl object"); - return -1; + error = -1; + goto out_err; } st->host = git__strdup(host); @@ -576,6 +578,12 @@ int git_openssl_stream_new(git_stream **out, const char *host, const char *port) *out = (git_stream *) st; return 0; + +out_err: + git_stream_free(st->io); + git__free(st); + + return error; } #else From 05bf67b90186d2ad2c6c9adfa2c0024520cdb342 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 11:16:36 +0100 Subject: [PATCH 18/20] openssl_stream: fix NULL pointer dereference --- src/openssl_stream.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/openssl_stream.c b/src/openssl_stream.c index 840e7dc3f..b713171c6 100644 --- a/src/openssl_stream.c +++ b/src/openssl_stream.c @@ -383,6 +383,8 @@ static int verify_server_cert(SSL *ssl, const char *host) GITERR_CHECK_ALLOC(peer_cn); memcpy(peer_cn, ASN1_STRING_data(str), size); peer_cn[size] = '\0'; + } else { + goto cert_fail_name; } } else { int size = ASN1_STRING_to_UTF8(&peer_cn, str); From 3d1abc5afcee2b878d835df11530aab8ffa0d1e1 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 22 Feb 2016 17:13:23 +0100 Subject: [PATCH 19/20] xmerge: fix memory leak on error path --- src/xdiff/xmerge.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/xdiff/xmerge.c b/src/xdiff/xmerge.c index 7b7e0e2d3..7928d1418 100644 --- a/src/xdiff/xmerge.c +++ b/src/xdiff/xmerge.c @@ -646,6 +646,8 @@ int xdl_merge(mmfile_t *orig, mmfile_t *mf1, mmfile_t *mf2, if (xdl_change_compact(&xe2.xdf1, &xe2.xdf2, xpp->flags) < 0 || xdl_change_compact(&xe2.xdf2, &xe2.xdf1, xpp->flags) < 0 || xdl_build_script(&xe2, &xscr2) < 0) { + xdl_free_script(xscr1); + xdl_free_env(&xe1); xdl_free_env(&xe2); return -1; } From 32f0798413f83cbd1c22e11d81eeb9f664181ec9 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 23 Feb 2016 11:07:03 +0100 Subject: [PATCH 20/20] diff_tform: fix potential NULL pointer access The `normalize_find_opts` function in theory allows for the incoming diff to have no repository. When the caller does not pass in diff find options or if the GIT_DIFF_FIND_BY_CONFIG value is set, though, we try to derive the configuration from the diff's repository configuration without first verifying that the repository is actually set to a non-NULL value. Fix this issue by explicitly checking if the repository is set and if it is not, fall back to a default value of GIT_DIFF_FIND_RENAMES. --- src/diff_tform.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/diff_tform.c b/src/diff_tform.c index 7cff34159..8577f06b8 100644 --- a/src/diff_tform.c +++ b/src/diff_tform.c @@ -261,18 +261,23 @@ static int normalize_find_opts( if (!given || (given->flags & GIT_DIFF_FIND_ALL) == GIT_DIFF_FIND_BY_CONFIG) { - char *rule = - git_config__get_string_force(cfg, "diff.renames", "true"); - int boolval; + if (diff->repo) { + char *rule = + git_config__get_string_force(cfg, "diff.renames", "true"); + int boolval; - if (!git__parse_bool(&boolval, rule) && !boolval) - /* don't set FIND_RENAMES if bool value is false */; - else if (!strcasecmp(rule, "copies") || !strcasecmp(rule, "copy")) - opts->flags |= GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES; - else + if (!git__parse_bool(&boolval, rule) && !boolval) + /* don't set FIND_RENAMES if bool value is false */; + else if (!strcasecmp(rule, "copies") || !strcasecmp(rule, "copy")) + opts->flags |= GIT_DIFF_FIND_RENAMES | GIT_DIFF_FIND_COPIES; + else + opts->flags |= GIT_DIFF_FIND_RENAMES; + + git__free(rule); + } else { + /* set default flag */ opts->flags |= GIT_DIFF_FIND_RENAMES; - - git__free(rule); + } } /* some flags imply others */