[PATCH] builtin-clone.c: fix memory leak in cmd_clone()

[PATCH] builtin-clone.c: fix memory leak in cmd_clone()

[Ali Gholami Rudi]

With this patch, cmd_clone() safely frees its xstrdup()-allocated
memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()
which is called asynchronously) were changed to use static arrays rather
than sharing the memory allocated in cmd_clone().

Signed-off-by: Ali Gholami Rudi <ali@rudi.ir>
---
 builtin-clone.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 0031b5f..8149fc0 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -268,8 +268,8 @@ static const struct ref *clone_local(const char *src_repo,
 	return ret;
 }
 
-static const char *junk_work_tree;
-static const char *junk_git_dir;
+static char junk_work_tree[PATH_MAX];
+static char junk_git_dir[PATH_MAX];
 pid_t junk_pid;
 
 static void remove_junk(void)
@@ -277,12 +277,12 @@ static void remove_junk(void)
 	struct strbuf sb = STRBUF_INIT;
 	if (getpid() != junk_pid)
 		return;
-	if (junk_git_dir) {
+	if (*junk_git_dir) {
 		strbuf_addstr(&sb, junk_git_dir);
 		remove_dir_recursively(&sb, 0);
 		strbuf_reset(&sb);
 	}
-	if (junk_work_tree) {
+	if (*junk_work_tree) {
 		strbuf_addstr(&sb, junk_work_tree);
 		remove_dir_recursively(&sb, 0);
 		strbuf_reset(&sb);
@@ -320,8 +320,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0;
 	struct stat buf;
-	const char *repo_name, *repo, *work_tree, *git_dir;
-	char *path, *dir;
+	const char *repo_name, *work_tree;
+	char *path, *dir, *repo, *git_dir;
 	int dest_exists;
 	const struct ref *refs, *head_points_at, *remote_head, *mapped_refs;
 	struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
@@ -362,7 +362,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	else if (!strchr(repo_name, ':'))
 		repo = xstrdup(make_absolute_path(repo_name));
 	else
-		repo = repo_name;
+		repo = xstrdup(repo_name);
 
 	if (argc == 2)
 		dir = xstrdup(argv[1]);
@@ -393,7 +393,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!option_bare) {
-		junk_work_tree = work_tree;
+		strcpy(junk_work_tree, work_tree);
 		if (safe_create_leading_directories_const(work_tree) < 0)
 			die("could not create leading directories of '%s': %s",
 					work_tree, strerror(errno));
@@ -402,7 +402,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 					work_tree, strerror(errno));
 		set_git_work_tree(work_tree);
 	}
-	junk_git_dir = git_dir;
+	strcpy(junk_git_dir, git_dir);
 	atexit(remove_junk);
 	sigchain_push_common(remove_junk_on_signal);
 
@@ -590,6 +590,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
 	strbuf_release(&value);
+	free(path);
+	free(repo);
+	free(dir);
+	free(git_dir);
 	junk_pid = 0;
 	return err;
 }

Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()

[Johannes Schindelin]

Hi,

On Wed, 1 Apr 2009, Ali Gholami Rudi wrote:

> With this patch, cmd_clone() safely frees its xstrdup()-allocated
> memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()
> which is called asynchronously) were changed to use static arrays rather
> than sharing the memory allocated in cmd_clone().

If you want to go down that route, you will have a long way to go: the 
assumption is pretty much in every cmd_() and main() function that 
singletons will be free()d automatically when the process ends.

Ciao,
Dscho

Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()

[Ali Gholami Rudi]

Hi,

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Wed, 1 Apr 2009, Ali Gholami Rudi wrote:
> 
> > With this patch, cmd_clone() safely frees its xstrdup()-allocated
> > memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()
> > which is called asynchronously) were changed to use static arrays rather
> > than sharing the memory allocated in cmd_clone().
> 
> If you want to go down that route, you will have a long way to go: the 
> assumption is pretty much in every cmd_() and main() function that 
> singletons will be free()d automatically when the process ends.

Well... I saw strbuf_release() calls in the end of cmd_clone() and had a
quick look at a few other cmd_*() functions; it seems most of them (?)
try to free their memory.  I thought it might make sense to do that for
cmd_clone().  But you're right; they will be freed eventually.  (It
seems like a minor leak which is respected only some of the times :-) )

Regards,
Ali

Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()

[Junio C Hamano]

Ali Gholami Rudi <ali@rudi.ir> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> On Wed, 1 Apr 2009, Ali Gholami Rudi wrote:
>> 
>> > With this patch, cmd_clone() safely frees its xstrdup()-allocated
>> > memory.  Also junk_work_tree and junk_git_dir (used in remove_junk()
>> > which is called asynchronously) were changed to use static arrays rather
>> > than sharing the memory allocated in cmd_clone().
>> 
>> If you want to go down that route, you will have a long way to go: the 
>> assumption is pretty much in every cmd_() and main() function that 
>> singletons will be free()d automatically when the process ends.
>
> Well... I saw strbuf_release() calls in the end of cmd_clone() and had a
> quick look at a few other cmd_*() functions; it seems most of them (?)
> try to free their memory.  I thought it might make sense to do that for
> cmd_clone().  But you're right; they will be freed eventually.  (It
> seems like a minor leak which is respected only some of the times :-) )

Yup, I'll queue (I won't have time today to work on git it seems) the
other two patches from you, but I was going to drop this one---unless your
plan was to make cmd_clone() callable more than once in order to use it in
say a C rewrite of git submodule or something like that.

Re: [PATCH] builtin-clone.c: fix memory leak in cmd_clone()

[Ali Gholami Rudi]

Hi,

Junio C Hamano <gitster@pobox.com> wrote:
> Yup, I'll queue (I won't have time today to work on git it seems) the

Thanks.

> other two patches from you, but I was going to drop this one---unless your
> plan was to make cmd_clone() callable more than once in order to use it in
> say a C rewrite of git submodule or something like that.

The only problem in builtin-clone.c seems to be remove_junk() which is
called from a signal handler or atexit().  cmd_clone() can be changed to
register this function only once.

remove_junk() uses junk_* global variables which are overwritten in each
cmd_clone() call.  Since no concurrent cmd_clone() is allowed (?) and we
only care about the last one, this does not seem to be an issue.

I'll probably send a new patch tomorrow.

Regards,
Ali