{"thread":{"id":"18682","subject":"[PATCH] builtin-clone.c: fix memory leak in cmd_clone()","startedAt":"2009-04-01T14:40:56Z","lastAt":"2009-04-01T20:17:04Z","messageCount":5,"participants":["Ali Gholami Rudi","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"110142","messageId":"20090401144056.GC2237@lilem.mirepesht","threadId":"18682","inReplyTo":null,"subject":"[PATCH] builtin-clone.c: fix memory leak in cmd_clone()","fromName":"Ali Gholami Rudi","fromEmail":"ali@rudi.ir","sentAt":"2009-04-01T14:40:56Z","receivedAt":"2009-04-01T14:40:56Z","isPatch":true,"sender":{"key":"ali@rudi.ir","avatar":null},"body":"With this patch, cmd_clone() safely frees its xstrdup()-allocated\nmemory.  Also junk_work_tree and junk_git_dir (used in remove_junk()\nwhich is called asynchronously) were changed to use static arrays rather\nthan sharing the memory allocated in cmd_clone().\n\nSigned-off-by: Ali Gholami Rudi <ali@rudi.ir>\n---\n builtin-clone.c |   22 +++++++++++++---------\n 1 files changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-clone.c b/builtin-clone.c\nindex 0031b5f..8149fc0 100644\n--- a/builtin-clone.c\n+++ b/builtin-clone.c\n@@ -268,8 +268,8 @@ static const struct ref *clone_local(const char *src_repo,\n \treturn ret;\n }\n \n-static const char *junk_work_tree;\n-static const char *junk_git_dir;\n+static char junk_work_tree[PATH_MAX];\n+static char junk_git_dir[PATH_MAX];\n pid_t junk_pid;\n \n static void remove_junk(void)\n@@ -277,12 +277,12 @@ static void remove_junk(void)\n \tstruct strbuf sb = STRBUF_INIT;\n \tif (getpid() != junk_pid)\n \t\treturn;\n-\tif (junk_git_dir) {\n+\tif (*junk_git_dir) {\n \t\tstrbuf_addstr(&sb, junk_git_dir);\n \t\tremove_dir_recursively(&sb, 0);\n \t\tstrbuf_reset(&sb);\n \t}\n-\tif (junk_work_tree) {\n+\tif (*junk_work_tree) {\n \t\tstrbuf_addstr(&sb, junk_work_tree);\n \t\tremove_dir_recursively(&sb, 0);\n \t\tstrbuf_reset(&sb);\n@@ -320,8 +320,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0;\n \tstruct stat buf;\n-\tconst char *repo_name, *repo, *work_tree, *git_dir;\n-\tchar *path, *dir;\n+\tconst char *repo_name, *work_tree;\n+\tchar *path, *dir, *repo, *git_dir;\n \tint dest_exists;\n \tconst struct ref *refs, *head_points_at, *remote_head, *mapped_refs;\n \tstruct strbuf key = STRBUF_INIT, value = STRBUF_INIT;\n@@ -362,7 +362,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \telse if (!strchr(repo_name, ':'))\n \t\trepo = xstrdup(make_absolute_path(repo_name));\n \telse\n-\t\trepo = repo_name;\n+\t\trepo = xstrdup(repo_name);\n \n \tif (argc == 2)\n \t\tdir = xstrdup(argv[1]);\n@@ -393,7 +393,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!option_bare) {\n-\t\tjunk_work_tree = work_tree;\n+\t\tstrcpy(junk_work_tree, work_tree);\n \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n \t\t\tdie(\"could not create leading directories of '%s': %s\",\n \t\t\t\t\twork_tree, strerror(errno));\n@@ -402,7 +402,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t\t\twork_tree, strerror(errno));\n \t\tset_git_work_tree(work_tree);\n \t}\n-\tjunk_git_dir = git_dir;\n+\tstrcpy(junk_git_dir, git_dir);\n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n \n@@ -590,6 +590,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\n \tstrbuf_release(&value);\n+\tfree(path);\n+\tfree(repo);\n+\tfree(dir);\n+\tfree(git_dir);\n \tjunk_pid = 0;\n \treturn err;\n }\n"},{"id":"110145","messageId":"alpine.DEB.1.00.0904011754250.13502@intel-tinevez-2-302","threadId":"18682","inReplyTo":"20090401144056.GC2237@lilem.mirepesht","subject":"Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-01T15:56:34Z","receivedAt":"2009-04-01T15:56:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 1 Apr 2009, Ali Gholami Rudi wrote:\n\n> With this patch, cmd_clone() safely frees its xstrdup()-allocated\n> memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()\n> which is called asynchronously) were changed to use static arrays rather\n> than sharing the memory allocated in cmd_clone().\n\nIf you want to go down that route, you will have a long way to go: the \nassumption is pretty much in every cmd_() and main() function that \nsingletons will be free()d automatically when the process ends.\n\nCiao,\nDscho\n"},{"id":"110147","messageId":"20090401161254.GD2237@lilem.mirepesht","threadId":"18682","inReplyTo":"alpine.DEB.1.00.0904011754250.13502@intel-tinevez-2-302","subject":"Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()","fromName":"Ali Gholami Rudi","fromEmail":"ali@rudi.ir","sentAt":"2009-04-01T16:12:54Z","receivedAt":"2009-04-01T16:12:54Z","isPatch":true,"sender":{"key":"ali@rudi.ir","avatar":null},"body":"Hi,\n\nJohannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> On Wed, 1 Apr 2009, Ali Gholami Rudi wrote:\n> \n> > With this patch, cmd_clone() safely frees its xstrdup()-allocated\n> > memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()\n> > which is called asynchronously) were changed to use static arrays rather\n> > than sharing the memory allocated in cmd_clone().\n> \n> If you want to go down that route, you will have a long way to go: the \n> assumption is pretty much in every cmd_() and main() function that \n> singletons will be free()d automatically when the process ends.\n\nWell... I saw strbuf_release() calls in the end of cmd_clone() and had a\nquick look at a few other cmd_*() functions; it seems most of them (?)\ntry to free their memory.  I thought it might make sense to do that for\ncmd_clone().  But you're right; they will be freed eventually.  (It\nseems like a minor leak which is respected only some of the times :-) )\n\nRegards,\nAli\n"},{"id":"110151","messageId":"7v3acs1gyf.fsf@gitster.siamese.dyndns.org","threadId":"18682","inReplyTo":"20090401161254.GD2237@lilem.mirepesht","subject":"Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-01T16:23:04Z","receivedAt":"2009-04-01T16:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ali Gholami Rudi <ali@rudi.ir> writes:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n>> On Wed, 1 Apr 2009, Ali Gholami Rudi wrote:\n>> \n>> > With this patch, cmd_clone() safely frees its xstrdup()-allocated\n>> > memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()\n>> > which is called asynchronously) were changed to use static arrays rather\n>> > than sharing the memory allocated in cmd_clone().\n>> \n>> If you want to go down that route, you will have a long way to go: the \n>> assumption is pretty much in every cmd_() and main() function that \n>> singletons will be free()d automatically when the process ends.\n>\n> Well... I saw strbuf_release() calls in the end of cmd_clone() and had a\n> quick look at a few other cmd_*() functions; it seems most of them (?)\n> try to free their memory.  I thought it might make sense to do that for\n> cmd_clone().  But you're right; they will be freed eventually.  (It\n> seems like a minor leak which is respected only some of the times :-) )\n\nYup, I'll queue (I won't have time today to work on git it seems) the\nother two patches from you, but I was going to drop this one---unless your\nplan was to make cmd_clone() callable more than once in order to use it in\nsay a C rewrite of git submodule or something like that.\n"},{"id":"110162","messageId":"20090401201704.GA2237@lilem.mirepesht","threadId":"18682","inReplyTo":"7v3acs1gyf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()","fromName":"Ali Gholami Rudi","fromEmail":"ali@rudi.ir","sentAt":"2009-04-01T20:17:04Z","receivedAt":"2009-04-01T20:17:04Z","isPatch":true,"sender":{"key":"ali@rudi.ir","avatar":null},"body":"Hi,\n\nJunio C Hamano <gitster@pobox.com> wrote:\n> Yup, I'll queue (I won't have time today to work on git it seems) the\n\nThanks.\n\n> other two patches from you, but I was going to drop this one---unless your\n> plan was to make cmd_clone() callable more than once in order to use it in\n> say a C rewrite of git submodule or something like that.\n\nThe only problem in builtin-clone.c seems to be remove_junk() which is\ncalled from a signal handler or atexit().  cmd_clone() can be changed to\nregister this function only once.\n\nremove_junk() uses junk_* global variables which are overwritten in each\ncmd_clone() call.  Since no concurrent cmd_clone() is allowed (?) and we\nonly care about the last one, this does not seem to be an issue.\n\nI'll probably send a new patch tomorrow.\n\nRegards,\nAli\n"}]}