{"thread":{"id":"38873","subject":"[PATCH 02/15] read-cache: Improve readability","startedAt":"2015-03-21T00:27:57Z","lastAt":"2015-03-24T21:20:31Z","messageCount":44,"participants":["Stefan Beller","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"258206","messageId":"1426897692-18322-1-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":null,"subject":"[PATCH 00/15] Fixing memory leaks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:27:57Z","receivedAt":"2015-03-21T00:27:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"So here comes another bunch of mem leak fixes. While \nI consider the first 10 patches a pretty safe bet,\nthe last 5 hopefully spark a discussion if we want \npatches which just clean up before the program ends.\n\nStefan Beller (15):\n read-cache: fix memleak\n read-cache: Improve readability\n read-cache: free cache entry in add_to_index in case of early return\n update-index: fix a memleak\n builtin/apply.c: fix a memleak\n merge-blobs.c: Fix a memleak\n merge-recursive: fix memleaks\n http-push: Remove unneeded cleanup\n http: release the memory of a http pack request as well\n commit.c: fix a memory leak\n builtin/check-attr: fix a memleak\n builtin/merge-base: fix memleak\n builtin/unpack-file: fix a memleak\n builtin/cat-file: free memleak\n ls-files: fix a memleak\n\n builtin/apply.c | 1 +\n builtin/cat-file.c | 1 +\n builtin/check-attr.c | 2 ++\n builtin/commit.c | 6 ++++--\n builtin/ls-files.c | 1 +\n builtin/merge-base.c | 1 +\n builtin/unpack-file.c | 1 +\n builtin/update-index.c | 1 +\n http-push.c | 1 -\n http.c | 1 +\n merge-blobs.c | 4 +++-\n merge-recursive.c | 3 +++\n read-cache.c | 13 ++++++++-----\n 13 files changed, 27 insertions(+), 9 deletions(-)\n\n-- \n2.3.0.81.gc37f363\n"},{"id":"258215","messageId":"1426897692-18322-2-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 01/15] read-cache: fix memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:27:58Z","receivedAt":"2015-03-21T00:27:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`ce` is allocated in make_cache_entry and should be freed if it is not\nused any more. refresh_cache_entry as a wrapper around refresh_cache_ent\nwill either return `ce` or a new updated cache entry which is allocated\nto new memory. In that case we need to free `ce` ourselfs.\n\nSigned-off-by: Stefan Beller \n---\n read-cache.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 8d71860..f72ea9f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -747,6 +747,8 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \t\tfree(ce);\n \t\treturn NULL;\n \t} else {\n+\t\tif (ret != ce)\n+\t\t\tfree(ce);\n \t\treturn ret;\n \t}\n }\n-- \n2.3.0.81.gc37f363\n"},{"id":"258203","messageId":"1426897692-18322-3-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 02/15] read-cache: Improve readability","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:27:59Z","receivedAt":"2015-03-21T00:27:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n read-cache.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex f72ea9f..769897e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t\t !hashcmp(alias->sha1, ce->sha1) &&\n \t\t ce->ce_mode == alias->ce_mode);\n \n-\tif (pretend)\n-\t\t;\n-\telse if (add_index_entry(istate, ce, add_option))\n+\tif (!pretend && add_index_entry(istate, ce, add_option))\n \t\treturn error(\"unable to add %s to index\",path);\n \tif (verbose && !was_same)\n \t\tprintf(\"add '%s'\\n\", path);\n-- \n2.3.0.81.gc37f363\n"},{"id":"258214","messageId":"1426897692-18322-4-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:00Z","receivedAt":"2015-03-21T00:28:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This frees `ce` would be leaking in the error path.\n\nAdditionally a free is moved towards the return. This helps code\nreadability as we often have this pattern of freeing resources just\nbefore return/exit and not in between the code.\n\nSigned-off-by: Stefan Beller \n---\n read-cache.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 769897e..935f91a 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -681,15 +681,18 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \talias = index_file_exists(istate, ce->name, ce_namelen(ce), ignore_case);\n \tif (alias && !ce_stage(alias) && !ie_match_stat(istate, alias, st, ce_option)) {\n \t\t/* Nothing changed, really */\n-\t\tfree(ce);\n \t\tif (!S_ISGITLINK(alias->ce_mode))\n \t\t\tce_mark_uptodate(alias);\n \t\talias->ce_flags |= CE_ADDED;\n+\n+\t\tfree(ce);\n \t\treturn 0;\n \t}\n \tif (!intent_only) {\n-\t\tif (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT))\n+\t\tif (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT)) {\n+\t\t\tfree(ce);\n \t\t\treturn error(\"unable to index file %s\", path);\n+\t\t}\n \t} else\n \t\tset_object_name_for_intent_to_add_entry(ce);\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258217","messageId":"1426897692-18322-5-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 04/15] update-index: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:01Z","receivedAt":"2015-03-21T00:28:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`old` is not used outside the loop and would get lost\nonce we reach the goto.\n\nSigned-off-by: Stefan Beller \n---\n builtin/update-index.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 5878986..6271b54 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -584,6 +584,7 @@ static int do_reupdate(int ac, const char **av,\n \t\tpath = xstrdup(ce->name);\n \t\tupdate_one(path);\n \t\tfree(path);\n+\t\tfree(old);\n \t\tif (save_nr != active_nr)\n \t\t\tgoto redo;\n \t}\n-- \n2.3.0.81.gc37f363\n"},{"id":"258204","messageId":"1426897692-18322-6-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 05/15] builtin/apply.c: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:02Z","receivedAt":"2015-03-21T00:28:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"oldlines is allocated earlier in the function and also freed on the\nsuccessful code path.\n\nSigned-off-by: Stefan Beller \n---\n builtin/apply.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 65b97ee..e152c4d 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2776,6 +2776,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\tdefault:\n \t\t\tif (apply_verbosely)\n \t\t\t\terror(_(\"invalid start of line: '%c'\"), first);\n+\t\t\tfree(oldlines);\n \t\t\treturn -1;\n \t\t}\n \t\tif (added_blank_line) {\n-- \n2.3.0.81.gc37f363\n"},{"id":"258218","messageId":"1426897692-18322-7-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 06/15] merge-blobs.c: Fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:03Z","receivedAt":"2015-03-21T00:28:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n merge-blobs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-blobs.c b/merge-blobs.c\nindex 57211bc..7abb894 100644\n--- a/merge-blobs.c\n+++ b/merge-blobs.c\n@@ -14,8 +14,10 @@ static int fill_mmfile_blob(mmfile_t *f, struct blob *obj)\n \tbuf = read_sha1_file(obj->object.sha1, &type, &size);\n \tif (!buf)\n \t\treturn -1;\n-\tif (type != OBJ_BLOB)\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n \t\treturn -1;\n+\t}\n \tf->ptr = buf;\n \tf->size = size;\n \treturn 0;\n-- \n2.3.0.81.gc37f363\n"},{"id":"258212","messageId":"1426897692-18322-8-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 07/15] merge-recursive: fix memleaks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:04Z","receivedAt":"2015-03-21T00:28:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Usually when using string lists it looks like:\n struct string_list a = STRING_LIST_INIT_NODUP;\n // do stuff with a such as\n string_list_insert(&a, \"test string\");\n print_string_list(&a, \"test prefix\");\n // Cleaning up works on everything inside the struct, not on the\n // struct itself:\n string_list_clear(&a);\n\nBut as we deal with the pointers to the string lists directly, we also\nneed to free the actual struct.\n\nSigned-off-by: Stefan Beller \n---\n merge-recursive.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 771f5e2..1c9c30d 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1858,6 +1858,9 @@ int merge_trees(struct merge_options *o,\n \t\tstring_list_clear(re_head, 0);\n \t\tstring_list_clear(entries, 1);\n \n+\t\tfree(re_merge);\n+\t\tfree(re_head);\n+\t\tfree(entries);\n \t}\n \telse\n \t\tclean = 1;\n-- \n2.3.0.81.gc37f363\n"},{"id":"258205","messageId":"1426897692-18322-9-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 08/15] http-push: Remove unneeded cleanup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:05Z","receivedAt":"2015-03-21T00:28:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"preq is NULL as the condition the line before dictates. And the cleanup\nfunction release_http_pack_request is not null pointer safe.\n\nSigned-off-by: Stefan Beller \n---\n http-push.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex bfb1c96..c98dad2 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -316,7 +316,6 @@ static void start_fetch_packed(struct transfer_request *request)\n \n \tpreq = new_http_pack_request(target, repo->url);\n \tif (preq == NULL) {\n-\t\trelease_http_pack_request(preq);\n \t\trepo->can_update_info_refs = 0;\n \t\treturn;\n \t}\n-- \n2.3.0.81.gc37f363\n"},{"id":"258216","messageId":"1426897692-18322-10-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 09/15] http: release the memory of a http pack request as well","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:06Z","receivedAt":"2015-03-21T00:28:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The cleanup function is used in 4 places now and it's always safe to\nfree up the memory as well.\n\nSigned-off-by: Stefan Beller \n---\n http.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/http.c b/http.c\nindex 9c825af..4b179f6 100644\n--- a/http.c\n+++ b/http.c\n@@ -1462,6 +1462,7 @@ void release_http_pack_request(struct http_pack_request *preq)\n \t}\n \tpreq->slot = NULL;\n \tfree(preq->url);\n+\tfree(preq);\n }\n \n int finish_http_pack_request(struct http_pack_request *preq)\n-- \n2.3.0.81.gc37f363\n"},{"id":"258207","messageId":"1426897692-18322-11-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 10/15] commit.c: fix a memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:07Z","receivedAt":"2015-03-21T00:28:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n builtin/commit.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 961e467..da79ac4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -229,7 +229,7 @@ static int commit_index_files(void)\n static int list_paths(struct string_list *list, const char *with_tree,\n \t\t const char *prefix, const struct pathspec *pattern)\n {\n-\tint i;\n+\tint i, ret;\n \tchar *m;\n \n \tif (!pattern->nr)\n@@ -256,7 +256,9 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \t\t\titem->util = item; /* better a valid pointer than a fake one */\n \t}\n \n-\treturn report_path_error(m, pattern, prefix);\n+\tret = report_path_error(m, pattern, prefix);\n+\tfree(m);\n+\treturn ret;\n }\n \n static void add_remove_files(struct string_list *list)\n-- \n2.3.0.81.gc37f363\n"},{"id":"258209","messageId":"1426897692-18322-12-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 11/15] builtin/check-attr: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:08Z","receivedAt":"2015-03-21T00:28:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nNotes:\n I do not quite recall a discussion about such fixes\n when I started doing these fixes like 2 years ago.\n \n As this is a main function of a subcommand the freed\n memory is likely to have no impact as the process\n is done soon, so then it gets freed by the OS which\n is likey to be faster as the OS frees the whole pages\n of the process. Also there is no expected memory\n shortage as the process is going to be done soon\n as opposed to fixing mem leaks early in the process.\n \n An upside of fixes like this one is however to make\n code analysis tools produce less noise, so narrowing\n down the *real* issue may be easier.\n \n I wonder if we could have a 'weak' free which does\n nothing if git is compiled regularly and actually\n frees the memory if it is build with a flag to tell\n it to do so. This would help finding the real issues\n as the noise goes down and it would still be 'fast'\n as it could be when compiled for productive use.\n \n On the other hand I don't like to have another\n 'invented here again' systemcall-ish function as\n it would clutter the code and you'd have to remember\n to use that weak free.\n \n I dunno.\n\n builtin/check-attr.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 21d2bed..fa96356 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -182,5 +182,7 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \t\t\tcheck_attr(prefix, cnt, check, argv[i]);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n+\n+\tfree(check);\n \treturn 0;\n }\n-- \n2.3.0.81.gc37f363\n"},{"id":"258210","messageId":"1426897692-18322-13-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 12/15] builtin/merge-base: fix memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:09Z","receivedAt":"2015-03-21T00:28:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nNotes:\n see discussion on previous patch.\n\n builtin/merge-base.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 08a8217..4a8ff02 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -22,6 +22,7 @@ static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n \t\tresult = result->next;\n \t}\n \n+\tfree(rev);\n \treturn 0;\n }\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258211","messageId":"1426897692-18322-14-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 13/15] builtin/unpack-file: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:10Z","receivedAt":"2015-03-21T00:28:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nNotes:\n This is for discussion again,\n see 2nd previous patch.\n \n However why do we close the fd when we know\n the program ends soon? So let's also\n free the memory.\n\n builtin/unpack-file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 1920029..ea5767e 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -17,6 +17,7 @@ static char *create_temp_file(unsigned char *sha1)\n \tif (write_in_full(fd, buf, size) != size)\n \t\tdie_errno(\"unable to write temp-file\");\n \tclose(fd);\n+\tfree(buf);\n \treturn path;\n }\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258213","messageId":"1426897692-18322-15-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 14/15] builtin/cat-file: free memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:11Z","receivedAt":"2015-03-21T00:28:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nNotes:\n see discussion\n\n builtin/cat-file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex df99df4..8de6b0d 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -104,6 +104,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\tdie(\"git cat-file %s: bad file\", obj_name);\n \n \twrite_or_die(1, buf, size);\n+\tfree(buf);\n \treturn 0;\n }\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258208","messageId":"1426897692-18322-16-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"[PATCH 15/15] ls-files: fix a memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T00:28:12Z","receivedAt":"2015-03-21T00:28:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nNotes:\n see discussion\n\n builtin/ls-files.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 914054d..306defa 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -590,6 +590,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tif (bad)\n \t\t\tfprintf(stderr, \"Did you forget to 'git add'?\\n\");\n \n+\t\tfree(ps_matched);\n \t\treturn bad ? 1 : 0;\n \t}\n \n-- \n2.3.0.81.gc37f363\n"},{"id":"258221","messageId":"xmqqa8z7c9xi.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 00/15] Fixing memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:21:29Z","receivedAt":"2015-03-21T03:21:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So here comes another bunch of mem leak fixes.\n\nLooked mostly sensible.\n"},{"id":"258222","messageId":"xmqq619vc9pl.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 01/15] read-cache: fix memleak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:26:14Z","receivedAt":"2015-03-21T03:26:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> `ce` is allocated in make_cache_entry and should be freed if it is not\n> used any more. refresh_cache_entry as a wrapper around refresh_cache_ent\n> will either return `ce` or a new updated cache entry which is allocated\n> to new memory. In that case we need to free `ce` ourselfs.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> read-cache.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 8d71860..f72ea9f 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -747,6 +747,8 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n> \t\tfree(ce);\n> \t\treturn NULL;\n> \t} else {\n> +\t\tif (ret != ce)\n> +\t\t\tfree(ce);\n> \t\treturn ret;\n> \t}\n> }\n\nGood, I vaguely recall that we did something similar in another\ncodepath that forgot the fact that refresh_cache_entry() may make\nthe incoming ce unnecessary.\n\nAs the rule is \"if ret is different from ce, then ce must be freed\"\nin this codepath, I wonder if this is easier to read:\n\n\tret = refresh_cache_entry(ce, ...);\n if (ret != ce)\n \tfree(ce);\n\treturn ret;\n"},{"id":"258223","messageId":"xmqq1tkjc9g6.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:31:53Z","receivedAt":"2015-03-21T03:31:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This frees `ce` would be leaking in the error path.\n\nAt this point ce is not yet added to the index, so it is clear it is\nsafe to free it---otherwise we will leak it. Good.\n\n> Additionally a free is moved towards the return.\n\nI am on the fence on this one between two schools and do not have a\nstrong preference. One school is to free as soon as you know you do\nnot need it, which is a valid stance to take. Another is, as you\ndid, not to care about the minimum necessary lifetime of the storage\nand free them all at the end, which is also valid. Technically, the\nformer could be more performant while the latter is easier on the\neyes.\n"},{"id":"258224","messageId":"xmqqwq2baui7.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-5-git-send-email-sbeller@google.com","subject":"Re: [PATCH 04/15] update-index: fix a memleak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:40:00Z","receivedAt":"2015-03-21T03:40:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> `old` is not used outside the loop and would get lost\n> once we reach the goto.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/update-index.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 5878986..6271b54 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -584,6 +584,7 @@ static int do_reupdate(int ac, const char **av,\n> \t\tpath = xstrdup(ce->name);\n> \t\tupdate_one(path);\n> \t\tfree(path);\n> +\t\tfree(old);\n\nThe change looks trivially correct.\n\nA tangent; I wonder if we want to make path a strbuf that is\ndeclared in the function scope, reset (not released) at each\niteration of the loop and released after the loop; keep allocating\nand freeing a small piece of string all the time feels somewhat\nwasteful.\n\nAlso, we might want to add to the \"Be careful\" comment to describe\nwhy this cannot just be a call to \"update_one(ce->name)\" that does\nnot use \"path\".\n"},{"id":"258225","messageId":"xmqqsiczau9p.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-6-git-send-email-sbeller@google.com","subject":"Re: [PATCH 05/15] builtin/apply.c: fix a memleak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:45:06Z","receivedAt":"2015-03-21T03:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> oldlines is allocated earlier in the function and also freed on the\n> successful code path.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/apply.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 65b97ee..e152c4d 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -2776,6 +2776,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n> \t\tdefault:\n> \t\t\tif (apply_verbosely)\n> \t\t\t\terror(_(\"invalid start of line: '%c'\"), first);\n> +\t\t\tfree(oldlines);\n> \t\t\treturn -1;\n\nGood.\n\nBy the way, aren't the following also leaking here?\n\n - the strbuf newlines that starts out as \"size\"\n - line[] arrays of preimage and postimage\n\n\n\n\n> \t\t}\n> \t\tif (added_blank_line) {\n"},{"id":"258226","messageId":"xmqqoannau4h.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-8-git-send-email-sbeller@google.com","subject":"Re: [PATCH 07/15] merge-recursive: fix memleaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:48:14Z","receivedAt":"2015-03-21T03:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Usually when using string lists it looks like:\n> struct string_list a = STRING_LIST_INIT_NODUP;\n> // do stuff with a such as\n> string_list_insert(&a, \"test string\");\n> print_string_list(&a, \"test prefix\");\n> // Cleaning up works on everything inside the struct, not on the\n> // struct itself:\n> string_list_clear(&a);\n>\n> But as we deal with the pointers to the string lists directly, we also\n> need to free the actual struct.\n\nIn other words, these two were allocated for the sole use of this\nfuction by get_renames(), so this function is responsible for\nfreeing it?\n\nSounds sensible.\n\n> Signed-off-by: Stefan Beller \n> ---\n> merge-recursive.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 771f5e2..1c9c30d 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1858,6 +1858,9 @@ int merge_trees(struct merge_options *o,\n> \t\tstring_list_clear(re_head, 0);\n> \t\tstring_list_clear(entries, 1);\n> \n> +\t\tfree(re_merge);\n> +\t\tfree(re_head);\n> +\t\tfree(entries);\n> \t}\n> \telse\n> \t\tclean = 1;\n"},{"id":"258227","messageId":"xmqqk2ybatm1.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-11-git-send-email-sbeller@google.com","subject":"Re: [PATCH 10/15] commit.c: fix a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T03:59:18Z","receivedAt":"2015-03-21T03:59:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/commit.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 961e467..da79ac4 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -229,7 +229,7 @@ static int commit_index_files(void)\n> static int list_paths(struct string_list *list, const char *with_tree,\n> \t\t const char *prefix, const struct pathspec *pattern)\n> {\n> -\tint i;\n> +\tint i, ret;\n> \tchar *m;\n> \n> \tif (!pattern->nr)\n> @@ -256,7 +256,9 @@ static int list_paths(struct string_list *list, const char *with_tree,\n> \t\t\titem->util = item; /* better a valid pointer than a fake one */\n> \t}\n> \n> -\treturn report_path_error(m, pattern, prefix);\n> +\tret = report_path_error(m, pattern, prefix);\n> +\tfree(m);\n> +\treturn ret;\n> }\n\nLooks correct.\n\nA tangent. We may want to move report_path_error() to somewhere\nmore \"common\"-ish, like dir.c which is where we have bulk of\npathspec matching infrastructure. Seeing the function used from\nbuiltin/ls-files.c makes me feel dirty.\n\nA further tangent (Duy Cc'ed for this point). We might want to\nrethink the interface to ce_path_match() and report_path_error()\nso that we do not have to do a separate allocation of \"has this\npathspec been used?\" array. This was a remnant from the olden days\nback when pathspec were mere \"const char **\" where we did not have\nany room to add per-item bit---these days pathspec is repreasented\nas an array of \"struct pathspec\" and we can afford to add a bit\nto the structure---which will make this kind of leak much less\nlikely to happen.\n"},{"id":"258228","messageId":"xmqqfv8zathi.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-12-git-send-email-sbeller@google.com","subject":"Re: [PATCH 11/15] builtin/check-attr: fix a memleak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T04:02:01Z","receivedAt":"2015-03-21T04:02:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Notes:\n> I do not quite recall a discussion about such fixes\n> when I started doing these fixes like 2 years ago.\n> \n> As this is a main function of a subcommand the freed\n> memory is likely to have no impact as the process\n> is done soon, so then it gets freed by the OS which\n> is likey to be faster as the OS frees the whole pages\n> of the process.\n\nYou seem to understand it well without \"recalling\" ;-)\n\nAs you said, unless the false hits from valgrind on these overwhelm\nthe real issues we are looking for, it is usually not worth the\npatch churn to fix these. Another valid reason is if we plan to\nlibify a function that used to be a top-level.\n"},{"id":"258229","messageId":"xmqqbnjnaso7.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 02/15] read-cache: Improve readability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-21T04:19:36Z","receivedAt":"2015-03-21T04:19:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n> read-cache.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index f72ea9f..769897e 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n> \t\t !hashcmp(alias->sha1, ce->sha1) &&\n> \t\t ce->ce_mode == alias->ce_mode);\n> \n> -\tif (pretend)\n> -\t\t;\n> -\telse if (add_index_entry(istate, ce, add_option))\n> +\tif (!pretend && add_index_entry(istate, ce, add_option))\n> \t\treturn error(\"unable to add %s to index\",path);\n> \tif (verbose && !was_same)\n> \t\tprintf(\"add '%s'\\n\", path);\n\nI have a moderately strong feeling against this change, as the code\nwas done this way quite deliberately to keep it readable, namely, to\navoid using && to concatenate two boolean expressions that are in\ntotally different class inside condition part of if/while, where A\nis a precondition guard for B (i.e. you cannot evaluate B unless A\nholds) and B is called primarily for its side effect. The problem\nis that, once you start liberally doing\n\n\tif (A && B && C && D ...)\n\nwith booleans with mixed semantics (guards and actions), it will\nquickly get harder to tell which one is which.\n\nI could have written it as\n\n\tif (!pretend) {\n \tif (add_index_entry(...))\n\t\t\treturn error(...);\n\t}\n\nand that would have been just as readable as the original; it\nclearly separates the guard (i.e. only do the add-index thing when\nwe are not pretending) and the operation that is done for the side\neffect.\n\nBut I find the original tells you \"if pretend mode, do *nothing*\"\nand \"otherwise, try add_index_entry() and act on its error\" very\nclearly. Of course, I am biased as the original is my code from\n38ed1d89 (\"git-add -n -u\" should not add but just report,\n2008-05-21).\n\nFYI, between preference and taste, I'd say this one is much closer\nto the latter than the former.\n\nBy the way, aren't we leaking ce when we are merely pretending?\n"},{"id":"258230","messageId":"CAGZ79kYVWUofm+Hc2tWv8vLBxc01HqCGM=efYyb0jdGB-64zBg@mail.gmail.com","threadId":"38873","inReplyTo":"xmqq1tkjc9g6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T05:10:44Z","receivedAt":"2015-03-21T05:10:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 20, 2015 at 8:31 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> This frees `ce` would be leaking in the error path.\n>\n> At this point ce is not yet added to the index, so it is clear it is\n> safe to free it---otherwise we will leak it. Good.\n>\n>> Additionally a free is moved towards the return.\n>\n> I am on the fence on this one between two schools and do not have a\n> strong preference. One school is to free as soon as you know you do\n> not need it, which is a valid stance to take. Another is, as you\n> did, not to care about the minimum necessary lifetime of the storage\n> and free them all at the end, which is also valid. Technically, the\n> former could be more performant while the latter is easier on the\n> eyes.\n\nI only recall to have seen the latter school so far, which is why I\nmade the change in the first place assuming the school of freeing\nASAP has no strong supporters inside the git community.\n\nI can resend the patch dropping the reordering, if you prefer.\n"},{"id":"258231","messageId":"CAGZ79kaLJgC6rWdRSeVVQSBYn0soJuzqnrK_bNtgAPWimPYE8w@mail.gmail.com","threadId":"38873","inReplyTo":"xmqqbnjnaso7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 02/15] read-cache: Improve readability","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-21T05:11:47Z","receivedAt":"2015-03-21T05:11:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 20, 2015 at 9:19 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> read-cache.c | 4 +---\n>> 1 file changed, 1 insertion(+), 3 deletions(-)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index f72ea9f..769897e 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n>> !hashcmp(alias->sha1, ce->sha1) &&\n>> ce->ce_mode == alias->ce_mode);\n>>\n>> - if (pretend)\n>> - ;\n>> - else if (add_index_entry(istate, ce, add_option))\n>> + if (!pretend && add_index_entry(istate, ce, add_option))\n>> return error(\"unable to add %s to index\",path);\n>> if (verbose && !was_same)\n>> printf(\"add '%s'\\n\", path);\n>\n> I have a moderately strong feeling against this change, as the code\n> was done this way quite deliberately to keep it readable, namely, to\n> avoid using && to concatenate two boolean expressions that are in\n> totally different class inside condition part of if/while, where A\n> is a precondition guard for B (i.e. you cannot evaluate B unless A\n> holds) and B is called primarily for its side effect. The problem\n> is that, once you start liberally doing\n>\n> if (A && B && C && D ...)\n>\n> with booleans with mixed semantics (guards and actions), it will\n> quickly get harder to tell which one is which.\n>\n> I could have written it as\n>\n> if (!pretend) {\n> if (add_index_entry(...))\n> return error(...);\n> }\n\nThis makes sense to point out the different semantics to me.\nMaybe I have read too much of the refs code lately as there we\nhave these long chains which combine precondition with error\nchecking. :/ That's why I thought it would be global to git to not\ncare much about this semantics distinction.\n\n>\n> and that would have been just as readable as the original; it\n> clearly separates the guard (i.e. only do the add-index thing when\n> we are not pretending) and the operation that is done for the side\n> effect.\n>\n> But I find the original tells you \"if pretend mode, do *nothing*\"\n> and \"otherwise, try add_index_entry() and act on its error\" very\n> clearly. Of course, I am biased as the original is my code from\n> 38ed1d89 (\"git-add -n -u\" should not add but just report,\n> 2008-05-21).\n>\n> FYI, between preference and taste, I'd say this one is much closer\n> to the latter than the former.\n>\n> By the way, aren't we leaking ce when we are merely pretending?\n\nYes we are, that's how I found this spot. (coverity pointed out ce was\nleaking, so I was refactoring to actually make it easier to fix it, and then\nheavily reordered the patch series afterwards. That spot was forgotten\napparently.\n\n>\n>\n"},{"id":"258305","messageId":"xmqqa8z4dezs.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"CAGZ79kYVWUofm+Hc2tWv8vLBxc01HqCGM=efYyb0jdGB-64zBg@mail.gmail.com","subject":"Re: [PATCH 03/15] read-cache: free cache entry in add_to_index in case of early return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T19:11:19Z","receivedAt":"2015-03-22T19:11:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Fri, Mar 20, 2015 at 8:31 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> This frees `ce` would be leaking in the error path.\n>>\n>> At this point ce is not yet added to the index, so it is clear it is\n>> safe to free it---otherwise we will leak it. Good.\n>>\n>>> Additionally a free is moved towards the return.\n>>\n>> I am on the fence on this one between two schools and do not have a\n>> strong preference. One school is to free as soon as you know you do\n>> not need it, which is a valid stance to take. Another is, as you\n>> did, not to care about the minimum necessary lifetime of the storage\n>> and free them all at the end, which is also valid. Technically, the\n>> former could be more performant while the latter is easier on the\n>> eyes.\n>\n> I only recall to have seen the latter school so far, which is why I\n> made the change in the first place assuming the school of freeing\n> ASAP has no strong supporters inside the git community.\n>\n> I can resend the patch dropping the reordering, if you prefer.\n\nI already said that I do not have a preference ;-)\n\nWill queue 3/15 as-is, drop 2/15 and wait on 1/15.\n"},{"id":"258307","messageId":"xmqq619sdeb9.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"CAGZ79kaLJgC6rWdRSeVVQSBYn0soJuzqnrK_bNtgAPWimPYE8w@mail.gmail.com","subject":"Re: [PATCH 02/15] read-cache: Improve readability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T19:26:02Z","receivedAt":"2015-03-22T19:26:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Maybe I have read too much of the refs code lately as there we\n> have these long chains which combine precondition with error\n> checking.\n\nOf course, things are not so black-and-white in the real world. \n\nYou can also take an extreme stance and view\n\n\tif (!pretend && do_it_and_report_error())\n \terror(...);\n\ndifferently. I would explain that what the whole thing is trying to\nachieve as \"'do it' part is the primary thing we want to do, but it\nonly can be done when we are not pretending, and we show an error\nwhen the 'do it' part fails.\" and would suggest structuring it more\nlike this:\n\n\tif (pretend)\n \t; /* nothing */\n\telse if (do_it_and_report_error())\n \terror(...);\n\nor\n\n\tif (!pretend) {\n \tif (do_it_and_report_error())\n \terror(...);\n\t}\n\nBut you could say \"The final objective of the whole thing is to show\nan error message, but if we are pretending or if our attempt to 'do\nit' succeeds, we do not have to show the error\", and such a view may\nmake sense depending on what that 'do it' is. The original may be\njustified under such an interpretation.\n\nWe may be tempted to write (note: each boolean term may be a call\nwith many complex arguments)\n\n if (A && B && C && D && E)\n\t...\n\nwhen it is in fact logically is more like this:\n\n /* does not make sense to attempt C when A && B does not hold */\n if (A && B) {\n \tif (C && D && E)\n \t...\n }\n\nBut it becomes a judgement call if splitting that into nested two if\nstatements is better for overall readability when the top-level if\nstatement has an else clause. You cannot turn it into\n\n /* does not make sense to attempt C when A && B does not hold */\n if (A && B) {\n \tif (C && D && E)\n \t...\n } else {\n ... /* this cannot be what the original's else clause did */\n }\n\nblindly. It would need further restructuring. I think the code\ninside the refs.c is a result of making such judgement calls.\n\n>> By the way, aren't we leaking ce when we are merely pretending?\n>\n> Yes we are, that's how I found this spot. (coverity pointed out ce was\n> leaking, so I was refactoring to actually make it easier to fix it, and then\n> heavily reordered the patch series afterwards. That spot was forgotten\n> apparently.\n\nI dropped 2/15 and expect the real fix in the future; no rush,\nthough.\n"},{"id":"258309","messageId":"xmqq1tkgddto.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1426897692-18322-10-git-send-email-sbeller@google.com","subject":"Re: [PATCH 09/15] http: release the memory of a http pack request as well","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-22T19:36:35Z","receivedAt":"2015-03-22T19:36:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The cleanup function is used in 4 places now and it's always safe to\n> free up the memory as well.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> http.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/http.c b/http.c\n> index 9c825af..4b179f6 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1462,6 +1462,7 @@ void release_http_pack_request(struct http_pack_request *preq)\n> \t}\n> \tpreq->slot = NULL;\n> \tfree(preq->url);\n> +\tfree(preq);\n> }\n> \n> int finish_http_pack_request(struct http_pack_request *preq)\n\nFreeing of preq in all the callers of this one looks sensible,\nexcept for the one in finish_request() of http-push.c that pulls an\npreq instance out of request->userData.\n\nCan somebody help me follow the dataflow to convince me that this is\nnot leading to double-free in start_fetch_packed()?\n\nThanks.\n"},{"id":"258368","messageId":"CAGZ79kY=b3=ppmB+eJOAgcUec7hq+ycJiCvBzkTc7U78Mgog_g@mail.gmail.com","threadId":"38873","inReplyTo":"xmqq619vc9pl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 01/15] read-cache: fix memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-23T16:24:16Z","receivedAt":"2015-03-23T16:24:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Mar 20, 2015 at 8:26 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> `ce` is allocated in make_cache_entry and should be freed if it is not\n>> used any more. refresh_cache_entry as a wrapper around refresh_cache_ent\n>> will either return `ce` or a new updated cache entry which is allocated\n>> to new memory. In that case we need to free `ce` ourselfs.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> read-cache.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 8d71860..f72ea9f 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -747,6 +747,8 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n>> free(ce);\n>> return NULL;\n>> } else {\n>> + if (ret != ce)\n>> + free(ce);\n>> return ret;\n>> }\n>> }\n>\n> Good, I vaguely recall that we did something similar in another\n> codepath that forgot the fact that refresh_cache_entry() may make\n> the incoming ce unnecessary.\n>\n> As the rule is \"if ret is different from ce, then ce must be freed\"\n> in this codepath, I wonder if this is easier to read:\n>\n> ret = refresh_cache_entry(ce, ...);\n> if (ret != ce)\n> free(ce);\n> return ret;\n>\n\nIndeed it is. Thanks for pointing out!\n\nI'll send this as part of another series\napplying on top of sb/leaks soon.\n"},{"id":"258370","messageId":"1427129620-13380-1-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"xmqqwq2baui7.fsf@gitster.dls.corp.google.com","subject":"[PATCH] update-index: Don't copy memory around","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-23T16:53:40Z","receivedAt":"2015-03-23T16:53:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\nOn Fri, Mar 20, 2015 at 8:40 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> `old` is not used outside the loop and would get lost\n>> once we reach the goto.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>>  builtin/update-index.c | 1 +\n>>  1 file changed, 1 insertion(+)\n>>\n>> diff --git a/builtin/update-index.c b/builtin/update-index.c\n>> index 5878986..6271b54 100644\n>> --- a/builtin/update-index.c\n>> +++ b/builtin/update-index.c\n>> @@ -584,6 +584,7 @@ static int do_reupdate(int ac, const char **av,\n>>               path = xstrdup(ce->name);\n>>               update_one(path);\n>>               free(path);\n>> +             free(old);\n>\n> The change looks trivially correct.\n>\n> A tangent; I wonder if we want to make path a strbuf that is\n> declared in the function scope, reset (not released) at each\n> iteration of the loop and released after the loop; keep allocating\n> and freeing a small piece of string all the time feels somewhat\n> wasteful.\n\n> Also, we might want to add to the \"Be careful\" comment to describe\n> why this cannot just be a call to \"update_one(ce->name)\" that does\n> not use \"path\".\n\nIndeed I am rather wondering why we need to pass in a copy to update_one\ninstead of ce->name. I was reading update_one and looking for why, but\neventually noticed update_one accepts a 'const char *', so it's not changed\nin there. Digging down the into update_one also doesn't make it obvious to me.\n\nI found (5699d17ee094, 2013-11-14, read-cache.c: fix memory leaks caused by\nremoved cache entries), which adds this duplication, though I do not understand\nwhy. This passes the test suite, so I wonder if this patch would be a subtle bug\nnow.\n\n builtin/update-index.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 6271b54..5857405 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -564,7 +564,6 @@ static int do_reupdate(int ac, const char **av,\n \t\tconst struct cache_entry *ce = active_cache[pos];\n \t\tstruct cache_entry *old = NULL;\n \t\tint save_nr;\n-\t\tchar *path;\n \n \t\tif (ce_stage(ce) || !ce_path_match(ce, &pathspec, NULL))\n \t\t\tcontinue;\n@@ -581,9 +580,7 @@ static int do_reupdate(int ac, const char **av,\n \t\t * or worse yet 'allow_replace', active_nr may decrease.\n \t\t */\n \t\tsave_nr = active_nr;\n-\t\tpath = xstrdup(ce->name);\n-\t\tupdate_one(path);\n-\t\tfree(path);\n+\t\tupdate_one(ce->name);\n \t\tfree(old);\n \t\tif (save_nr != active_nr)\n \t\t\tgoto redo;\n-- \n2.3.0.81.gc37f363\n"},{"id":"258371","messageId":"xmqq1tkfbpw3.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1427129620-13380-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] update-index: Don't copy memory around","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T17:11:08Z","receivedAt":"2015-03-23T17:11:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> ... though I do not understand\n> why. This passes the test suite, so I wonder if this patch would be a subtle bug\n> now.\n\nI recall the last time I traced the code I noticed that the ce can\nget passed to a codepath that causes its removal from update_one(),\nand then the pathname itself (which used to be ce->name but remember\nthat ce is invalid already at that point!) is still used after ce is\nremoved, which was the reason why you would introduce a bug if you\nstop copying the path. You may have to follow the code again a bit\ncloser to make sure but I am reasonably sure that was the reason.\n"},{"id":"258372","messageId":"1427130811-21222-1-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"xmqqsiczau9p.fsf@gitster.dls.corp.google.com","subject":"[PATCH] builtin/apply.c: fix a memleak (Fixup, squashable)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-23T17:13:31Z","receivedAt":"2015-03-23T17:13:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\n> By the way, aren't the following also leaking here?\n>\n>  - the strbuf newlines that starts out as \"size\"\n>  - line[] arrays of preimage and postimage\n>\n\nyes :(\n\nI think a viable replacement may be below.\nThat way the returned value of the function is not -1, but 1.\nWe're using the return value only as a boolean, so it doesn't\nreally matter.\n\n builtin/apply.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n \ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex e152c4d..0769b09 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -2776,8 +2776,8 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\tdefault:\n \t\t\tif (apply_verbosely)\n \t\t\t\terror(_(\"invalid start of line: '%c'\"), first);\n-\t\t\tfree(oldlines);\n-\t\t\treturn -1;\n+\t\t\tapplied_pos = -1;\n+\t\t\tgoto out;\n \t\t}\n \t\tif (added_blank_line) {\n \t\t\tif (!new_blank_lines_at_end)\n@@ -2916,6 +2916,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n \t\t\t (int)(old - oldlines), oldlines);\n \t}\n \n+out:\n \tfree(oldlines);\n \tstrbuf_release(&newlines);\n \tfree(preimage.line_allocated);\n-- \n2.3.0.81.gc37f363\n"},{"id":"258373","messageId":"xmqqwq27aaku.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1427130811-21222-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] builtin/apply.c: fix a memleak (Fixup, squashable)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T17:27:13Z","receivedAt":"2015-03-23T17:27:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n>\n>> By the way, aren't the following also leaking here?\n>>\n>>  - the strbuf newlines that starts out as \"size\"\n>>  - line[] arrays of preimage and postimage\n>>\n>\n> yes :(\n>\n> I think a viable replacement may be below.\n\nYeah, that looks a lot more sensible and straight-forward.\n\nThanks.\n\n> That way the returned value of the function is not -1, but 1.\n> We're using the return value only as a boolean, so it doesn't\n> really matter.\n>\n> builtin/apply.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index e152c4d..0769b09 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -2776,8 +2776,8 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n> \t\tdefault:\n> \t\t\tif (apply_verbosely)\n> \t\t\t\terror(_(\"invalid start of line: '%c'\"), first);\n> -\t\t\tfree(oldlines);\n> -\t\t\treturn -1;\n> +\t\t\tapplied_pos = -1;\n> +\t\t\tgoto out;\n> \t\t}\n> \t\tif (added_blank_line) {\n> \t\t\tif (!new_blank_lines_at_end)\n> @@ -2916,6 +2916,7 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,\n> \t\t\t (int)(old - oldlines), oldlines);\n> \t}\n> \n> +out:\n> \tfree(oldlines);\n> \tstrbuf_release(&newlines);\n> \tfree(preimage.line_allocated);\n"},{"id":"258374","messageId":"1427133432-26703-1-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"CAGZ79kY=b3=ppmB+eJOAgcUec7hq+ycJiCvBzkTc7U78Mgog_g@mail.gmail.com","subject":"[PATCH 1/2] read-cache: fix memleak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-23T17:57:11Z","receivedAt":"2015-03-23T17:57:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`ce` is allocated in make_cache_entry and should be freed if it is not\nused any more. refresh_cache_entry as a wrapper around refresh_cache_ent\nwill either return `ce` or a new updated cache entry which is allocated\nto new memory. In that case we need to free `ce` ourselfs.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n read-cache.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 60abec6..a102565 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -746,12 +746,9 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \tce->ce_mode = create_ce_mode(mode);\n \n \tret = refresh_cache_entry(ce, refresh_options);\n-\tif (!ret) {\n+\tif (ret != ce)\n \t\tfree(ce);\n-\t\treturn NULL;\n-\t} else {\n-\t\treturn ret;\n-\t}\n+\treturn ret;\n }\n \n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n-- \n2.3.0.81.gc37f363\n"},{"id":"258375","messageId":"1427133432-26703-2-git-send-email-sbeller@google.com","threadId":"38873","inReplyTo":"1427133432-26703-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] read-cache.c: fix a memleak in add_to_index","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-23T17:57:12Z","receivedAt":"2015-03-23T17:57:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\n I have reread your remarks from the weekend, and I agree\n this looks more intuitive. Thanks for pointing out the subtle details\n to make programming an art!\n \n read-cache.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex a102565..f837212 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -706,10 +706,14 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t\t !hashcmp(alias->sha1, ce->sha1) &&\n \t\t ce->ce_mode == alias->ce_mode);\n \n-\tif (pretend)\n-\t\t;\n-\telse if (add_index_entry(istate, ce, add_option))\n-\t\treturn error(\"unable to add %s to index\",path);\n+\tif (!pretend) {\n+\t\tif (add_index_entry(istate, ce, add_option)) {\n+\t\t\tfree(ce);\n+\t\t\treturn error(\"unable to add %s to index\",path);\n+\t\t}\n+\t} else {\n+\t\tfree(ce);\n+\t}\n \tif (verbose && !was_same)\n \t\tprintf(\"add '%s'\\n\", path);\n \treturn 0;\n-- \n2.3.0.81.gc37f363\n"},{"id":"258376","messageId":"xmqqpp7za8q3.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1427133432-26703-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/2] read-cache.c: fix a memleak in add_to_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T18:07:16Z","receivedAt":"2015-03-23T18:07:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n>\n> I have reread your remarks from the weekend, and I agree\n> this looks more intuitive. Thanks for pointing out the subtle details\n> to make programming an art!\n\nHeh, Our mails crossed, I guess. I've done this myself like this\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Mon, 23 Mar 2015 10:58:00 -0700\nSubject: [PATCH] add_to_index(): free unused cache-entry\n\nWe allocate a cache-entry pretty early in the function and then\ndecide either not to do anything when we are pretending to add, or\nadd it and then get an error (another possibility is obviously to\nsucceed).\n\nWhen pretending or failing to add, we forgot to free the\ncache-entry.\n\nNoticed during a discussion on Stefan's patch to change the coding\nstyle without fixing the issue ;-)\n\nSigned-off-by: Junio C Hamano \n---\n read-cache.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 60abec6..5b922fd5 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -707,9 +707,11 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \t\t ce->ce_mode == alias->ce_mode);\n \n \tif (pretend)\n-\t\t;\n-\telse if (add_index_entry(istate, ce, add_option))\n-\t\treturn error(\"unable to add %s to index\",path);\n+\t\tfree(ce);\n+\telse if (add_index_entry(istate, ce, add_option)) {\n+\t\tfree(ce);\n+\t\treturn error(\"unable to add %s to index\", path);\n+\t}\n \tif (verbose && !was_same)\n \t\tprintf(\"add '%s'\\n\", path);\n \treturn 0;\n-- \n2.3.3-454-g85aa98f\n"},{"id":"258377","messageId":"xmqqlhina8ip.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"1427133432-26703-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/2] read-cache: fix memleak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T18:11:42Z","receivedAt":"2015-03-23T18:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> `ce` is allocated in make_cache_entry and should be freed if it is not\n> used any more. refresh_cache_entry as a wrapper around refresh_cache_ent\n> will either return `ce` or a new updated cache entry which is allocated\n> to new memory. In that case we need to free `ce` ourselfs.\n\nThanks. I started \"commit --amend\" to do s/ourselfs/ourselves/, but\nended up tweaking the description a bit more.\n\n-- .8 --\nFrom: Stefan Beller \nDate: Mon, 23 Mar 2015 10:57:11 -0700\nSubject: [PATCH] read-cache: fix memleak\n\n`ce` is allocated in make_cache_entry and should be freed if it is not\nused any more. refresh_cache_entry as a wrapper around refresh_cache_ent\nwill either return\n\n - the `ce` given as the parameter, when it was up-to-date;\n - a new updated cache entry which is allocated to new memory; or\n - a NULL when refreshing failed.\n\nIn the latter two cases, the original cache-entry `ce` is not used\nand needs to be freed. The rule can be expressed as \"if the return\nvalue from refresh is different from the original ce, ce is no\nlonger used.\"\n\nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n read-cache.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5b922fd5..0052b72 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -748,12 +748,9 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \tce->ce_mode = create_ce_mode(mode);\n \n \tret = refresh_cache_entry(ce, refresh_options);\n-\tif (!ret) {\n+\tif (ret != ce)\n \t\tfree(ce);\n-\t\treturn NULL;\n-\t} else {\n-\t\treturn ret;\n-\t}\n+\treturn ret;\n }\n \n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n-- \n2.3.3-454-g85aa98f\n"},{"id":"258409","messageId":"CACsJy8A3CptGYNAZ=+k0ykBCp6SGoOLY0nX20WkWQp=qUnxwWg@mail.gmail.com","threadId":"38873","inReplyTo":"xmqqk2ybatm1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/15] commit.c: fix a memory leak","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-03-24T13:42:17Z","receivedAt":"2015-03-24T13:42:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano wrote:\n> A further tangent (Duy Cc'ed for this point). We might want to\n> rethink the interface to ce_path_match() and report_path_error()\n> so that we do not have to do a separate allocation of \"has this\n> pathspec been used?\" array. This was a remnant from the olden days\n> back when pathspec were mere \"const char **\" where we did not have\n> any room to add per-item bit---these days pathspec is repreasented\n> as an array of \"struct pathspec\" and we can afford to add a bit\n> to the structure---which will make this kind of leak much less\n> likely to happen.\n\nI just want to say \"noted\" (and therefore in my backlog). But no\npromise that it will happen any time soon. Low hanging fruit, perhaps\nsome people may be interested..\n-- \nDuy\n"},{"id":"258422","messageId":"CAGZ79kZbe9d7wG+CJ0o93hDs3sFjkSp68pBHXpjOysbzgvjo3w@mail.gmail.com","threadId":"38873","inReplyTo":"xmqq1tkgddto.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 09/15] http: release the memory of a http pack request as well","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-24T16:54:07Z","receivedAt":"2015-03-24T16:54:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Mar 22, 2015 at 12:36 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> The cleanup function is used in 4 places now and it's always safe to\n>> free up the memory as well.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> http.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/http.c b/http.c\n>> index 9c825af..4b179f6 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -1462,6 +1462,7 @@ void release_http_pack_request(struct http_pack_request *preq)\n>> }\n>> preq->slot = NULL;\n>> free(preq->url);\n>> + free(preq);\n>> }\n>>\n>> int finish_http_pack_request(struct http_pack_request *preq)\n>\n> Freeing of preq in all the callers of this one looks sensible,\n> except for the one in finish_request() of http-push.c that pulls an\n> preq instance out of request->userData.\n>\n> Can somebody help me follow the dataflow to convince me that this is\n> not leading to double-free in start_fetch_packed()?\n\nI am not sure where you suspect the double free problem.\n\nIn start_fetch_packed we have a call to release_http_pack_request\n2 times but just in an error-out-and-cleanup case, so either of one cases\nis called.\n\nIn the latter place (http-push.c lines 335-347), we have code like\n request->userData = preq;\n if (!start_active_slot(preq->slot)) {\n release_http_pack_request(preq);\n repo->can_update_info_refs = 0;\n release_request(request);\n }\n\nDo you mean that the release_http_pack_request and release_request may collide\nas the `release_request(request);` has a pointer to preq via its userData field?\n\nWell there is hope, as `release_request` only touches\n free(request->url);\n free(request);\n\nand not the userData pointer.\n\n\nI am a bit puzzled what you're trying to hint at.\n\n>\n> Thanks.\n>\n"},{"id":"258433","messageId":"xmqqh9ta70dc.fsf@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"CAGZ79kZbe9d7wG+CJ0o93hDs3sFjkSp68pBHXpjOysbzgvjo3w@mail.gmail.com","subject":"Re: [PATCH 09/15] http: release the memory of a http pack request as well","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T17:48:15Z","receivedAt":"2015-03-24T17:48:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Well there is hope, as `release_request` only touches\n> free(request->url);\n> free(request);\n>\n> and not the userData pointer.\n\nOK.\n\n> I am a bit puzzled what you're trying to hint at.\n\nThe caller does this:\n\n static void start_fetch_packed(struct transfer_request *request)\n {\n ...\n preq = new_http_pack_request(target, repo->url);\n ...\n preq->slot->callback_func = process_response;\n preq->slot->callback_data = request;\n request->slot = preq->slot;\n request->userData = preq;\n\n /* Try to get the request started, abort the request on error */\n request->state = RUN_FETCH_PACKED;\n if (!start_active_slot(preq->slot)) {\n fprintf(stderr, \"Unable to start GET request\\n\");\n release_http_pack_request(preq);\n repo->can_update_info_refs = 0;\n release_request(request);\n }\n }\n\nand start_active_slot() actually not just \"starts\" but calls\ncurl_multi_perform() to do things, like calling process_response(),\nwhich has calls to release_{,http_pack_}request(). I didn't see\nthose releases and the releases we see in the above (i.e. when !start)\nwill not run at the same time (but I see it now ;-))\n\nIn short, not hinting at anything. I was genuinely having a hard time\nfollowing the codeflow.\n"},{"id":"258450","messageId":"xmqq7fu65c4y.fsf_-_@gitster.dls.corp.google.com","threadId":"38873","inReplyTo":"CACsJy8A3CptGYNAZ=+k0ykBCp6SGoOLY0nX20WkWQp=qUnxwWg@mail.gmail.com","subject":"Re* [PATCH 10/15] commit.c: fix a memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-24T21:17:01Z","receivedAt":"2015-03-24T21:17:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sat, Mar 21, 2015 at 10:59 AM, Junio C Hamano wrote:\n>> A further tangent (Duy Cc'ed for this point). We might want to\n>> rethink the interface to ce_path_match() and report_path_error()\n>> so that we do not have to do a separate allocation of \"has this\n>> pathspec been used?\" array. This was a remnant from the olden days\n>> back when pathspec were mere \"const char **\" where we did not have\n>> any room to add per-item bit---these days pathspec is repreasented\n>> as an array of \"struct pathspec\" and we can afford to add a bit\n>> to the structure---which will make this kind of leak much less\n>> likely to happen.\n>\n> I just want to say \"noted\" (and therefore in my backlog). But no\n> promise that it will happen any time soon. Low hanging fruit, perhaps\n> some people may be interested..\n\nOK, the other one I just did so that we won't forget. Otherwise we\nwill leave too many loose ends untied.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Tue, 24 Mar 2015 14:12:10 -0700\nSubject: [PATCH] report_path_error(): move to dir.c\n\nThe expected call sequence is for the caller to use match_pathspec()\nrepeatedly on a set of pathspecs, accumulating the \"hits\" in a\nseparate array, and then call this function to diagnose a pathspec\nthat never matched anything, as that can indicate a typo from the\ncommand line, e.g. \"git commit Maekfile\".\n\nMany builtin commands use this function from builtin/ls-files.c,\nwhich is not a very healthy arrangement. ls-files might have been\nthe first command to feel the need for such a helper, but the need\nis shared by everybody who uses the \"match and then report\" pattern.\n\nMove it to dir.c where match_pathspec() is defined.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/ls-files.c | 43 -------------------------------------------\n cache.h | 1 -\n dir.c | 43 +++++++++++++++++++++++++++++++++++++++++++\n dir.h | 1 +\n 4 files changed, 44 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 47c3880..47d70b2 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -354,49 +354,6 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)\n \t}\n }\n \n-int report_path_error(const char *ps_matched,\n-\t\t const struct pathspec *pathspec,\n-\t\t const char *prefix)\n-{\n-\t/*\n-\t * Make sure all pathspec matched; otherwise it is an error.\n-\t */\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint num, errors = 0;\n-\tfor (num = 0; num < pathspec->nr; num++) {\n-\t\tint other, found_dup;\n-\n-\t\tif (ps_matched[num])\n-\t\t\tcontinue;\n-\t\t/*\n-\t\t * The caller might have fed identical pathspec\n-\t\t * twice. Do not barf on such a mistake.\n-\t\t * FIXME: parse_pathspec should have eliminated\n-\t\t * duplicate pathspec.\n-\t\t */\n-\t\tfor (found_dup = other = 0;\n-\t\t !found_dup && other < pathspec->nr;\n-\t\t other++) {\n-\t\t\tif (other == num || !ps_matched[other])\n-\t\t\t\tcontinue;\n-\t\t\tif (!strcmp(pathspec->items[other].original,\n-\t\t\t\t pathspec->items[num].original))\n-\t\t\t\t/*\n-\t\t\t\t * Ok, we have a match already.\n-\t\t\t\t */\n-\t\t\t\tfound_dup = 1;\n-\t\t}\n-\t\tif (found_dup)\n-\t\t\tcontinue;\n-\n-\t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n-\t\t pathspec->items[num].original);\n-\t\terrors++;\n-\t}\n-\tstrbuf_release(&sb);\n-\treturn errors;\n-}\n-\n static const char * const ls_files_usage[] = {\n \tN_(\"git ls-files [options] [...]\"),\n \tNULL\ndiff --git a/cache.h b/cache.h\nindex f23fdbe..8ec0b65 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1411,7 +1411,6 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\n #define ws_tab_width(rule) ((rule) & WS_TAB_WIDTH_MASK)\n \n /* ls-files */\n-int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);\n void overlay_tree_on_cache(const char *tree_name, const char *prefix);\n \n char *alias_lookup(const char *alias);\ndiff --git a/dir.c b/dir.c\nindex 797805d..5d6e102 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -377,6 +377,49 @@ int match_pathspec(const struct pathspec *ps,\n \treturn negative ? 0 : positive;\n }\n \n+int report_path_error(const char *ps_matched,\n+\t\t const struct pathspec *pathspec,\n+\t\t const char *prefix)\n+{\n+\t/*\n+\t * Make sure all pathspec matched; otherwise it is an error.\n+\t */\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint num, errors = 0;\n+\tfor (num = 0; num < pathspec->nr; num++) {\n+\t\tint other, found_dup;\n+\n+\t\tif (ps_matched[num])\n+\t\t\tcontinue;\n+\t\t/*\n+\t\t * The caller might have fed identical pathspec\n+\t\t * twice. Do not barf on such a mistake.\n+\t\t * FIXME: parse_pathspec should have eliminated\n+\t\t * duplicate pathspec.\n+\t\t */\n+\t\tfor (found_dup = other = 0;\n+\t\t !found_dup && other < pathspec->nr;\n+\t\t other++) {\n+\t\t\tif (other == num || !ps_matched[other])\n+\t\t\t\tcontinue;\n+\t\t\tif (!strcmp(pathspec->items[other].original,\n+\t\t\t\t pathspec->items[num].original))\n+\t\t\t\t/*\n+\t\t\t\t * Ok, we have a match already.\n+\t\t\t\t */\n+\t\t\t\tfound_dup = 1;\n+\t\t}\n+\t\tif (found_dup)\n+\t\t\tcontinue;\n+\n+\t\terror(\"pathspec '%s' did not match any file(s) known to git.\",\n+\t\t pathspec->items[num].original);\n+\t\terrors++;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn errors;\n+}\n+\n /*\n * Return the length of the \"simple\" part of a path match limiter.\n */\ndiff --git a/dir.h b/dir.h\nindex 55e5345..ed336ad 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -135,6 +135,7 @@ extern char *common_prefix(const struct pathspec *pathspec);\n extern int match_pathspec(const struct pathspec *pathspec,\n \t\t\t const char *name, int namelen,\n \t\t\t int prefix, char *seen, int is_dir);\n+extern int report_path_error(const char *ps_matched, const struct pathspec *pathspec, const char *prefix);\n extern int within_depth(const char *name, int namelen, int depth, int max_depth);\n \n extern int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec);\n-- \n2.3.4-449-gebec173\n"},{"id":"258451","messageId":"CAGZ79kZrYn9C=asYoszjJY3PxYGGd=fJw3h0g92L=Hnr3MG4xg@mail.gmail.com","threadId":"38873","inReplyTo":"xmqq7fu65c4y.fsf_-_@gitster.dls.corp.google.com","subject":"Re: Re* [PATCH 10/15] commit.c: fix a memory leak","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-03-24T21:20:31Z","receivedAt":"2015-03-24T21:20:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 24, 2015 at 2:17 PM, Junio C Hamano wrote:\n>\n> Move it to dir.c where match_pathspec() is defined.\n>\n> Signed-off-by: Junio C Hamano \n\nReviewed-by: Stefan Beller \n"}]}