From 2d9426b049335c1a39be7ea7416094e944bfe63c Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:00 -0700 Subject: [PATCH 01/10] read-cache: free cache entry in add_to_index in case of early return This frees `ce` would be leaking in the error path. Additionally a free is moved towards the return. This helps code readability as we often have this pattern of freeing resources just before return/exit and not in between the code. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- read-cache.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/read-cache.c b/read-cache.c index 8d71860f69..60abec6055 100644 --- a/read-cache.c +++ b/read-cache.c @@ -681,15 +681,18 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, alias = index_file_exists(istate, ce->name, ce_namelen(ce), ignore_case); if (alias && !ce_stage(alias) && !ie_match_stat(istate, alias, st, ce_option)) { /* Nothing changed, really */ - free(ce); if (!S_ISGITLINK(alias->ce_mode)) ce_mark_uptodate(alias); alias->ce_flags |= CE_ADDED; + + free(ce); return 0; } if (!intent_only) { - if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT)) + if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT)) { + free(ce); return error("unable to index file %s", path); + } } else set_object_name_for_intent_to_add_entry(ce); From 1b7cb8969cb204b64b9f8ce25c86986e8144d352 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:01 -0700 Subject: [PATCH 02/10] update-index: fix a memleak `old` is not used outside the loop and would get lost once we reach the goto. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/update-index.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/update-index.c b/builtin/update-index.c index 587898624c..6271b54adc 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -584,6 +584,7 @@ static int do_reupdate(int ac, const char **av, path = xstrdup(ce->name); update_one(path); free(path); + free(old); if (save_nr != active_nr) goto redo; } From f0b1f1ece71a2bcf99e5890757ee0e41490ec7e0 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:02 -0700 Subject: [PATCH 03/10] builtin/apply.c: fix a memleak oldlines is allocated earlier in the function and also freed on the successful code path. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/apply.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/apply.c b/builtin/apply.c index 65b97eee69..0769b09287 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -2776,7 +2776,8 @@ static int apply_one_fragment(struct image *img, struct fragment *frag, default: if (apply_verbosely) error(_("invalid start of line: '%c'"), first); - return -1; + applied_pos = -1; + goto out; } if (added_blank_line) { if (!new_blank_lines_at_end) @@ -2915,6 +2916,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag, (int)(old - oldlines), oldlines); } +out: free(oldlines); strbuf_release(&newlines); free(preimage.line_allocated); From d687839c29624de4b00ac22a6d8160172b357e14 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:03 -0700 Subject: [PATCH 04/10] merge-blobs.c: fix a memleak Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- merge-blobs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/merge-blobs.c b/merge-blobs.c index 57211bccb7..7abb894c68 100644 --- a/merge-blobs.c +++ b/merge-blobs.c @@ -14,8 +14,10 @@ static int fill_mmfile_blob(mmfile_t *f, struct blob *obj) buf = read_sha1_file(obj->object.sha1, &type, &size); if (!buf) return -1; - if (type != OBJ_BLOB) + if (type != OBJ_BLOB) { + free(buf); return -1; + } f->ptr = buf; f->size = size; return 0; From 473091e21ef2dbee6087cdf0429858faf2b50740 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:04 -0700 Subject: [PATCH 05/10] merge-recursive: fix memleaks These string_list instances were allocated by get_renames() and get_unmerged for the sole use of this caller, and the function is responsible for freeing them, not just their contents. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- merge-recursive.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/merge-recursive.c b/merge-recursive.c index 771f5e21b0..1c9c30db6c 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1858,6 +1858,9 @@ int merge_trees(struct merge_options *o, string_list_clear(re_head, 0); string_list_clear(entries, 1); + free(re_merge); + free(re_head); + free(entries); } else clean = 1; From e280888cfb5c0fb5b75acaab04aa2ca28da3225b Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:05 -0700 Subject: [PATCH 06/10] http-push: remove unneeded cleanup preq is NULL as the condition the line before dictates. And the cleanup function release_http_pack_request is not null pointer safe. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- http-push.c | 1 - 1 file changed, 1 deletion(-) diff --git a/http-push.c b/http-push.c index 0beb7ab67f..9469684e9c 100644 --- a/http-push.c +++ b/http-push.c @@ -316,7 +316,6 @@ static void start_fetch_packed(struct transfer_request *request) preq = new_http_pack_request(target, repo->url); if (preq == NULL) { - release_http_pack_request(preq); repo->can_update_info_refs = 0; return; } From 5d0b9bf86d98b690e443b266e95add7fab334f14 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:07 -0700 Subject: [PATCH 07/10] commit.c: fix a memory leak Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- builtin/commit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 961e467242..da79ac4bc7 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -229,7 +229,7 @@ static int commit_index_files(void) static int list_paths(struct string_list *list, const char *with_tree, const char *prefix, const struct pathspec *pattern) { - int i; + int i, ret; char *m; if (!pattern->nr) @@ -256,7 +256,9 @@ static int list_paths(struct string_list *list, const char *with_tree, item->util = item; /* better a valid pointer than a fake one */ } - return report_path_error(m, pattern, prefix); + ret = report_path_error(m, pattern, prefix); + free(m); + return ret; } static void add_remove_files(struct string_list *list) From 067178ed8a7822e6bc88ad606b707fc33658e6fc Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 23 Mar 2015 10:58:00 -0700 Subject: [PATCH 08/10] add_to_index(): free unused cache-entry We allocate a cache-entry pretty early in the function and then decide either not to do anything when we are pretending to add, or add it and then get an error (another possibility is obviously to succeed). When pretending or failing to add, we forgot to free the cache-entry. Noticed during a discussion on Stefan's patch to change the coding style without fixing the issue ;-) Signed-off-by: Junio C Hamano --- read-cache.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/read-cache.c b/read-cache.c index 60abec6055..5b922fd583 100644 --- a/read-cache.c +++ b/read-cache.c @@ -707,9 +707,11 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, ce->ce_mode == alias->ce_mode); if (pretend) - ; - else if (add_index_entry(istate, ce, add_option)) - return error("unable to add %s to index",path); + free(ce); + else if (add_index_entry(istate, ce, add_option)) { + free(ce); + return error("unable to add %s to index", path); + } if (verbose && !was_same) printf("add '%s'\n", path); return 0; From 915e44c6357f3bd9d5fa498a201872c4367302d3 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Mon, 23 Mar 2015 10:57:11 -0700 Subject: [PATCH 09/10] read-cache: fix memleak `ce` is allocated in make_cache_entry and should be freed if it is not used any more. refresh_cache_entry as a wrapper around refresh_cache_ent will either return - the `ce` given as the parameter, when it was up-to-date; - a new updated cache entry which is allocated to new memory; or - a NULL when refreshing failed. In the latter two cases, the original cache-entry `ce` is not used and needs to be freed. The rule can be expressed as "if the return value from refresh is different from the original ce, ce is no longer used." Helped-by: Junio C Hamano Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- read-cache.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/read-cache.c b/read-cache.c index 5b922fd583..0052b72d9c 100644 --- a/read-cache.c +++ b/read-cache.c @@ -748,12 +748,9 @@ struct cache_entry *make_cache_entry(unsigned int mode, ce->ce_mode = create_ce_mode(mode); ret = refresh_cache_entry(ce, refresh_options); - if (!ret) { + if (ret != ce) free(ce); - return NULL; - } else { - return ret; - } + return ret; } int ce_same_name(const struct cache_entry *a, const struct cache_entry *b) From 826aed50cbb072d8f159e4c8ba0f9bd3df21a234 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Fri, 20 Mar 2015 17:28:06 -0700 Subject: [PATCH 10/10] http: release the memory of a http pack request as well The cleanup function is used in 4 places now and it's always safe to free up the memory as well. Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- http.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http.c b/http.c index 9c825afefd..4b179f6fc8 100644 --- a/http.c +++ b/http.c @@ -1462,6 +1462,7 @@ void release_http_pack_request(struct http_pack_request *preq) } preq->slot = NULL; free(preq->url); + free(preq); } int finish_http_pack_request(struct http_pack_request *preq)