GIT: [PATCH] exec_cmd: system_path memory leak fix

GIT: [PATCH] exec_cmd: system_path memory leak fix

[0xAX]

Hello All,
I found memory leak at exec_cmd.c in system_path function with valgrind.
After this patch valgrind doesn't show any memory leaks, but I'm not sure
that it is the best way to solve this problem. There are a couple of places
that uses system_path, if this way will be good, i'll make another patches
to fix this leak.

Waiting for feedback.

Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>

[PATCH] exec_cmd: system_path memory leak fix

[0xAX]

Signed-off-by: 0xAX <kuleshovmail@gmail.com>
---
 exec_cmd.c | 25 ++++++++++++++++---------
 exec_cmd.h |  4 ++--
 git.c      | 12 +++++++++---
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..08f8f80 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
 	static const char *prefix;
@@ -14,9 +14,10 @@ const char *system_path(const char *path)
 	static const char *prefix = PREFIX;
 #endif
 	struct strbuf d = STRBUF_INIT;
+	char *new_path = NULL;
 
 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);
 
 #ifdef RUNTIME_PREFIX
 	assert(argv0_path);
@@ -32,10 +33,13 @@ const char *system_path(const char *path)
 				"Using static fallback '%s'.\n", prefix);
 	}
 #endif
-
 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	new_path = malloc((strlen(prefix) + strlen(path)) + 2);
+	sprintf(new_path, "%s/%s", prefix, path);
+
+	strbuf_release(&d);
+
+	return new_path;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
@@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path)
 
 
 /* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
 {
 	const char *env;
 
 	if (argv_exec_path)
-		return argv_exec_path;
+		return strdup(argv_exec_path);
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
 	if (env && *env) {
-		return env;
+		return strdup(env);
 	}
 
 	return system_path(GIT_EXEC_PATH);
@@ -96,7 +100,9 @@ void setup_path(void)
 	const char *old_path = getenv("PATH");
 	struct strbuf new_path = STRBUF_INIT;
 
-	add_path(&new_path, git_exec_path());
+	char *exec_path = git_exec_path();
+
+	add_path(&new_path, exec_path);
 	add_path(&new_path, argv0_path);
 
 	if (old_path)
@@ -107,6 +113,7 @@ void setup_path(void)
 	setenv("PATH", new_path.buf, 1);
 
 	strbuf_release(&new_path);
+	free(exec_path);
 }
 
 const char **prepare_git_cmd(const char **argv)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@
 
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
 
 #endif /* GIT_EXEC_CMD_H */
diff --git a/git.c b/git.c
index 82d7a1c..499dc2a 100644
--- a/git.c
+++ b/git.c
@@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "--html-path")) {
-			puts(system_path(GIT_HTML_PATH));
+			char *git_html_path = system_path(GIT_HTML_PATH);
+			puts(git_html_path);
+			free(git_html_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--man-path")) {
-			puts(system_path(GIT_MAN_PATH));
+			char *git_man_path = system_path(GIT_MAN_PATH);
+			puts(git_man_path);
+			free(git_man_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--info-path")) {
-			puts(system_path(GIT_INFO_PATH));
+			char *git_info_path = system_path(GIT_INFO_PATH);
+			puts(git_info_path);
+			free(git_info_path);
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
 			use_pager = 1;
-- 
2.2.0.rc3.191.gfdea3f0

Re: [PATCH] exec_cmd: system_path memory leak fix

[0xAX]

For example some testing with valgrind:

1. pu (952f0aaadff4afca1497450911587a973c8cca02)

valgrind git help
==27034== Memcheck, a memory error detector
==27034== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==27034== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==27034== Command: git help
==27034==
==27034== HERE GIT HELP OUTPUT
==27034==
==27034== HEAP SUMMARY:
==27034==     in use at exit: 65 bytes in 1 blocks
==27034==   total heap usage: 71 allocs, 70 frees, 11,928 bytes allocated
==27034==
==27034== LEAK SUMMARY:
==27034==    definitely lost: 65 bytes in 1 blocks
==27034==    indirectly lost: 0 bytes in 0 blocks
==27034==      possibly lost: 0 bytes in 0 blocks
==27034==    still reachable: 0 bytes in 0 blocks
==27034==         suppressed: 0 bytes in 0 blocks
==27034== Rerun with --leak-check=full to see details of leaked memory
==27034==
==27034== For counts of detected and suppressed errors, rerun with: -v
==27034== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

2. With patch:

valgrind git help
==28945== Memcheck, a memory error detector
==28945== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==28945== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==28945== Command: git help
==28945==
==28945== HERE git help OUTPUT
==28945==
==28945== HEAP SUMMARY:
==28945==     in use at exit: 0 bytes in 0 blocks
==28945==   total heap usage: 72 allocs, 72 frees, 11,956 bytes allocated
==28945==
==28945== All heap blocks were freed -- no leaks are possible
==28945==
==28945== For counts of detected and suppressed errors, rerun with: -v
==28945== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Of course it is very small leak, but i just started to deep in git
source code. Hope i will be useful for community.

Thank you.

0xAX <kuleshovmail@gmail.com> @ 2014-11-23 19:56 ALMT:

> Signed-off-by: 0xAX <kuleshovmail@gmail.com>
> ---
>  exec_cmd.c | 25 ++++++++++++++++---------
>  exec_cmd.h |  4 ++--
>  git.c      | 12 +++++++++---
>  3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 698e752..08f8f80 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -6,7 +6,7 @@
>  static const char *argv_exec_path;
>  static const char *argv0_path;
>
> -const char *system_path(const char *path)
> +char *system_path(const char *path)
>  {
>  #ifdef RUNTIME_PREFIX
>  	static const char *prefix;
> @@ -14,9 +14,10 @@ const char *system_path(const char *path)
>  	static const char *prefix = PREFIX;
>  #endif
>  	struct strbuf d = STRBUF_INIT;
> +	char *new_path = NULL;
>
>  	if (is_absolute_path(path))
> -		return path;
> +		return strdup(path);
>
>  #ifdef RUNTIME_PREFIX
>  	assert(argv0_path);
> @@ -32,10 +33,13 @@ const char *system_path(const char *path)
>  				"Using static fallback '%s'.\n", prefix);
>  	}
>  #endif
> -
>  	strbuf_addf(&d, "%s/%s", prefix, path);
> -	path = strbuf_detach(&d, NULL);
> -	return path;
> +	new_path = malloc((strlen(prefix) + strlen(path)) + 2);
> +	sprintf(new_path, "%s/%s", prefix, path);
> +
> +	strbuf_release(&d);
> +
> +	return new_path;
>  }
>
>  const char *git_extract_argv0_path(const char *argv0)
> @@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path)
>
>
>  /* Returns the highest-priority, location to look for git programs. */
> -const char *git_exec_path(void)
> +char *git_exec_path(void)
>  {
>  	const char *env;
>
>  	if (argv_exec_path)
> -		return argv_exec_path;
> +		return strdup(argv_exec_path);
>
>  	env = getenv(EXEC_PATH_ENVIRONMENT);
>  	if (env && *env) {
> -		return env;
> +		return strdup(env);
>  	}
>
>  	return system_path(GIT_EXEC_PATH);
> @@ -96,7 +100,9 @@ void setup_path(void)
>  	const char *old_path = getenv("PATH");
>  	struct strbuf new_path = STRBUF_INIT;
>
> -	add_path(&new_path, git_exec_path());
> +	char *exec_path = git_exec_path();
> +
> +	add_path(&new_path, exec_path);
>  	add_path(&new_path, argv0_path);
>
>  	if (old_path)
> @@ -107,6 +113,7 @@ void setup_path(void)
>  	setenv("PATH", new_path.buf, 1);
>
>  	strbuf_release(&new_path);
> +	free(exec_path);
>  }
>
>  const char **prepare_git_cmd(const char **argv)
> diff --git a/exec_cmd.h b/exec_cmd.h
> index e4c9702..03c8599 100644
> --- a/exec_cmd.h
> +++ b/exec_cmd.h
> @@ -3,12 +3,12 @@
>
>  extern void git_set_argv_exec_path(const char *exec_path);
>  extern const char *git_extract_argv0_path(const char *path);
> -extern const char *git_exec_path(void);
> +extern char *git_exec_path(void);
>  extern void setup_path(void);
>  extern const char **prepare_git_cmd(const char **argv);
>  extern int execv_git_cmd(const char **argv); /* NULL terminated */
>  LAST_ARG_MUST_BE_NULL
>  extern int execl_git_cmd(const char *cmd, ...);
> -extern const char *system_path(const char *path);
> +extern char *system_path(const char *path);
>
>  #endif /* GIT_EXEC_CMD_H */
> diff --git a/git.c b/git.c
> index 82d7a1c..499dc2a 100644
> --- a/git.c
> +++ b/git.c
> @@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  				exit(0);
>  			}
>  		} else if (!strcmp(cmd, "--html-path")) {
> -			puts(system_path(GIT_HTML_PATH));
> +			char *git_html_path = system_path(GIT_HTML_PATH);
> +			puts(git_html_path);
> +			free(git_html_path);
>  			exit(0);
>  		} else if (!strcmp(cmd, "--man-path")) {
> -			puts(system_path(GIT_MAN_PATH));
> +			char *git_man_path = system_path(GIT_MAN_PATH);
> +			puts(git_man_path);
> +			free(git_man_path);
>  			exit(0);
>  		} else if (!strcmp(cmd, "--info-path")) {
> -			puts(system_path(GIT_INFO_PATH));
> +			char *git_info_path = system_path(GIT_INFO_PATH);
> +			puts(git_info_path);
> +			free(git_info_path);
>  			exit(0);
>  		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
>  			use_pager = 1;

--
Best regards.
0xAX

Re: GIT: [PATCH] exec_cmd: system_path memory leak fix

[Junio C Hamano]

0xAX <kuleshovmail@gmail.com> writes:

> Hello All,
> I found memory leak at exec_cmd.c in system_path function with valgrind.
> After this patch valgrind doesn't show any memory leaks, but I'm not sure
> that it is the best way to solve this problem. There are a couple of places
> that uses system_path, if this way will be good, i'll make another patches
> to fix this leak.
>
> Waiting for feedback.

It is a bit sad that the callers of the function are prepared for
the returned value to be volatile (that is why the ones that want to
do something else between the time they call it and the time they
use the value returned do make copies), while other callers that
immediately use the returned value do not have to be worried about
freeing it, but with the change you have to force everybody to
remember freeing the returned value.

If you instead limited to your change only to these two points:

 - make "struct strbuf d" static;
 - return d.buf instead of the result of strbuf_detach(&d)

the updated system_path() will keep the promise all the caller
expects from it, i.e. the value out of the function is volatile but
the callers do not have to free it, in all cases, no?

> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>

Please have that S-o-b: line (and the same name in GIT_AUTHOR_NAME)
on the patches that we'd use "git am" on, not on the cover letter
;-).  Nom de guerre nobody else recognizes outside your close
friends may be fun, but I feel that it is not quite appropriate to
appear in "git shortlog -s" output on a public project like ours.

Thanks.

Re: [PATCH] exec_cmd: system_path memory leak fix

[Junio C Hamano]

0xAX <kuleshovmail@gmail.com> writes:

> Signed-off-by: 0xAX <kuleshovmail@gmail.com>
> ---

The comment on names I've already mentioned elsewhere.

You need a better explanation than a "no log message", as you are
not doing "system-path memory leak fix".

You are doing a lot more.  Perhaps the story would start like this:

    system_path(): make the callers own the returned string

    The function sometimes returns a newly allocated string and
    sometimes returns a borrowed string, the latter of which the
    callers must not free().

    The existing callers all assume that the return value belongs to
    the callee and most of them copy it with strdup() when they want
    to keep it around.  They end up leaking the returned copy when
    the callee returned a new string.
    
    Change the contract between the callers and system_path() to
    make the returned string owned by the callers; they are
    responsible for freeing it when done, but they do not have to
    make their own copy to store it away.

    This accidentally fixes some unsafe callers as well.  For
    example, ...


>  exec_cmd.c | 25 ++++++++++++++++---------
>  exec_cmd.h |  4 ++--
>  git.c      | 12 +++++++++---

Even though I said that this changes the contract between the caller
and the callee and make things wasteful for some, I personally think
it is going in the right direction.

The change accidentally fixes some unsafe callers.  For example, the
first hit from "git grep system_path" is this:

    attr.c- static const char *system_wide;
    attr.c- if (!system_wide)
    attr.c:         system_wide = system_path(ETC_GITATTRIBUTES);
    attr.c- return system_wide;

This is obviously unsafe for a volatile return value from the callee
and needs to have strdup() on it, but with the patch there no longer
is need for such a caller-side strdup().

But this change also introduces new bugs, I think.  For example, the
second hit from "git grep system_path" is this:

  builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));

Now the caller owns and is responsible for freeing the returned
value, but without opening the file in question in an editor or a
pager we can tell immediately that there is no way this line is not
adding a new memory leak.

> index 698e752..08f8f80 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -6,7 +6,7 @@
>  static const char *argv_exec_path;
>  static const char *argv0_path;
>  
> -const char *system_path(const char *path)
> +char *system_path(const char *path)
>  {
>  #ifdef RUNTIME_PREFIX
>  	static const char *prefix;
> @@ -14,9 +14,10 @@ const char *system_path(const char *path)
>  	static const char *prefix = PREFIX;
>  #endif
>  	struct strbuf d = STRBUF_INIT;
> +	char *new_path = NULL;
>  
>  	if (is_absolute_path(path))
> -		return path;
> +		return strdup(path);
>  
>  #ifdef RUNTIME_PREFIX
>  	assert(argv0_path);
> @@ -32,10 +33,13 @@ const char *system_path(const char *path)
>  				"Using static fallback '%s'.\n", prefix);
>  	}
>  #endif
> -
>  	strbuf_addf(&d, "%s/%s", prefix, path);
> -	path = strbuf_detach(&d, NULL);
> -	return path;
> +	new_path = malloc((strlen(prefix) + strlen(path)) + 2);
> +	sprintf(new_path, "%s/%s", prefix, path);
> +
> +	strbuf_release(&d);
> +
> +	return new_path;

Are you duplicating what strbuf_addf() is doing on the strbuf d,
manually creating the same in new_path, while discarding what the
existing code you did not remove with this patch already computed?

Isn't it sufficient to add strdup(path) for the absolute case and do
nothing else to this function?  I have no idea what you are doing
here.

Re: [PATCH] exec_cmd: system_path memory leak fix

[Alex Kuleshov]

Junio C Hamano <gitster@pobox.com> @ 2014-11-24 00:51 ALMT:

> 0xAX <kuleshovmail@gmail.com> writes:
>
>> Signed-off-by: 0xAX <kuleshovmail@gmail.com>
>> ---
>
> The comment on names I've already mentioned elsewhere.

Yes, i understand about names.

>
> You need a better explanation than a "no log message", as you are
> not doing "system-path memory leak fix".
>
> You are doing a lot more.  Perhaps the story would start like this:
>
>     system_path(): make the callers own the returned string

Did it.

>
>     The function sometimes returns a newly allocated string and
>     sometimes returns a borrowed string, the latter of which the
>     callers must not free().
>
>     The existing callers all assume that the return value belongs to
>     the callee and most of them copy it with strdup() when they want
>     to keep it around.  They end up leaking the returned copy when
>     the callee returned a new string.
>
>     Change the contract between the callers and system_path() to
>     make the returned string owned by the callers; they are
>     responsible for freeing it when done, but they do not have to
>     make their own copy to store it away.

Yes you're right, i just started to read git source code some days ago,
and it's hard to understand in some places for the start. Now i see it,
thanks for explanation.

>
>     This accidentally fixes some unsafe callers as well.  For
>     example, ...
>
>
>>  exec_cmd.c | 25 ++++++++++++++++---------
>>  exec_cmd.h |  4 ++--
>>  git.c      | 12 +++++++++---
>
> Even though I said that this changes the contract between the caller
> and the callee and make things wasteful for some, I personally think
> it is going in the right direction.
>
> The change accidentally fixes some unsafe callers.  For example, the
> first hit from "git grep system_path" is this:
>
>     attr.c- static const char *system_wide;
>     attr.c- if (!system_wide)
>     attr.c:         system_wide = system_path(ETC_GITATTRIBUTES);
>     attr.c- return system_wide;
>
> This is obviously unsafe for a volatile return value from the callee
> and needs to have strdup() on it, but with the patch there no longer
> is need for such a caller-side strdup().
>
> But this change also introduces new bugs, I think.  For example, the
> second hit from "git grep system_path" is this:
>
>   builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
>
> Now the caller owns and is responsible for freeing the returned
> value, but without opening the file in question in an editor or a
> pager we can tell immediately that there is no way this line is not
> adding a new memory leak.
>
>> index 698e752..08f8f80 100644
>> --- a/exec_cmd.c
>> +++ b/exec_cmd.c
>> @@ -6,7 +6,7 @@
>>  static const char *argv_exec_path;
>>  static const char *argv0_path;
>>
>> -const char *system_path(const char *path)
>> +char *system_path(const char *path)
>>  {
>>  #ifdef RUNTIME_PREFIX
>>  	static const char *prefix;
>> @@ -14,9 +14,10 @@ const char *system_path(const char *path)
>>  	static const char *prefix = PREFIX;
>>  #endif
>>  	struct strbuf d = STRBUF_INIT;
>> +	char *new_path = NULL;
>>
>>  	if (is_absolute_path(path))
>> -		return path;
>> +		return strdup(path);
>>
>>  #ifdef RUNTIME_PREFIX
>>  	assert(argv0_path);
>> @@ -32,10 +33,13 @@ const char *system_path(const char *path)
>>  				"Using static fallback '%s'.\n", prefix);
>>  	}
>>  #endif
>> -
>>  	strbuf_addf(&d, "%s/%s", prefix, path);
>> -	path = strbuf_detach(&d, NULL);
>> -	return path;
>> +	new_path = malloc((strlen(prefix) + strlen(path)) + 2);
>> +	sprintf(new_path, "%s/%s", prefix, path);
>> +
>> +	strbuf_release(&d);
>> +
>> +	return new_path;
>
> Are you duplicating what strbuf_addf() is doing on the strbuf d,
> manually creating the same in new_path, while discarding what the
> existing code you did not remove with this patch already computed?
>
> Isn't it sufficient to add strdup(path) for the absolute case and do
> nothing else to this function?  I have no idea what you are doing
> here.

I have added changes from your previous feedback, how can I attach
second (changed) patch to this mail thread with git send-email?

--
Best regards.
0xAX

Re: [PATCH] exec_cmd: system_path memory leak fix

[Alex Kuleshov]

Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>
---
 exec_cmd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..7ed9bcc 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -13,7 +13,7 @@ const char *system_path(const char *path)
 #else
 	static const char *prefix = PREFIX;
 #endif
-	struct strbuf d = STRBUF_INIT;
+	static struct strbuf d = STRBUF_INIT;

 	if (is_absolute_path(path))
 		return path;
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif

 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	return d.buf;
 }

 const char *git_extract_argv0_path(const char *argv0)
@@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)
 void setup_path(void)
 {
 	const char *old_path = getenv("PATH");
-	struct strbuf new_path = STRBUF_INIT;
+	static struct strbuf new_path = STRBUF_INIT;

 	add_path(&new_path, git_exec_path());
 	add_path(&new_path, argv0_path);
--
2.2.0.rc3.190.g952f0aa.dirty

Re: [PATCH] exec_cmd: system_path memory leak fix

[Jeff King]

On Mon, Nov 24, 2014 at 01:19:29AM +0600, Alex Kuleshov wrote:

> 
> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>
> ---
>  exec_cmd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 698e752..7ed9bcc 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -13,7 +13,7 @@ const char *system_path(const char *path)
>  #else
>  	static const char *prefix = PREFIX;
>  #endif
> -	struct strbuf d = STRBUF_INIT;
> +	static struct strbuf d = STRBUF_INIT;
> 
>  	if (is_absolute_path(path))
>  		return path;
> @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>  #endif
> 
>  	strbuf_addf(&d, "%s/%s", prefix, path);
> -	path = strbuf_detach(&d, NULL);
> -	return path;
> +	return d.buf;
>  }

If I am reading this right, calls to system_path() will always reuse the
same buffer, even if they are called with another "path" argument. So
all callers must make sure to make a copy if they are going to hold on
to it for a long time. Grepping for callers shows us saving the result
to a static variable in at least git_etc_gitattributes, copy_templates,
and get_html_page_path. Don't these all need to learn to xstrdup the
return value?

-Peff

Re: [PATCH] exec_cmd: system_path memory leak fix

[Eric Sunshine]

On Sun, Nov 23, 2014 at 2:19 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote:
>
> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>
> ---
>  exec_cmd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 698e752..7ed9bcc 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -13,7 +13,7 @@ const char *system_path(const char *path)
>  #else
>         static const char *prefix = PREFIX;
>  #endif
> -       struct strbuf d = STRBUF_INIT;
> +       static struct strbuf d = STRBUF_INIT;

Curious. Did the unit tests pass with this change?

In the original code, the strbuf starts out empty each time
system_path() is called, and strbuf_addf() populates it. After your
change, the strbuf starts empty only on the very first call, and
subsequent calls append more content rather than replacing it. At the
very least, to fix, you now need to invoke strbuf_reset() before
invoking strbuf_addf().

>         if (is_absolute_path(path))
>                 return path;
> @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>  #endif
>
>         strbuf_addf(&d, "%s/%s", prefix, path);
> -       path = strbuf_detach(&d, NULL);
> -       return path;
> +       return d.buf;
>  }
>
>  const char *git_extract_argv0_path(const char *argv0)
> @@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)
>  void setup_path(void)
>  {
>         const char *old_path = getenv("PATH");
> -       struct strbuf new_path = STRBUF_INIT;
> +       static struct strbuf new_path = STRBUF_INIT;
>
>         add_path(&new_path, git_exec_path());
>         add_path(&new_path, argv0_path);

Not sure what this change is about. The last couple lines of this function are:

    setenv("PATH", new_path.buf, 1);
    strbuf_release(&new_path);

which means that the buffer held by the strbuf is being released
anyhow, whether static or not.

Re: [PATCH] exec_cmd: system_path memory leak fix

[Junio C Hamano]

Alex Kuleshov <kuleshovmail@gmail.com> writes:

> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>

So this one does not change the contract between the caller and the
callee.  It does not change the fact that you need to explain your
change, though.  The story should read something like this:

    system_path(): always return a volatile result

    The function sometimes returns a newly allocated string and
    sometimes returns a borrowed string, the latter of which the
    callers must not free().

    The existing callers all assume that the return value belongs to
    the callee and most of them copy it with strdup() when they want
    to keep it around.  They end up leaking the returned copy when
    the callee returned a new string.

    Make sure all callers receive a volatile result, so that they do
    not have to be worried about freeing the returned string.

    There however are existing callers that are already broken, for
    example, ...

    Fixing these callers are done as separate patches, that can be
    applied either before or after this patch.

Personally, as I already said, I think the approach the previous
version took (but not the implementation) to change the contract
between the callers and this function is probably a good idea in
the longer term.  Leaking a bit by forgetting to convert a few
callers to free the returned values will not affect the correctness,
but making the returned value consistently more volatile will
expose existing breakage more often and will break codepaths that
happened to be working by accident.

Thanks.

Re: [PATCH] exec_cmd: system_path memory leak fix

[Alex Kuleshov]

Jeff King:


>If I am reading this right, calls to system_path() will always reuse the
>same buffer, even if they are called with another "path" argument. So
>all callers must make sure to make a copy if they are going to hold on
>to it for a long time. Grepping for callers shows us saving the result
>to a static variable in at least git_etc_gitattributes, copy_templates,
>and get_html_page_path. Don't these all need to learn to xstrdup the
>return value?

Hello Jeff, yes as i wrote in previous message i saw that there some
places uses system_path function and I just wanted to find correct way
to solve problem with system_path, in near time i'll check and adapt if
need all of this places for updated system_path.

Eric Sunshine:

>Curious. Did the unit tests pass with this change?

Yes i launched unit tests and there are the same result as in origin/pu.

>Not sure what this change is about. The last couple lines of this function are:
>
>    setenv("PATH", new_path.buf, 1);
>    strbuf_release(&new_path);
>
>which means that the buffer held by the strbuf is being released
>anyhow, whether static or not.

Ah, yes, just a type, will update it.

Junio C Hamano:

>Fixing these callers are done as separate patches, that can be
>applied either before or after this patch.

How to do it better? Update this patch, fix all callers which broken and
concat this patches to one or make separate patches?

Thanks all for feedback.

--
Best regards.
0xAX

Re: [PATCH] exec_cmd: system_path memory leak fix

[Junio C Hamano]

[jc: added those who were mentioned but were missing back to Cc]

On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote:
>
> Junio C Hamano:
>
>>Fixing these callers are done as separate patches, that can be
>>applied either before or after this patch.
>
> How to do it better? Update this patch, fix all callers which broken and
> concat this patches to one or make separate patches?

As I said, I do not think the approach your v2 takes is better than the original
approach to pass the ownership of the returned value to the caller. I'd say that
a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath
where it returns an absolute pathname given as-is, with necessary changes to
callers that do not currently free the received result to free it when they are
done, and to callers that currently do strdup() of the received result not to do
strdup(), in a single patch, would be the right thing to do.

I think I already wrote the bulk of proposed commit message for you for such
a change earlier ;-) The one that talks about changing the contract between the
system_path() and its callers.

Thanks.

Re: [PATCH] exec_cmd: system_path memory leak fix

[Alex Kuleshov]

Junio C Hamano <gitster@pobox.com> @ 2014-11-24 13:37 ALMT:

> [jc: added those who were mentioned but were missing back to Cc]
>
> On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote:
>>
>> Junio C Hamano:
>>
>>>Fixing these callers are done as separate patches, that can be
>>>applied either before or after this patch.
>>
>> How to do it better? Update this patch, fix all callers which broken and
>> concat this patches to one or make separate patches?
>
> As I said, I do not think the approach your v2 takes is better than the original
> approach to pass the ownership of the returned value to the caller. I'd say that
> a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath
> where it returns an absolute pathname given as-is, with necessary changes to
> callers that do not currently free the received result to free it when they are
> done, and to callers that currently do strdup() of the received result not to do
> strdup(), in a single patch, would be the right thing to do.
>
> I think I already wrote the bulk of proposed commit message for you for such
> a change earlier ;-) The one that talks about changing the contract between the
> system_path() and its callers.
>
> Thanks.

OK, but i'm little confused now, long thread :)

So what's our next strategy?

I'll make system_path will return volatile value, so callers must free
it by itself, but we have two types of callers:

Callers which doesn't need to store system_path return value (like git
--man-path and etc.. in git.c), and which need to store it (as in attr.c).

We must to free system_path return value in first case, and does not
need to create copy of returned string with xstrdup.

Am i correct or i'm wrong with this?

Thank you.

--
Best regards.
0xAX

Re: [PATCH] exec_cmd: system_path memory leak fix

[Alexander Kuleshov]

Now system_path returns path which is allocated string to callers;
It prevents memory leaks in some places. All callers of system_path
are owners of path string and they must release it.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 attr.c            |  8 +++---
 builtin/config.c  | 73 +++++++++++++++++++++++++++++++++++++++++--------------
 builtin/help.c    | 30 ++++++++++++++++-------
 builtin/init-db.c | 12 +++++++--
 cache.h           |  4 +--
 config.c          | 10 +++++---
 exec_cmd.c        | 22 ++++++++---------
 exec_cmd.h        |  4 +--
 git.c             | 16 +++++++++---
 wrapper.c         |  4 ++-
 10 files changed, 127 insertions(+), 56 deletions(-)

diff --git a/attr.c b/attr.c
index cd54697..f96ddb4 100644
--- a/attr.c
+++ b/attr.c
@@ -462,9 +462,9 @@ static void drop_attr_stack(void)
 	}
 }

-static const char *git_etc_gitattributes(void)
+static char *git_etc_gitattributes(void)
 {
-	static const char *system_wide;
+	static char *system_wide;
 	if (!system_wide)
 		system_wide = system_path(ETC_GITATTRIBUTES);
 	return system_wide;
@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
 	attr_stack = elem;

 	if (git_attr_system()) {
-		elem = read_attr_from_file(git_etc_gitattributes(), 1);
+		char *etc_attributes = git_etc_gitattributes();
+		elem = read_attr_from_file(etc_attributes, 1);
+		free(etc_attributes);
 		if (elem) {
 			elem->origin = NULL;
 			elem->prev = attr_stack;
diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..8ab3179 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (given_config_source.blob)
 			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
-		config_file = xstrdup(given_config_source.file ?
-				      given_config_source.file : git_path("config"));
+
+		if (use_system_config)
+			config_file = given_config_source.file ?
+						  given_config_source.file : git_path("config");
+		else
+			config_file = xstrdup(given_config_source.file ?
+								  given_config_source.file : git_path("config"));
+
 		if (use_global_config) {
 			int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);
 			if (fd) {
@@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret == CONFIG_NOTHING_SET)
 			error("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s.", argv[0]);
+
+		if (use_system_config)
+			free(given_config_source.file);
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value, argv[2], 0);
+		ret = git_config_set_multivar_in_file(given_config_source.file,
+											  argv[0], value, argv[2], 0);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_ADD) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value,
-						       CONFIG_REGEX_NONE, 0);
+		ret = git_config_set_multivar_in_file(given_config_source.file, argv[0],
+											  value, CONFIG_REGEX_NONE, 0);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value, argv[2], 1);
+		ret = git_config_set_multivar_in_file(given_config_source.file, argv[0],
+											  value, argv[2], 1);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_urlmatch(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_UNSET) {
+		int ret;
 		check_write();
 		check_argc(argc, 1, 2);
-		if (argc == 2)
-			return git_config_set_multivar_in_file(given_config_source.file,
-							       argv[0], NULL, argv[1], 0);
-		else
-			return git_config_set_in_file(given_config_source.file,
-						      argv[0], NULL);
+		if (argc == 2){
+			ret = git_config_set_multivar_in_file(given_config_source.file,
+												  argv[0], NULL, argv[1], 0);
+			if (use_system_config)
+				free(given_config_source.file);
+			return ret;
+		}
+		else{
+			ret = git_config_set_in_file(given_config_source.file,
+										 argv[0], NULL);
+			if (use_system_config)
+				free(given_config_source.file);
+			return ret;
+		}
 	}
 	else if (actions == ACTION_UNSET_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], NULL, argv[1], 1);
+		ret = git_config_set_multivar_in_file(given_config_source.file,
+											  argv[0], NULL, argv[1], 1);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_source.file,
-							argv[0], argv[1]);
+												argv[0], argv[1]);
+		if (use_system_config)
+			free(given_config_source.file);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
@@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
+		if (use_system_config)
+			free(given_config_source.file);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
diff --git a/builtin/help.c b/builtin/help.c
index b3c818e..20ffbb1 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -326,7 +326,8 @@ static void setup_man_path(void)
 	 * old_path, the ':' at the end will let 'man' to try
 	 * system-wide paths after ours to find the manual page. If
 	 * there is old_path, we need ':' as delimiter. */
-	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+	char *git_man_path = system_path(GIT_MAN_PATH);
+	strbuf_addstr(&new_path, git_man_path);
 	strbuf_addch(&new_path, ':');
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
@@ -334,6 +335,7 @@ static void setup_man_path(void)
 	setenv("MANPATH", new_path.buf, 1);

 	strbuf_release(&new_path);
+	free(git_man_path);
 }

 static void exec_viewer(const char *name, const char *page)
@@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd)
 static void show_info_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
-	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
+	char *git_info_path = system_path(GIT_INFO_PATH);
+	setenv("INFOPATH", git_info_path, 1);
+	free(git_info_path);
 	execlp("info", "info", "gitman", page, (char *)NULL);
 	die(_("no info viewer handled the request"));
 }

-static void get_html_page_path(struct strbuf *page_path, const char *page)
+static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)
 {
 	struct stat st;
-	if (!html_path)
-		html_path = system_path(GIT_HTML_PATH);

 	/* Check that we have a git documentation directory. */
 	if (!strstr(html_path, "://")) {
 		if (stat(mkpath("%s/git.html", html_path), &st)
-		    || !S_ISREG(st.st_mode))
-			die("'%s': not a documentation directory.", html_path);
+				|| !S_ISREG(st.st_mode)){
+			printf("'%s': not a documentation directory.\n", html_path);
+			free(html_path);
+			exit(1);
+		}
 	}

 	strbuf_init(page_path, 0);
@@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
  * HTML.
  */
 #ifndef open_html
-static void open_html(const char *path)
+static void open_html(char *path)
 {
 	execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
+	free(path);
 }
 #endif

@@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
 	struct strbuf page_path; /* it leaks but we exec bellow */
+	char* html_path = NULL;
+
+	if (!html_path)
+		html_path = system_path(GIT_HTML_PATH);

-	get_html_page_path(&page_path, page);
+	get_html_page_path(&page_path, page, html_path);

 	open_html(page_path.buf);
 }
@@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix)

 	return 0;
 }
+
+// git help --web commit
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2619ca5..6f36f3f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)
 		template_dir = init_db_template_dir;
 	if (!template_dir)
 		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
-	if (!template_dir[0])
+	if (!template_dir[0]){
+		if (!template_dir)
+			free((char*)template_dir);
 		return;
+	}
 	template_len = strlen(template_dir);
-	if (PATH_MAX <= (template_len+strlen("/config")))
+	if (PATH_MAX <= (template_len+strlen("/config"))){
+		if (!template_dir)
+			free((char*)template_dir);
 		die(_("insanely long template path %s"), template_dir);
+	}
 	strcpy(template_path, template_dir);
 	if (template_path[template_len-1] != '/') {
 		template_path[template_len++] = '/';
@@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir)
 	dir = opendir(template_path);
 	if (!dir) {
 		warning(_("templates not found %s"), template_dir);
+		free((char*)template_dir);
 		return;
 	}

@@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir)
 			"a wrong format version %d from '%s'"),
 			repository_format_version,
 			template_dir);
+		free((char*)template_dir);
 		closedir(dir);
 		return;
 	}
diff --git a/cache.h b/cache.h
index 1b05115..7048e7a 100644
--- a/cache.h
+++ b/cache.h
@@ -1354,7 +1354,7 @@ extern int update_server_info(int);

 struct git_config_source {
 	unsigned int use_stdin:1;
-	const char *file;
+	char *file;
 	const char *blob;
 };

@@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
 extern int git_config_rename_section_in_file(const char *, const char *, const char *);
-extern const char *git_etc_gitconfig(void);
+extern char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
diff --git a/config.c b/config.c
index ae1398f..0ed8890 100644
--- a/config.c
+++ b/config.c
@@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
 	return git_config_from_blob_sha1(fn, name, sha1, data);
 }

-const char *git_etc_gitconfig(void)
+char *git_etc_gitconfig(void)
 {
-	static const char *system_wide;
+	static char *system_wide;
 	if (!system_wide)
 		system_wide = system_path(ETC_GITCONFIG);
 	return system_wide;
@@ -1172,13 +1172,15 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	int ret = 0, found = 0;
 	char *xdg_config = NULL;
 	char *user_config = NULL;
+	char *git_etc_config = git_etc_gitconfig();

 	home_config_paths(&user_config, &xdg_config, "config");

-	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
-		ret += git_config_from_file(fn, git_etc_gitconfig(),
+	if (git_etc_config && !access_or_die(git_etc_config, R_OK, 0)) {
+		ret += git_config_from_file(fn, git_etc_config,
 					    data);
 		found += 1;
+		free(git_etc_config);
 	}

 	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..d35ecac 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;

-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
 	static const char *prefix;
@@ -16,7 +16,7 @@ const char *system_path(const char *path)
 	struct strbuf d = STRBUF_INIT;

 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);

 #ifdef RUNTIME_PREFIX
 	assert(argv0_path);
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif

 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	return d.buf;
 }

 const char *git_extract_argv0_path(const char *argv0)
@@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)


 /* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
 {
-	const char *env;
+	char *env;

 	if (argv_exec_path)
-		return argv_exec_path;
+		return strdup(argv_exec_path);

 	env = getenv(EXEC_PATH_ENVIRONMENT);
-	if (env && *env) {
-		return env;
-	}
+	if (env && *env)
+		return strdup(env);

 	return system_path(GIT_EXEC_PATH);
 }
@@ -96,7 +94,8 @@ void setup_path(void)
 	const char *old_path = getenv("PATH");
 	struct strbuf new_path = STRBUF_INIT;

-	add_path(&new_path, git_exec_path());
+	char *exec_path = git_exec_path();
+	add_path(&new_path, exec_path);
 	add_path(&new_path, argv0_path);

 	if (old_path)
@@ -107,6 +106,7 @@ void setup_path(void)
 	setenv("PATH", new_path.buf, 1);

 	strbuf_release(&new_path);
+	free(exec_path);
 }

 const char **prepare_git_cmd(const char **argv)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@

 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);

 #endif /* GIT_EXEC_CMD_H */
diff --git a/git.c b/git.c
index 82d7a1c..d01c4f1 100644
--- a/git.c
+++ b/git.c
@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (*cmd == '=')
 				git_set_argv_exec_path(cmd + 1);
 			else {
-				puts(git_exec_path());
+				char *exec_path = git_exec_path();
+				puts(exec_path);
+				free(exec_path);
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "--html-path")) {
-			puts(system_path(GIT_HTML_PATH));
+			char *git_html_path = system_path(GIT_HTML_PATH);
+			puts(git_html_path);
+			free(git_html_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--man-path")) {
-			puts(system_path(GIT_MAN_PATH));
+			char *git_man_path = system_path(GIT_MAN_PATH);
+			puts(git_man_path);
+			free(git_man_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--info-path")) {
-			puts(system_path(GIT_INFO_PATH));
+			char *git_info_path = system_path(GIT_INFO_PATH);
+			puts(git_info_path);
+			free(git_info_path);
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
 			use_pager = 1;
diff --git a/wrapper.c b/wrapper.c
index 007ec0d..ede9268 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -526,8 +526,10 @@ int access_or_warn(const char *path, int mode, unsigned flag)
 int access_or_die(const char *path, int mode, unsigned flag)
 {
 	int ret = access(path, mode);
-	if (ret && !access_error_is_ok(errno, flag))
+	if (ret && !access_error_is_ok(errno, flag)){
+		free((char*)path);
 		die_errno(_("unable to access '%s'"), path);
+	}
 	return ret;
 }

--
2.2.0-rc3

Re: [PATCH] exec_cmd: system_path memory leak fix

[Alex Kuleshov]

Now system_path returns path which is allocated string to callers;
It prevents memory leaks in some places. All callers of system_path
are owners of path string and they must release it.

Added new parameter to wrapper.c/int access_or_die - etc_config, because
only etc_config in this case use system_path and we don't know do we
need to free path or not.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 attr.c            |  8 +++---
 builtin/config.c  | 73 +++++++++++++++++++++++++++++++++++++++++--------------
 builtin/help.c    | 30 ++++++++++++++++-------
 builtin/init-db.c | 12 +++++++--
 cache.h           |  4 +--
 config.c          | 19 ++++++++-------
 exec_cmd.c        | 22 ++++++++---------
 exec_cmd.h        |  4 +--
 git-compat-util.h |  2 +-
 git.c             | 16 +++++++++---
 wrapper.c         |  7 ++++--
 11 files changed, 134 insertions(+), 63 deletions(-)

diff --git a/attr.c b/attr.c
index cd54697..f96ddb4 100644
--- a/attr.c
+++ b/attr.c
@@ -462,9 +462,9 @@ static void drop_attr_stack(void)
 	}
 }

-static const char *git_etc_gitattributes(void)
+static char *git_etc_gitattributes(void)
 {
-	static const char *system_wide;
+	static char *system_wide;
 	if (!system_wide)
 		system_wide = system_path(ETC_GITATTRIBUTES);
 	return system_wide;
@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
 	attr_stack = elem;

 	if (git_attr_system()) {
-		elem = read_attr_from_file(git_etc_gitattributes(), 1);
+		char *etc_attributes = git_etc_gitattributes();
+		elem = read_attr_from_file(etc_attributes, 1);
+		free(etc_attributes);
 		if (elem) {
 			elem->origin = NULL;
 			elem->prev = attr_stack;
diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..266d42b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (given_config_source.blob)
 			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
-		config_file = xstrdup(given_config_source.file ?
-				      given_config_source.file : git_path("config"));
+
+		if (use_system_config)
+			config_file = given_config_source.file ?
+							given_config_source.file : git_path("config");
+		else
+			config_file = xstrdup(given_config_source.file ?
+								  given_config_source.file : git_path("config"));
+
 		if (use_global_config) {
 			int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);
 			if (fd) {
@@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret == CONFIG_NOTHING_SET)
 			error("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s.", argv[0]);
+
+		if (use_system_config)
+			free(given_config_source.file);
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value, argv[2], 0);
+		ret = git_config_set_multivar_in_file(given_config_source.file,
+											  argv[0], value, argv[2], 0);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_ADD) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value,
-						       CONFIG_REGEX_NONE, 0);
+		ret = git_config_set_multivar_in_file(given_config_source.file, argv[0],
+											  value, CONFIG_REGEX_NONE, 0);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value, argv[2], 1);
+		ret = git_config_set_multivar_in_file(given_config_source.file, argv[0],
+											  value, argv[2], 1);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_urlmatch(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_UNSET) {
+		int ret;
 		check_write();
 		check_argc(argc, 1, 2);
-		if (argc == 2)
-			return git_config_set_multivar_in_file(given_config_source.file,
-							       argv[0], NULL, argv[1], 0);
-		else
-			return git_config_set_in_file(given_config_source.file,
-						      argv[0], NULL);
+		if (argc == 2){
+			ret = git_config_set_multivar_in_file(given_config_source.file,
+												  argv[0], NULL, argv[1], 0);
+			if (use_system_config)
+				free(given_config_source.file);
+			return ret;
+		}
+		else{
+			ret = git_config_set_in_file(given_config_source.file,
+										 argv[0], NULL);
+			if (use_system_config)
+				free(given_config_source.file);
+			return ret;
+		}
 	}
 	else if (actions == ACTION_UNSET_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], NULL, argv[1], 1);
+		ret = git_config_set_multivar_in_file(given_config_source.file,
+											  argv[0], NULL, argv[1], 1);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_source.file,
-							argv[0], argv[1]);
+												argv[0], argv[1]);
+		if (use_system_config)
+			free(given_config_source.file);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
@@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
+		if (use_system_config)
+			free(given_config_source.file);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
diff --git a/builtin/help.c b/builtin/help.c
index b3c818e..20ffbb1 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -326,7 +326,8 @@ static void setup_man_path(void)
 	 * old_path, the ':' at the end will let 'man' to try
 	 * system-wide paths after ours to find the manual page. If
 	 * there is old_path, we need ':' as delimiter. */
-	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+	char *git_man_path = system_path(GIT_MAN_PATH);
+	strbuf_addstr(&new_path, git_man_path);
 	strbuf_addch(&new_path, ':');
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
@@ -334,6 +335,7 @@ static void setup_man_path(void)
 	setenv("MANPATH", new_path.buf, 1);

 	strbuf_release(&new_path);
+	free(git_man_path);
 }

 static void exec_viewer(const char *name, const char *page)
@@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd)
 static void show_info_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
-	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
+	char *git_info_path = system_path(GIT_INFO_PATH);
+	setenv("INFOPATH", git_info_path, 1);
+	free(git_info_path);
 	execlp("info", "info", "gitman", page, (char *)NULL);
 	die(_("no info viewer handled the request"));
 }

-static void get_html_page_path(struct strbuf *page_path, const char *page)
+static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)
 {
 	struct stat st;
-	if (!html_path)
-		html_path = system_path(GIT_HTML_PATH);

 	/* Check that we have a git documentation directory. */
 	if (!strstr(html_path, "://")) {
 		if (stat(mkpath("%s/git.html", html_path), &st)
-		    || !S_ISREG(st.st_mode))
-			die("'%s': not a documentation directory.", html_path);
+				|| !S_ISREG(st.st_mode)){
+			printf("'%s': not a documentation directory.\n", html_path);
+			free(html_path);
+			exit(1);
+		}
 	}

 	strbuf_init(page_path, 0);
@@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
  * HTML.
  */
 #ifndef open_html
-static void open_html(const char *path)
+static void open_html(char *path)
 {
 	execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
+	free(path);
 }
 #endif

@@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
 	struct strbuf page_path; /* it leaks but we exec bellow */
+	char* html_path = NULL;
+
+	if (!html_path)
+		html_path = system_path(GIT_HTML_PATH);

-	get_html_page_path(&page_path, page);
+	get_html_page_path(&page_path, page, html_path);

 	open_html(page_path.buf);
 }
@@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix)

 	return 0;
 }
+
+// git help --web commit
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2619ca5..6f36f3f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)
 		template_dir = init_db_template_dir;
 	if (!template_dir)
 		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
-	if (!template_dir[0])
+	if (!template_dir[0]){
+		if (!template_dir)
+			free((char*)template_dir);
 		return;
+	}
 	template_len = strlen(template_dir);
-	if (PATH_MAX <= (template_len+strlen("/config")))
+	if (PATH_MAX <= (template_len+strlen("/config"))){
+		if (!template_dir)
+			free((char*)template_dir);
 		die(_("insanely long template path %s"), template_dir);
+	}
 	strcpy(template_path, template_dir);
 	if (template_path[template_len-1] != '/') {
 		template_path[template_len++] = '/';
@@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir)
 	dir = opendir(template_path);
 	if (!dir) {
 		warning(_("templates not found %s"), template_dir);
+		free((char*)template_dir);
 		return;
 	}

@@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir)
 			"a wrong format version %d from '%s'"),
 			repository_format_version,
 			template_dir);
+		free((char*)template_dir);
 		closedir(dir);
 		return;
 	}
diff --git a/cache.h b/cache.h
index 1b05115..7048e7a 100644
--- a/cache.h
+++ b/cache.h
@@ -1354,7 +1354,7 @@ extern int update_server_info(int);

 struct git_config_source {
 	unsigned int use_stdin:1;
-	const char *file;
+	char *file;
 	const char *blob;
 };

@@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
 extern int git_config_rename_section_in_file(const char *, const char *, const char *);
-extern const char *git_etc_gitconfig(void);
+extern char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
diff --git a/config.c b/config.c
index ae1398f..b71c0fb 100644
--- a/config.c
+++ b/config.c
@@ -122,7 +122,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}

-	if (!access_or_die(path, R_OK, 0)) {
+	if (!access_or_die(path, R_OK, 0, 0)) {
 		if (++inc->depth > MAX_INCLUDE_DEPTH)
 			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
 			    cf && cf->name ? cf->name : "the command line");
@@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
 	return git_config_from_blob_sha1(fn, name, sha1, data);
 }

-const char *git_etc_gitconfig(void)
+char *git_etc_gitconfig(void)
 {
-	static const char *system_wide;
+	static char *system_wide;
 	if (!system_wide)
 		system_wide = system_path(ETC_GITCONFIG);
 	return system_wide;
@@ -1172,26 +1172,27 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	int ret = 0, found = 0;
 	char *xdg_config = NULL;
 	char *user_config = NULL;
+	char *git_etc_config = git_etc_gitconfig();

 	home_config_paths(&user_config, &xdg_config, "config");

-	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
-		ret += git_config_from_file(fn, git_etc_gitconfig(),
-					    data);
+	if (git_etc_config && !access_or_die(git_etc_config, R_OK, 0, 1)) {
+		ret += git_config_from_file(fn, git_etc_config, data);
 		found += 1;
+		free(git_etc_config);
 	}

-	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
+	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK, 0)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}

-	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
+	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK, 0)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}

-	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
+	if (repo_config && !access_or_die(repo_config, R_OK, 0, 0)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..d35ecac 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;

-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
 	static const char *prefix;
@@ -16,7 +16,7 @@ const char *system_path(const char *path)
 	struct strbuf d = STRBUF_INIT;

 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);

 #ifdef RUNTIME_PREFIX
 	assert(argv0_path);
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif

 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	return d.buf;
 }

 const char *git_extract_argv0_path(const char *argv0)
@@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)


 /* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
 {
-	const char *env;
+	char *env;

 	if (argv_exec_path)
-		return argv_exec_path;
+		return strdup(argv_exec_path);

 	env = getenv(EXEC_PATH_ENVIRONMENT);
-	if (env && *env) {
-		return env;
-	}
+	if (env && *env)
+		return strdup(env);

 	return system_path(GIT_EXEC_PATH);
 }
@@ -96,7 +94,8 @@ void setup_path(void)
 	const char *old_path = getenv("PATH");
 	struct strbuf new_path = STRBUF_INIT;

-	add_path(&new_path, git_exec_path());
+	char *exec_path = git_exec_path();
+	add_path(&new_path, exec_path);
 	add_path(&new_path, argv0_path);

 	if (old_path)
@@ -107,6 +106,7 @@ void setup_path(void)
 	setenv("PATH", new_path.buf, 1);

 	strbuf_release(&new_path);
+	free(exec_path);
 }

 const char **prepare_git_cmd(const char **argv)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@

 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);

 #endif /* GIT_EXEC_CMD_H */
diff --git a/git-compat-util.h b/git-compat-util.h
index 400e921..fcc88db 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -813,7 +813,7 @@ int remove_or_warn(unsigned int mode, const char *path);
  */
 #define ACCESS_EACCES_OK (1U << 0)
 int access_or_warn(const char *path, int mode, unsigned flag);
-int access_or_die(const char *path, int mode, unsigned flag);
+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config);

 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
diff --git a/git.c b/git.c
index 82d7a1c..d01c4f1 100644
--- a/git.c
+++ b/git.c
@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (*cmd == '=')
 				git_set_argv_exec_path(cmd + 1);
 			else {
-				puts(git_exec_path());
+				char *exec_path = git_exec_path();
+				puts(exec_path);
+				free(exec_path);
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "--html-path")) {
-			puts(system_path(GIT_HTML_PATH));
+			char *git_html_path = system_path(GIT_HTML_PATH);
+			puts(git_html_path);
+			free(git_html_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--man-path")) {
-			puts(system_path(GIT_MAN_PATH));
+			char *git_man_path = system_path(GIT_MAN_PATH);
+			puts(git_man_path);
+			free(git_man_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--info-path")) {
-			puts(system_path(GIT_INFO_PATH));
+			char *git_info_path = system_path(GIT_INFO_PATH);
+			puts(git_info_path);
+			free(git_info_path);
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
 			use_pager = 1;
diff --git a/wrapper.c b/wrapper.c
index 007ec0d..f97adbc 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -523,11 +523,14 @@ int access_or_warn(const char *path, int mode, unsigned flag)
 	return ret;
 }

-int access_or_die(const char *path, int mode, unsigned flag)
+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config)
 {
 	int ret = access(path, mode);
-	if (ret && !access_error_is_ok(errno, flag))
+	if (ret && !access_error_is_ok(errno, flag)){
+		if (etc_config)
+			free((char*)path);
 		die_errno(_("unable to access '%s'"), path);
+	}
 	return ret;
 }

--
2.2.0.rc3.191.g70a3888.dirty

[PATCH] change contract between system_path and it's callers

[0xAX]

Now system_path returns path which is allocated string to callers;
It prevents memory leaks in some places. All callers of system_path
are owners of path string and they must release it.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 attr.c            |  8 +++---
 builtin/config.c  | 73 +++++++++++++++++++++++++++++++++++++++++--------------
 builtin/help.c    | 30 ++++++++++++++++-------
 builtin/init-db.c | 12 +++++++--
 cache.h           |  4 +--
 config.c          | 19 ++++++++-------
 exec_cmd.c        | 22 ++++++++---------
 exec_cmd.h        |  4 +--
 git-compat-util.h |  2 +-
 git.c             | 16 +++++++++---
 wrapper.c         |  7 ++++--
 11 files changed, 134 insertions(+), 63 deletions(-)

diff --git a/attr.c b/attr.c
index cd54697..f96ddb4 100644
--- a/attr.c
+++ b/attr.c
@@ -462,9 +462,9 @@ static void drop_attr_stack(void)
 	}
 }
 
-static const char *git_etc_gitattributes(void)
+static char *git_etc_gitattributes(void)
 {
-	static const char *system_wide;
+	static char *system_wide;
 	if (!system_wide)
 		system_wide = system_path(ETC_GITATTRIBUTES);
 	return system_wide;
@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
 	attr_stack = elem;
 
 	if (git_attr_system()) {
-		elem = read_attr_from_file(git_etc_gitattributes(), 1);
+		char *etc_attributes = git_etc_gitattributes();
+		elem = read_attr_from_file(etc_attributes, 1);
+		free(etc_attributes);
 		if (elem) {
 			elem->origin = NULL;
 			elem->prev = attr_stack;
diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..266d42b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (given_config_source.blob)
 			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
-		config_file = xstrdup(given_config_source.file ?
-				      given_config_source.file : git_path("config"));
+
+		if (use_system_config)
+			config_file = given_config_source.file ?
+							given_config_source.file : git_path("config");
+		else
+			config_file = xstrdup(given_config_source.file ?
+								  given_config_source.file : git_path("config"));
+
 		if (use_global_config) {
 			int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);
 			if (fd) {
@@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (ret == CONFIG_NOTHING_SET)
 			error("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s.", argv[0]);
+
+		if (use_system_config)
+			free(given_config_source.file);
 		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value, argv[2], 0);
+		ret = git_config_set_multivar_in_file(given_config_source.file,
+											  argv[0], value, argv[2], 0);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_ADD) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value,
-						       CONFIG_REGEX_NONE, 0);
+		ret = git_config_set_multivar_in_file(given_config_source.file, argv[0],
+											  value, CONFIG_REGEX_NONE, 0);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 2, 3);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], value, argv[2], 1);
+		ret = git_config_set_multivar_in_file(given_config_source.file, argv[0],
+											  value, argv[2], 1);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		return get_urlmatch(argv[0], argv[1]);
 	}
 	else if (actions == ACTION_UNSET) {
+		int ret;
 		check_write();
 		check_argc(argc, 1, 2);
-		if (argc == 2)
-			return git_config_set_multivar_in_file(given_config_source.file,
-							       argv[0], NULL, argv[1], 0);
-		else
-			return git_config_set_in_file(given_config_source.file,
-						      argv[0], NULL);
+		if (argc == 2){
+			ret = git_config_set_multivar_in_file(given_config_source.file,
+												  argv[0], NULL, argv[1], 0);
+			if (use_system_config)
+				free(given_config_source.file);
+			return ret;
+		}
+		else{
+			ret = git_config_set_in_file(given_config_source.file,
+										 argv[0], NULL);
+			if (use_system_config)
+				free(given_config_source.file);
+			return ret;
+		}
 	}
 	else if (actions == ACTION_UNSET_ALL) {
+		int ret;
 		check_write();
 		check_argc(argc, 1, 2);
-		return git_config_set_multivar_in_file(given_config_source.file,
-						       argv[0], NULL, argv[1], 1);
+		ret = git_config_set_multivar_in_file(given_config_source.file,
+											  argv[0], NULL, argv[1], 1);
+		if (use_system_config)
+			free(given_config_source.file);
+		return ret;
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
 		check_write();
 		check_argc(argc, 2, 2);
 		ret = git_config_rename_section_in_file(given_config_source.file,
-							argv[0], argv[1]);
+												argv[0], argv[1]);
+		if (use_system_config)
+			free(given_config_source.file);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
@@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 1);
 		ret = git_config_rename_section_in_file(given_config_source.file,
 							argv[0], NULL);
+		if (use_system_config)
+			free(given_config_source.file);
 		if (ret < 0)
 			return ret;
 		if (ret == 0)
diff --git a/builtin/help.c b/builtin/help.c
index b3c818e..20ffbb1 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -326,7 +326,8 @@ static void setup_man_path(void)
 	 * old_path, the ':' at the end will let 'man' to try
 	 * system-wide paths after ours to find the manual page. If
 	 * there is old_path, we need ':' as delimiter. */
-	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+	char *git_man_path = system_path(GIT_MAN_PATH);
+	strbuf_addstr(&new_path, git_man_path);
 	strbuf_addch(&new_path, ':');
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
@@ -334,6 +335,7 @@ static void setup_man_path(void)
 	setenv("MANPATH", new_path.buf, 1);
 
 	strbuf_release(&new_path);
+	free(git_man_path);
 }
 
 static void exec_viewer(const char *name, const char *page)
@@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd)
 static void show_info_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
-	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
+	char *git_info_path = system_path(GIT_INFO_PATH);
+	setenv("INFOPATH", git_info_path, 1);
+	free(git_info_path);
 	execlp("info", "info", "gitman", page, (char *)NULL);
 	die(_("no info viewer handled the request"));
 }
 
-static void get_html_page_path(struct strbuf *page_path, const char *page)
+static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)
 {
 	struct stat st;
-	if (!html_path)
-		html_path = system_path(GIT_HTML_PATH);
 
 	/* Check that we have a git documentation directory. */
 	if (!strstr(html_path, "://")) {
 		if (stat(mkpath("%s/git.html", html_path), &st)
-		    || !S_ISREG(st.st_mode))
-			die("'%s': not a documentation directory.", html_path);
+				|| !S_ISREG(st.st_mode)){
+			printf("'%s': not a documentation directory.\n", html_path);
+			free(html_path);
+			exit(1);
+		}
 	}
 
 	strbuf_init(page_path, 0);
@@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
  * HTML.
  */
 #ifndef open_html
-static void open_html(const char *path)
+static void open_html(char *path)
 {
 	execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
+	free(path);
 }
 #endif
 
@@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
 	struct strbuf page_path; /* it leaks but we exec bellow */
+	char* html_path = NULL;
+
+	if (!html_path)
+		html_path = system_path(GIT_HTML_PATH);
 
-	get_html_page_path(&page_path, page);
+	get_html_page_path(&page_path, page, html_path);
 
 	open_html(page_path.buf);
 }
@@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	return 0;
 }
+
+// git help --web commit
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2619ca5..6f36f3f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)
 		template_dir = init_db_template_dir;
 	if (!template_dir)
 		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
-	if (!template_dir[0])
+	if (!template_dir[0]){
+		if (!template_dir)
+			free((char*)template_dir);
 		return;
+	}
 	template_len = strlen(template_dir);
-	if (PATH_MAX <= (template_len+strlen("/config")))
+	if (PATH_MAX <= (template_len+strlen("/config"))){
+		if (!template_dir)
+			free((char*)template_dir);
 		die(_("insanely long template path %s"), template_dir);
+	}
 	strcpy(template_path, template_dir);
 	if (template_path[template_len-1] != '/') {
 		template_path[template_len++] = '/';
@@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir)
 	dir = opendir(template_path);
 	if (!dir) {
 		warning(_("templates not found %s"), template_dir);
+		free((char*)template_dir);
 		return;
 	}
 
@@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir)
 			"a wrong format version %d from '%s'"),
 			repository_format_version,
 			template_dir);
+		free((char*)template_dir);
 		closedir(dir);
 		return;
 	}
diff --git a/cache.h b/cache.h
index 1b05115..7048e7a 100644
--- a/cache.h
+++ b/cache.h
@@ -1354,7 +1354,7 @@ extern int update_server_info(int);
 
 struct git_config_source {
 	unsigned int use_stdin:1;
-	const char *file;
+	char *file;
 	const char *blob;
 };
 
@@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
 extern int git_config_rename_section_in_file(const char *, const char *, const char *);
-extern const char *git_etc_gitconfig(void);
+extern char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
 extern int git_env_bool(const char *, int);
 extern unsigned long git_env_ulong(const char *, unsigned long);
diff --git a/config.c b/config.c
index ae1398f..b71c0fb 100644
--- a/config.c
+++ b/config.c
@@ -122,7 +122,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access_or_die(path, R_OK, 0)) {
+	if (!access_or_die(path, R_OK, 0, 0)) {
 		if (++inc->depth > MAX_INCLUDE_DEPTH)
 			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
 			    cf && cf->name ? cf->name : "the command line");
@@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn,
 	return git_config_from_blob_sha1(fn, name, sha1, data);
 }
 
-const char *git_etc_gitconfig(void)
+char *git_etc_gitconfig(void)
 {
-	static const char *system_wide;
+	static char *system_wide;
 	if (!system_wide)
 		system_wide = system_path(ETC_GITCONFIG);
 	return system_wide;
@@ -1172,26 +1172,27 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 	int ret = 0, found = 0;
 	char *xdg_config = NULL;
 	char *user_config = NULL;
+	char *git_etc_config = git_etc_gitconfig();
 
 	home_config_paths(&user_config, &xdg_config, "config");
 
-	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
-		ret += git_config_from_file(fn, git_etc_gitconfig(),
-					    data);
+	if (git_etc_config && !access_or_die(git_etc_config, R_OK, 0, 1)) {
+		ret += git_config_from_file(fn, git_etc_config, data);
 		found += 1;
+		free(git_etc_config);
 	}
 
-	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
+	if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK, 0)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
+	if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK, 0)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
 
-	if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
+	if (repo_config && !access_or_die(repo_config, R_OK, 0, 0)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..d35ecac 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
 	static const char *prefix;
@@ -16,7 +16,7 @@ const char *system_path(const char *path)
 	struct strbuf d = STRBUF_INIT;
 
 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);
 
 #ifdef RUNTIME_PREFIX
 	assert(argv0_path);
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif
 
 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	return d.buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
@@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
 
 
 /* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
 {
-	const char *env;
+	char *env;
 
 	if (argv_exec_path)
-		return argv_exec_path;
+		return strdup(argv_exec_path);
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
-	if (env && *env) {
-		return env;
-	}
+	if (env && *env)
+		return strdup(env);
 
 	return system_path(GIT_EXEC_PATH);
 }
@@ -96,7 +94,8 @@ void setup_path(void)
 	const char *old_path = getenv("PATH");
 	struct strbuf new_path = STRBUF_INIT;
 
-	add_path(&new_path, git_exec_path());
+	char *exec_path = git_exec_path();
+	add_path(&new_path, exec_path);
 	add_path(&new_path, argv0_path);
 
 	if (old_path)
@@ -107,6 +106,7 @@ void setup_path(void)
 	setenv("PATH", new_path.buf, 1);
 
 	strbuf_release(&new_path);
+	free(exec_path);
 }
 
 const char **prepare_git_cmd(const char **argv)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@
 
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
 
 #endif /* GIT_EXEC_CMD_H */
diff --git a/git-compat-util.h b/git-compat-util.h
index 400e921..fcc88db 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -813,7 +813,7 @@ int remove_or_warn(unsigned int mode, const char *path);
  */
 #define ACCESS_EACCES_OK (1U << 0)
 int access_or_warn(const char *path, int mode, unsigned flag);
-int access_or_die(const char *path, int mode, unsigned flag);
+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config);
 
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
diff --git a/git.c b/git.c
index 82d7a1c..d01c4f1 100644
--- a/git.c
+++ b/git.c
@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (*cmd == '=')
 				git_set_argv_exec_path(cmd + 1);
 			else {
-				puts(git_exec_path());
+				char *exec_path = git_exec_path();
+				puts(exec_path);
+				free(exec_path);
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "--html-path")) {
-			puts(system_path(GIT_HTML_PATH));
+			char *git_html_path = system_path(GIT_HTML_PATH);
+			puts(git_html_path);
+			free(git_html_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--man-path")) {
-			puts(system_path(GIT_MAN_PATH));
+			char *git_man_path = system_path(GIT_MAN_PATH);
+			puts(git_man_path);
+			free(git_man_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--info-path")) {
-			puts(system_path(GIT_INFO_PATH));
+			char *git_info_path = system_path(GIT_INFO_PATH);
+			puts(git_info_path);
+			free(git_info_path);
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
 			use_pager = 1;
diff --git a/wrapper.c b/wrapper.c
index 007ec0d..f97adbc 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -523,11 +523,14 @@ int access_or_warn(const char *path, int mode, unsigned flag)
 	return ret;
 }
 
-int access_or_die(const char *path, int mode, unsigned flag)
+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config)
 {
 	int ret = access(path, mode);
-	if (ret && !access_error_is_ok(errno, flag))
+	if (ret && !access_error_is_ok(errno, flag)){
+		if (etc_config)
+			free((char*)path);
 		die_errno(_("unable to access '%s'"), path);
+	}
 	return ret;
 }
 
-- 
2.2.0.rc3.191.g70a3888.dirty

Re*: [PATCH] change contract between system_path and it's callers

[Junio C Hamano]

0xAX <kuleshovmail@gmail.com> writes:

> Now system_path returns path which is allocated string to callers;
> It prevents memory leaks in some places. All callers of system_path
> are owners of path string and they must release it.
>
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---

> -static const char *git_etc_gitattributes(void)
> +static char *git_etc_gitattributes(void)

Hmph, I think this should keep returning "const char *", as the
caller is not expected to free the pointer or write into the memory
held by the returned string.  The same applies to the change of the
type in "struct git_config_source".

The change in the semantics of system_path() made the "get once and
keep returning it" this function does safer and correct, but this
function itself did not change any behaviour from its caller's point
of view.

>  {
> -	static const char *system_wide;
> +	static char *system_wide;
>  	if (!system_wide)
>  		system_wide = system_path(ETC_GITATTRIBUTES);
>  	return system_wide;


> @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
>  	attr_stack = elem;
>  
>  	if (git_attr_system()) {
> -		elem = read_attr_from_file(git_etc_gitattributes(), 1);
> +		char *etc_attributes = git_etc_gitattributes();
> +		elem = read_attr_from_file(etc_attributes, 1);
> +		free(etc_attributes);

And freeing here is actively wrong, I think.  You are freeing the
piece of memory still pointed by "static char *system_wide" in the
function git_etc_gitattributes(); when it is called again, the
caller will get a pointer into the memory you have freed here.

> diff --git a/builtin/config.c b/builtin/config.c
> index 15a7bea..266d42b 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		if (given_config_source.blob)
>  			die("editing blobs is not supported");
>  		git_config(git_default_config, NULL);
> -		config_file = xstrdup(given_config_source.file ?
> -				      given_config_source.file : git_path("config"));
> +
> +		if (use_system_config)
> +			config_file = given_config_source.file ?
> +							given_config_source.file : git_path("config");
> +		else
> +			config_file = xstrdup(given_config_source.file ?
> +								  given_config_source.file : git_path("config"));
> +

Sorry, but I do not understand this change.

Why do you need "if use-system-config, do not allocate" here and
then later everywhere "if use-system-config, free it"?  I would
understand if the change were "earlier we had mixture of borrowed
and owned strings, but we make sure we always own the string by
running xstrdup() in the caller when we used to let this function
borrow, so that we can always free() before returning from here",
or something.

For example, in the original code, use_local_config codepath uses
git_pathdup(), which is an allocated piece of memory, to initialize
given_config_source.file.  Doesn't it need to be freed?

Even if it is not in the use_system_config mode, if
given_config_source.file is not an absolute path, we store a copy of
prefix-filename result to given_config_source.file.  Doesn't it need
to be freed?

I think the implementation strategy you took makes the result
unnecessarily messy and error prone.  Let's step back a bit.

When you have code that sometimes borrows memory and sometimes owns
memory, writing the clean-up part like this is error prone:

	char *var;

	if (A)
		var = borrowed memory;
	else if (B)
		var = borrowed memory;
	else if (C)
		var = xstrdup(borrowed memory);
	else	
		var = borrowed memory;

	use var; /* a loong piece of code in reality */

	if (C)
        	free(var);

because the set-up part can and will change over time as the code
evolves.  If written this way:

	char *var, *to_free = NULL;

	if (A)
		var = borrowed memory;
	else if (B)
		var = borrowed memory;
	else if (C)
		to_free = var = xstrdup(borrowed memory);
	else	
		var = borrowed memory;

	use var; /* a loong piece of code in reality */

	free(to_free);

the clean-up part would not have to worry how the set-up part
decided when to borrow memory and when to own memory.  Another way
to do so would obviously be:

	char *var;

	if (A)
		var = xstrdup(borrowed memory);
	else if (B)
		var = xstrdup(borrowed memory);
	else if (C)
		var = xstrdup(borrowed memory);
	else	
		var = xstrdup(borrowed memory);

	use var; /* a loong piece of code in reality */

       	free(var);

to always own the memory, but depending on the original code, it may
make the result more inefficient and the patch more noisy.

This is especially true when the clean-up part is spread all over
the place like your patch does, i.e. the "use var" part of the code
looks like

	if (X) {
        	... a looong piece of code to use var ...
	        if (C)
                	free(var);
	} else if (Y) {
        	... a looong piece of code to use var ...
	        if (C)
                	free(var);
        } else {
        	... a looong piece of code to use var ...
	        if (C)
                	free(var);
	}

which is simply unmaintainable.  Better to make "use var" and the
clean-up part structured more like this, using the to_free trick:

	if (X) {
        	... a looong piece of code to use var ...
		goto cleanup;
	} else if (Y) {
        	... a looong piece of code to use var ...
		goto cleanup;
        } else {
        	... a looong piece of code to use var ...
		goto cleanup;
	}

	cleanup:
		free(to_free);

> diff --git a/builtin/help.c b/builtin/help.c
> index b3c818e..20ffbb1 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -326,7 +326,8 @@ static void setup_man_path(void)
>  	 * old_path, the ':' at the end will let 'man' to try
>  	 * system-wide paths after ours to find the manual page. If
>  	 * there is old_path, we need ':' as delimiter. */
> -	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
> +	char *git_man_path = system_path(GIT_MAN_PATH);

Defile variable in the definition block above to avoid future
decl-after-stmt, i.e.

	 const char *old_path = getenv("MANPATH");
	+char *git_man_path = system_path(GIT_MAN_PATH);

	 /* We should always put ...

> +	strbuf_addstr(&new_path, git_man_path);

And better free it immediately after you are done, instead of at the
end of the function.

>  	strbuf_addch(&new_path, ':');
>  	if (old_path)
>  		strbuf_addstr(&new_path, old_path);
> @@ -334,6 +335,7 @@ static void setup_man_path(void)
>  	setenv("MANPATH", new_path.buf, 1);
>  
>  	strbuf_release(&new_path);
> +	free(git_man_path);
>  }

> -static void get_html_page_path(struct strbuf *page_path, const char *page)
> +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)
>  {
>  	struct stat st;
> -	if (!html_path)
> -		html_path = system_path(GIT_HTML_PATH);
>  
>  	/* Check that we have a git documentation directory. */
>  	if (!strstr(html_path, "://")) {
>  		if (stat(mkpath("%s/git.html", html_path), &st)
> -		    || !S_ISREG(st.st_mode))
> -			die("'%s': not a documentation directory.", html_path);
> +				|| !S_ISREG(st.st_mode)){
> +			printf("'%s': not a documentation directory.\n", html_path);
> +			free(html_path);
> +			exit(1);

This is an unnecessary churn, and an unwarranted change to the
externally visible behaviour.  die() gives its message to the
standard error stream (not to the standard output stream), and exits
with 128 (not with 1).

We are exiting immediately after; it is not worth changing the API
to the caller or external behaviour, only to free(html_path)
conditionally.

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 2619ca5..6f36f3f 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)
>  		template_dir = init_db_template_dir;
>  	if (!template_dir)
>  		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
> -	if (!template_dir[0])
> +	if (!template_dir[0]){

A SP between ) and {

> +		if (!template_dir)
> +			free((char*)template_dir);

No need for "call free only if it is not NULL"; free(NULL) works
just fine.

>  		return;
> +	}
>  	template_len = strlen(template_dir);
> -	if (PATH_MAX <= (template_len+strlen("/config")))
> +	if (PATH_MAX <= (template_len+strlen("/config"))){
> +		if (!template_dir)
> +			free((char*)template_dir);
>  		die(_("insanely long template path %s"), template_dir);

You are freeing and then using it in the message, and it cannot be
NULL anyway so "if (!template_dir)" is doubly unnecessary.

I wouldn't bother freeing immediately before dying, exiting, or
execing.

Here is my quick attempt, starting solely from the output of "git
grep system_path".  How does it look?

One thing to note is that this illustration does not consider memory
pointed at by the "system_wide" variable here (from attr.c)

        static const char *git_etc_gitattributes(void)
        {
                static const char *system_wide;
                if (!system_wide)
                        system_wide = system_path(ETC_GITATTRIBUTES);
                return system_wide;
        }

at the point of process exit as a "leak".

-- >8 --
Subject: system_path(): always return free'able memory to the caller

The function sometimes returns a newly allocated string and
sometimes returns a borrowed string, the latter of which the callers
must not free().  The existing callers all assume that the return
value belongs to the callee and most of them copy it with strdup()
when they want to keep it around.  They end up leaking the returned
copy when the callee returned a new string because they cannot tell
if they should free it.

Change the contract between the callers and system_path() to make
the returned string owned by the callers; they are responsible for
freeing it when done, but they do not have to make their own copy to
store it away.

Adjust the callers to make sure they do not leak the returned string
once they are done, but do not bother freeing it just before dying,
exiting or exec'ing other program to avoid unnecessary churn.

Reported-by: Alexander Kuleshov <kuleshovmail@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/help.c    |  9 +++++++--
 builtin/init-db.c | 15 ++++++++++-----
 exec_cmd.c        |  7 +++----
 exec_cmd.h        |  2 +-
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index b3c818e..e78c135 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -321,16 +321,18 @@ static void setup_man_path(void)
 {
 	struct strbuf new_path = STRBUF_INIT;
 	const char *old_path = getenv("MANPATH");
+	char *git_man_path = system_path(GIT_MAN_PATH);
 
 	/* We should always put ':' after our path. If there is no
 	 * old_path, the ':' at the end will let 'man' to try
 	 * system-wide paths after ours to find the manual page. If
 	 * there is old_path, we need ':' as delimiter. */
-	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+	strbuf_addstr(&new_path, git_man_path);
 	strbuf_addch(&new_path, ':');
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
 
+	free(git_man_path);
 	setenv("MANPATH", new_path.buf, 1);
 
 	strbuf_release(&new_path);
@@ -380,8 +382,10 @@ static void show_info_page(const char *git_cmd)
 static void get_html_page_path(struct strbuf *page_path, const char *page)
 {
 	struct stat st;
+	char *to_free = NULL;
+
 	if (!html_path)
-		html_path = system_path(GIT_HTML_PATH);
+		html_path = to_free = system_path(GIT_HTML_PATH);
 
 	/* Check that we have a git documentation directory. */
 	if (!strstr(html_path, "://")) {
@@ -392,6 +396,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
 
 	strbuf_init(page_path, 0);
 	strbuf_addf(page_path, "%s/%s.html", html_path, page);
+	free(to_free);
 }
 
 /*
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 2619ca5..9966522 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -119,15 +119,18 @@ static void copy_templates(const char *template_dir)
 	DIR *dir;
 	const char *git_dir = get_git_dir();
 	int len = strlen(git_dir);
+	char *to_free = NULL;
 
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
 		template_dir = init_db_template_dir;
 	if (!template_dir)
-		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
-	if (!template_dir[0])
+		template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
+	if (!template_dir[0]) {
+		free(to_free);
 		return;
+	}
 	template_len = strlen(template_dir);
 	if (PATH_MAX <= (template_len+strlen("/config")))
 		die(_("insanely long template path %s"), template_dir);
@@ -139,7 +142,7 @@ static void copy_templates(const char *template_dir)
 	dir = opendir(template_path);
 	if (!dir) {
 		warning(_("templates not found %s"), template_dir);
-		return;
+		goto free_return;
 	}
 
 	/* Make sure that template is from the correct vintage */
@@ -155,8 +158,7 @@ static void copy_templates(const char *template_dir)
 			"a wrong format version %d from '%s'"),
 			repository_format_version,
 			template_dir);
-		closedir(dir);
-		return;
+		goto close_free_return;
 	}
 
 	memcpy(path, git_dir, len);
@@ -166,7 +168,10 @@ static void copy_templates(const char *template_dir)
 	copy_templates_1(path, len,
 			 template_path, template_len,
 			 dir);
+close_free_return:
 	closedir(dir);
+free_return:
+	free(to_free);
 }
 
 static int git_init_db_config(const char *k, const char *v, void *cb)
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..8ab37b5 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
 	static const char *prefix;
@@ -16,7 +16,7 @@ const char *system_path(const char *path)
 	struct strbuf d = STRBUF_INIT;
 
 	if (is_absolute_path(path))
-		return path;
+		return xstrdup(path);
 
 #ifdef RUNTIME_PREFIX
 	assert(argv0_path);
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif
 
 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	return strbuf_detach(&d, NULL);
 }
 
 const char *git_extract_argv0_path(const char *argv0)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..93b0c02 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -9,6 +9,6 @@ extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
 
 #endif /* GIT_EXEC_CMD_H */

Re: Re*: [PATCH] change contract between system_path and it's callers

[Alex Kuleshov]

Junio C Hamano <gitster@pobox.com> @ 2014-11-25 01:33 ALMT:

>> -static const char *git_etc_gitattributes(void)
>> +static char *git_etc_gitattributes(void)
>
> Hmph, I think this should keep returning "const char *", as the
> caller is not expected to free the pointer or write into the memory
> held by the returned string.  The same applies to the change of the
> type in "struct git_config_source".
>
> The change in the semantics of system_path() made the "get once and
> keep returning it" this function does safer and correct, but this
> function itself did not change any behaviour from its caller's point
> of view.

Understand, will fix it.

>
>>  {
>> -	static const char *system_wide;
>> +	static char *system_wide;
>>  	if (!system_wide)
>>  		system_wide = system_path(ETC_GITATTRIBUTES);
>>  	return system_wide;
>
>
>> @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
>>  	attr_stack = elem;
>>
>>  	if (git_attr_system()) {
>> -		elem = read_attr_from_file(git_etc_gitattributes(), 1);
>> +		char *etc_attributes = git_etc_gitattributes();
>> +		elem = read_attr_from_file(etc_attributes, 1);
>> +		free(etc_attributes);
>
> And freeing here is actively wrong, I think.  You are freeing the
> piece of memory still pointed by "static char *system_wide" in the
> function git_etc_gitattributes(); when it is called again, the
> caller will get a pointer into the memory you have freed here.

Why? If i understand correctly we don't use etc_attributes anymore in
this function and if we'll call this function again
git_etc_gitattributes will create new pointer and system_path alloc
memory for it or i'm wrong with it?

>
>> diff --git a/builtin/config.c b/builtin/config.c
>> index 15a7bea..266d42b 100644
>> --- a/builtin/config.c
>> +++ b/builtin/config.c
>> @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>>  		if (given_config_source.blob)
>>  			die("editing blobs is not supported");
>>  		git_config(git_default_config, NULL);
>> -		config_file = xstrdup(given_config_source.file ?
>> -				      given_config_source.file : git_path("config"));
>> +
>> +		if (use_system_config)
>> +			config_file = given_config_source.file ?
>> +							given_config_source.file : git_path("config");
>> +		else
>> +			config_file = xstrdup(given_config_source.file ?
>> +								  given_config_source.file : git_path("config"));
>> +
>
> Sorry, but I do not understand this change.
>
> Why do you need "if use-system-config, do not allocate" here and
> then later everywhere "if use-system-config, free it"?  I would
> understand if the change were "earlier we had mixture of borrowed
> and owned strings, but we make sure we always own the string by
> running xstrdup() in the caller when we used to let this function
> borrow, so that we can always free() before returning from here",
> or something.
>
> For example, in the original code, use_local_config codepath uses
> git_pathdup(), which is an allocated piece of memory, to initialize
> given_config_source.file.  Doesn't it need to be freed?
>
> Even if it is not in the use_system_config mode, if
> given_config_source.file is not an absolute path, we store a copy of
> prefix-filename result to given_config_source.file.  Doesn't it need
> to be freed?
>
> I think the implementation strategy you took makes the result
> unnecessarily messy and error prone.  Let's step back a bit.
>
> When you have code that sometimes borrows memory and sometimes owns
> memory, writing the clean-up part like this is error prone:
>
> 	char *var;
>
> 	if (A)
> 		var = borrowed memory;
> 	else if (B)
> 		var = borrowed memory;
> 	else if (C)
> 		var = xstrdup(borrowed memory);
> 	else
> 		var = borrowed memory;
>
> 	use var; /* a loong piece of code in reality */
>
> 	if (C)
>         	free(var);
>
> because the set-up part can and will change over time as the code
> evolves.  If written this way:
>
> 	char *var, *to_free = NULL;
>
> 	if (A)
> 		var = borrowed memory;
> 	else if (B)
> 		var = borrowed memory;
> 	else if (C)
> 		to_free = var = xstrdup(borrowed memory);
> 	else
> 		var = borrowed memory;
>
> 	use var; /* a loong piece of code in reality */
>
> 	free(to_free);
>
> the clean-up part would not have to worry how the set-up part
> decided when to borrow memory and when to own memory.  Another way
> to do so would obviously be:
>
> 	char *var;
>
> 	if (A)
> 		var = xstrdup(borrowed memory);
> 	else if (B)
> 		var = xstrdup(borrowed memory);
> 	else if (C)
> 		var = xstrdup(borrowed memory);
> 	else
> 		var = xstrdup(borrowed memory);
>
> 	use var; /* a loong piece of code in reality */
>
>        	free(var);
>
> to always own the memory, but depending on the original code, it may
> make the result more inefficient and the patch more noisy.
>
> This is especially true when the clean-up part is spread all over
> the place like your patch does, i.e. the "use var" part of the code
> looks like
>
> 	if (X) {
>         	... a looong piece of code to use var ...
> 	        if (C)
>                 	free(var);
> 	} else if (Y) {
>         	... a looong piece of code to use var ...
> 	        if (C)
>                 	free(var);
>         } else {
>         	... a looong piece of code to use var ...
> 	        if (C)
>                 	free(var);
> 	}
>
> which is simply unmaintainable.  Better to make "use var" and the
> clean-up part structured more like this, using the to_free trick:
>
> 	if (X) {
>         	... a looong piece of code to use var ...
> 		goto cleanup;
> 	} else if (Y) {
>         	... a looong piece of code to use var ...
> 		goto cleanup;
>         } else {
>         	... a looong piece of code to use var ...
> 		goto cleanup;
> 	}
>
> 	cleanup:
> 		free(to_free);
>

Will look again on it tomorrow and hope send new patch (sorry deep night
today here), anyway thanks for great explanation.

>> diff --git a/builtin/help.c b/builtin/help.c
>> index b3c818e..20ffbb1 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -326,7 +326,8 @@ static void setup_man_path(void)
>>  	 * old_path, the ':' at the end will let 'man' to try
>>  	 * system-wide paths after ours to find the manual page. If
>>  	 * there is old_path, we need ':' as delimiter. */
>> -	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
>> +	char *git_man_path = system_path(GIT_MAN_PATH);
>
> Defile variable in the definition block above to avoid future
> decl-after-stmt, i.e.
>
> 	 const char *old_path = getenv("MANPATH");
> 	+char *git_man_path = system_path(GIT_MAN_PATH);
>
> 	 /* We should always put ...
>
>> +	strbuf_addstr(&new_path, git_man_path);
>
> And better free it immediately after you are done, instead of at the
> end of the function.

Will fix it.

>
>>  	strbuf_addch(&new_path, ':');
>>  	if (old_path)
>>  		strbuf_addstr(&new_path, old_path);
>> @@ -334,6 +335,7 @@ static void setup_man_path(void)
>>  	setenv("MANPATH", new_path.buf, 1);
>>
>>  	strbuf_release(&new_path);
>> +	free(git_man_path);
>>  }
>
>> -static void get_html_page_path(struct strbuf *page_path, const char *page)
>> +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)
>>  {
>>  	struct stat st;
>> -	if (!html_path)
>> -		html_path = system_path(GIT_HTML_PATH);
>>
>>  	/* Check that we have a git documentation directory. */
>>  	if (!strstr(html_path, "://")) {
>>  		if (stat(mkpath("%s/git.html", html_path), &st)
>> -		    || !S_ISREG(st.st_mode))
>> -			die("'%s': not a documentation directory.", html_path);
>> +				|| !S_ISREG(st.st_mode)){
>> +			printf("'%s': not a documentation directory.\n", html_path);
>> +			free(html_path);
>> +			exit(1);
>
> This is an unnecessary churn, and an unwarranted change to the
> externally visible behaviour.  die() gives its message to the
> standard error stream (not to the standard output stream), and exits
> with 128 (not with 1).
>
> We are exiting immediately after; it is not worth changing the API
> to the caller or external behaviour, only to free(html_path)
> conditionally.

So how can i free html_path here if when i'll return die instead exit? I
can't use free(html_path) before/after die call.

>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 2619ca5..6f36f3f 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)
>>  		template_dir = init_db_template_dir;
>>  	if (!template_dir)
>>  		template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
>> -	if (!template_dir[0])
>> +	if (!template_dir[0]){
>
> A SP between ) and {
>
>> +		if (!template_dir)
>> +			free((char*)template_dir);
>
> No need for "call free only if it is not NULL"; free(NULL) works
> just fine.

Will fix it.

>
>>  		return;
>> +	}
>>  	template_len = strlen(template_dir);
>> -	if (PATH_MAX <= (template_len+strlen("/config")))
>> +	if (PATH_MAX <= (template_len+strlen("/config"))){
>> +		if (!template_dir)
>> +			free((char*)template_dir);
>>  		die(_("insanely long template path %s"), template_dir);
>
> You are freeing and then using it in the message, and it cannot be
> NULL anyway so "if (!template_dir)" is doubly unnecessary.
>
> I wouldn't bother freeing immediately before dying, exiting, or
> execing.
>
> Here is my quick attempt, starting solely from the output of "git
> grep system_path".  How does it look?

It looks fantastic :)

>
> One thing to note is that this illustration does not consider memory
> pointed at by the "system_wide" variable here (from attr.c)
>
>         static const char *git_etc_gitattributes(void)
>         {
>                 static const char *system_wide;
>                 if (!system_wide)
>                         system_wide = system_path(ETC_GITATTRIBUTES);
>                 return system_wide;
>         }
>
> at the point of process exit as a "leak".

But why? We allocated memory to "system_wide" with system_path, next git
will exit somewhere with die, but system_wide didn't free... Or i'm
wrong here too?

Anyway many thanks for so great feedback, tomorrow I'll reread it and
will try to do all things in correct way.

Thanky you.

Re: Re*: [PATCH] change contract between system_path and it's callers

[Junio C Hamano]

Alex Kuleshov <kuleshovmail@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> @ 2014-11-25 01:33 ALMT:
> ...
>>>  	if (git_attr_system()) {
>>> -		elem = read_attr_from_file(git_etc_gitattributes(), 1);
>>> +		char *etc_attributes = git_etc_gitattributes();
>>> +		elem = read_attr_from_file(etc_attributes, 1);
>>> +		free(etc_attributes);
>>
>> And freeing here is actively wrong, I think.  You are freeing the
>> piece of memory still pointed by "static char *system_wide" in the
>> function git_etc_gitattributes(); when it is called again, the
>> caller will get a pointer into the memory you have freed here.
>
> Why? If i understand correctly we don't use etc_attributes anymore in
> this function and if we'll call this function again
> git_etc_gitattributes will create new pointer and system_path alloc
> memory for it or i'm wrong with it?

The function keeps a singleton in "static const char *system_wide"
so that it has to call system_path() only once, and keeps the value
for second and subsequent calls.  From its callers' point of view,
they are only peeking the memory it returns.

This aspect does not change with your patch.

    -static const char *git_etc_gitattributes(void)
    +static char *git_etc_gitattributes(void)
     {
    -	static const char *system_wide;
    +	static char *system_wide;
            if (!system_wide)
                    system_wide = system_path(ETC_GITATTRIBUTES);
            return system_wide;

Re: Re*: [PATCH] change contract between system_path and it's callers

[Junio C Hamano]

Alex Kuleshov <kuleshovmail@gmail.com> writes:

>> One thing to note is that this illustration does not consider memory
>> pointed at by the "system_wide" variable here (from attr.c)
>>
>>         static const char *git_etc_gitattributes(void)
>>         {
>>                 static const char *system_wide;
>>                 if (!system_wide)
>>                         system_wide = system_path(ETC_GITATTRIBUTES);
>>                 return system_wide;
>>         }
>>
>> at the point of process exit as a "leak".
>
> But why? We allocated memory to "system_wide" with system_path, next git
> will exit somewhere with die, but system_wide didn't free... Or i'm
> wrong here too?

It is in the same league as "static const char *git_dir" and friends
that appear in the file-scope-static of environment.c.  Keeping small
things around to be cleaned up by exit() is not a crime.

Re: Re*: [PATCH] change contract between system_path and it's callers

[Alexander Kuleshov]

Hello Junio,

I returned static to git_etc_gitattributes return value, but now i
can't understand where to free 'system_wide'. As i understand
correctly attr.c has following API for querying attributes:

git_attr

git_check_attr

And as i see in code git_attr doesn't use git_attr_check, so now we
have only git_check_attr and git_all_stars functions which are through
prepare_attr_stack and bootstrap_attr_stack in it uses path which
returned
 by system_path. So you're right if i free etc_attributes like this
and git_etc_gitattributes will return static value we'll have access
to freed memory if it will called again. But we have no access to
etc_attributes outside bootstrap_attr_stack so where will be best
place to free it?

Thank you

2014-11-25 2:50 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
> Alex Kuleshov <kuleshovmail@gmail.com> writes:
>
>>> One thing to note is that this illustration does not consider memory
>>> pointed at by the "system_wide" variable here (from attr.c)
>>>
>>>         static const char *git_etc_gitattributes(void)
>>>         {
>>>                 static const char *system_wide;
>>>                 if (!system_wide)
>>>                         system_wide = system_path(ETC_GITATTRIBUTES);
>>>                 return system_wide;
>>>         }
>>>
>>> at the point of process exit as a "leak".
>>
>> But why? We allocated memory to "system_wide" with system_path, next git
>> will exit somewhere with die, but system_wide didn't free... Or i'm
>> wrong here too?
>
> It is in the same league as "static const char *git_dir" and friends
> that appear in the file-scope-static of environment.c.  Keeping small
> things around to be cleaned up by exit() is not a crime.



-- 
_________________________
0xAX

Re: Re*: [PATCH] change contract between system_path and it's callers

[Alexander Kuleshov]

But if we still static const char* for git_etc_gitattributes and will
not free it in bootstrap_attr_stack there will no memory leak, just
tested it, so i'll remove free for it.

2014-11-25 12:45 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
> Hello Junio,
>
> I returned static to git_etc_gitattributes return value, but now i
> can't understand where to free 'system_wide'. As i understand
> correctly attr.c has following API for querying attributes:
>
> git_attr
>
> git_check_attr
>
> And as i see in code git_attr doesn't use git_attr_check, so now we
> have only git_check_attr and git_all_stars functions which are through
> prepare_attr_stack and bootstrap_attr_stack in it uses path which
> returned
>  by system_path. So you're right if i free etc_attributes like this
> and git_etc_gitattributes will return static value we'll have access
> to freed memory if it will called again. But we have no access to
> etc_attributes outside bootstrap_attr_stack so where will be best
> place to free it?
>
> Thank you
>
> 2014-11-25 2:50 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
>> Alex Kuleshov <kuleshovmail@gmail.com> writes:
>>
>>>> One thing to note is that this illustration does not consider memory
>>>> pointed at by the "system_wide" variable here (from attr.c)
>>>>
>>>>         static const char *git_etc_gitattributes(void)
>>>>         {
>>>>                 static const char *system_wide;
>>>>                 if (!system_wide)
>>>>                         system_wide = system_path(ETC_GITATTRIBUTES);
>>>>                 return system_wide;
>>>>         }
>>>>
>>>> at the point of process exit as a "leak".
>>>
>>> But why? We allocated memory to "system_wide" with system_path, next git
>>> will exit somewhere with die, but system_wide didn't free... Or i'm
>>> wrong here too?
>>
>> It is in the same league as "static const char *git_dir" and friends
>> that appear in the file-scope-static of environment.c.  Keeping small
>> things around to be cleaned up by exit() is not a crime.
>
>
>
> --
> _________________________
> 0xAX



-- 
_________________________
0xAX

Re: Re*: [PATCH] change contract between system_path and it's callers

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> But if we still static const char* for git_etc_gitattributes and will
> not free it in bootstrap_attr_stack there will no memory leak, just
> tested it, so i'll remove free for it.

Leak or no leak, freeing there is wrong.  It will free the piece of
memory the next caller will obtain from the git_etc_gitattributes()
function.  In other words, the return value from that function is
owned by git_etc_gitattributes(), not by the caller.

As to "leak", in the strictest sense of the word, i.e. "do we
allocate on the heap and exit without freeing?", it _is_ leaking,
and your above "just tested it" probably means "the tool I used did
not find/report it".  But as I said, it is not something worth
bothering about [*1*].

Think of it this way.

The git_etc_gitattributes() function goes like this with your patch
(and there is no fundamental difference in the original version,
which uses "const char *" where appropriate):

         static char *git_etc_gitattributes(void)
         {
                static char *system_wide;
                if (!system_wide)
                        system_wide = system_path(ETC_GITATTRIBUTES);
                return system_wide;
         }

If you knew that the pathname for /etc/gitattributes substitute will
never be longer than 256 bytes, you may have coded the above like so
instead:

        static char system_wide[256];
        static char *git_etc_gitattributes(void)
        {
                if (!system_wide[0]) {
                        char *ret = system_path(ETC_GITATTRIBUTES);
                        if (strncpy(system_wide, ret, 256) >= 256)
                                die("ETC_GITATTRIBUTES too long ");
                }       
                return system_wide;
        }

Even though we used the memory occupied by system_wide[] and did not
clean up before exit(), nobody would complain about leaking.

The existing code is the moral equivalent of the "up to 256 bytes"
illustration, but for a string whose size is not known at compile
time.  It is using and keeping the memory until program exit.
Nobody should complain about leaking.

That is, unless (1) the held memory is expected to be very large and
(2) you can prove that after the point you decide to insert free(),
nobody will ever need that information again.


[Footnote]

*1* The leaks we care about are of this form:

    void silly_leaker(void)
    {
        printf("%s\n", system_path(ETC_GITATTRIBUTES));
    }

    where each invocation allocates memory, uses it and then loses
    the reference to it without doing anything to clean-up.  You can
    call such a function unbounded number of times and waste
    unbounded amount of memory.

    The implementation of git_etc_gitattributes() is not of that
    kind.  An invocation of it allocates, uses and keeps.  The
    second and subsequent invocation does not allocate.  When you
    call it unbounded number of times, it does not waste unbounded
    amount of memory.  It just keeps what it needs to answer the
    next caller with.

    The pattern needs to be made thread-safe, but that is a separate
    topic.

Re: Re*: [PATCH] change contract between system_path and it's callers

[Alexander Kuleshov]

Hello Junio,

I'm working on this patch and builtin/config.c, i have a little question:

Look, here - https://github.com/git/git/blob/master/builtin/config.c#L509
and in the next if clauses we get allocated path, but if i test it
with given config file with git config -f....
(https://github.com/git/git/blob/master/builtin/config.c#L513) i have
prefix NULL and next if clause doesn't execute, so if i'll put
free(given_config_source.file) after config action i'll get error
because given_config_source.file wasn't allocated. How to be with
this?

Thank you.

2014-11-25 13:04 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
> But if we still static const char* for git_etc_gitattributes and will
> not free it in bootstrap_attr_stack there will no memory leak, just
> tested it, so i'll remove free for it.
>
> 2014-11-25 12:45 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
>> Hello Junio,
>>
>> I returned static to git_etc_gitattributes return value, but now i
>> can't understand where to free 'system_wide'. As i understand
>> correctly attr.c has following API for querying attributes:
>>
>> git_attr
>>
>> git_check_attr
>>
>> And as i see in code git_attr doesn't use git_attr_check, so now we
>> have only git_check_attr and git_all_stars functions which are through
>> prepare_attr_stack and bootstrap_attr_stack in it uses path which
>> returned
>>  by system_path. So you're right if i free etc_attributes like this
>> and git_etc_gitattributes will return static value we'll have access
>> to freed memory if it will called again. But we have no access to
>> etc_attributes outside bootstrap_attr_stack so where will be best
>> place to free it?
>>
>> Thank you
>>
>> 2014-11-25 2:50 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
>>> Alex Kuleshov <kuleshovmail@gmail.com> writes:
>>>
>>>>> One thing to note is that this illustration does not consider memory
>>>>> pointed at by the "system_wide" variable here (from attr.c)
>>>>>
>>>>>         static const char *git_etc_gitattributes(void)
>>>>>         {
>>>>>                 static const char *system_wide;
>>>>>                 if (!system_wide)
>>>>>                         system_wide = system_path(ETC_GITATTRIBUTES);
>>>>>                 return system_wide;
>>>>>         }
>>>>>
>>>>> at the point of process exit as a "leak".
>>>>
>>>> But why? We allocated memory to "system_wide" with system_path, next git
>>>> will exit somewhere with die, but system_wide didn't free... Or i'm
>>>> wrong here too?
>>>
>>> It is in the same league as "static const char *git_dir" and friends
>>> that appear in the file-scope-static of environment.c.  Keeping small
>>> things around to be cleaned up by exit() is not a crime.
>>
>>
>>
>> --
>> _________________________
>> 0xAX
>
>
>
> --
> _________________________
> 0xAX



-- 
_________________________
0xAX

Re: Re*: [PATCH] change contract between system_path and it's callers

[Alexander Kuleshov]

Ah, you little ahead me, so we do not care about config.c in this way,
because it takes git_etc_gitconfig() in the same way as
git_etc_gitattributes

2014-11-25 23:55 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
> Alexander Kuleshov <kuleshovmail@gmail.com> writes:
>
>> But if we still static const char* for git_etc_gitattributes and will
>> not free it in bootstrap_attr_stack there will no memory leak, just
>> tested it, so i'll remove free for it.
>
> Leak or no leak, freeing there is wrong.  It will free the piece of
> memory the next caller will obtain from the git_etc_gitattributes()
> function.  In other words, the return value from that function is
> owned by git_etc_gitattributes(), not by the caller.
>
> As to "leak", in the strictest sense of the word, i.e. "do we
> allocate on the heap and exit without freeing?", it _is_ leaking,
> and your above "just tested it" probably means "the tool I used did
> not find/report it".  But as I said, it is not something worth
> bothering about [*1*].
>
> Think of it this way.
>
> The git_etc_gitattributes() function goes like this with your patch
> (and there is no fundamental difference in the original version,
> which uses "const char *" where appropriate):
>
>          static char *git_etc_gitattributes(void)
>          {
>                 static char *system_wide;
>                 if (!system_wide)
>                         system_wide = system_path(ETC_GITATTRIBUTES);
>                 return system_wide;
>          }
>
> If you knew that the pathname for /etc/gitattributes substitute will
> never be longer than 256 bytes, you may have coded the above like so
> instead:
>
>         static char system_wide[256];
>         static char *git_etc_gitattributes(void)
>         {
>                 if (!system_wide[0]) {
>                         char *ret = system_path(ETC_GITATTRIBUTES);
>                         if (strncpy(system_wide, ret, 256) >= 256)
>                                 die("ETC_GITATTRIBUTES too long ");
>                 }
>                 return system_wide;
>         }
>
> Even though we used the memory occupied by system_wide[] and did not
> clean up before exit(), nobody would complain about leaking.
>
> The existing code is the moral equivalent of the "up to 256 bytes"
> illustration, but for a string whose size is not known at compile
> time.  It is using and keeping the memory until program exit.
> Nobody should complain about leaking.
>
> That is, unless (1) the held memory is expected to be very large and
> (2) you can prove that after the point you decide to insert free(),
> nobody will ever need that information again.
>
>
> [Footnote]
>
> *1* The leaks we care about are of this form:
>
>     void silly_leaker(void)
>     {
>         printf("%s\n", system_path(ETC_GITATTRIBUTES));
>     }
>
>     where each invocation allocates memory, uses it and then loses
>     the reference to it without doing anything to clean-up.  You can
>     call such a function unbounded number of times and waste
>     unbounded amount of memory.
>
>     The implementation of git_etc_gitattributes() is not of that
>     kind.  An invocation of it allocates, uses and keeps.  The
>     second and subsequent invocation does not allocate.  When you
>     call it unbounded number of times, it does not waste unbounded
>     amount of memory.  It just keeps what it needs to answer the
>     next caller with.
>
>     The pattern needs to be made thread-safe, but that is a separate
>     topic.



-- 
_________________________
0xAX

[PATCH 1/1] change contract between system_path and it's callers

[Alexander Kuleshov]

Now system_path returns path which is allocated string to callers;
It prevents memory leaks in some places. All callers of system_path
are owners of path string and they must release it.

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 builtin/help.c | 10 +++++++---
 exec_cmd.c     | 17 +++++++++--------
 exec_cmd.h     |  4 ++--
 git.c          | 16 ++++++++++++----
 4 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index b3c818e..544d1cc 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -321,12 +321,13 @@ static void setup_man_path(void)
 {
 	struct strbuf new_path = STRBUF_INIT;
 	const char *old_path = getenv("MANPATH");
-
+	char *git_man_path = system_path(GIT_MAN_PATH);
 	/* We should always put ':' after our path. If there is no
 	 * old_path, the ':' at the end will let 'man' to try
 	 * system-wide paths after ours to find the manual page. If
 	 * there is old_path, we need ':' as delimiter. */
-	strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));
+	strbuf_addstr(&new_path, git_man_path);
+	free(git_man_path);
 	strbuf_addch(&new_path, ':');
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
@@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)
 static void show_info_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
-	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
+	char *git_info_path = system_path(GIT_INFO_PATH);
+	setenv("INFOPATH", git_info_path, 1);
+	free(git_info_path);
 	execlp("info", "info", "gitman", page, (char *)NULL);
 	die(_("no info viewer handled the request"));
 }
@@ -392,6 +395,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
 
 	strbuf_init(page_path, 0);
 	strbuf_addf(page_path, "%s/%s.html", html_path, page);
+	free((char*)html_path);
 }
 
 /*
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..e3ebdd6 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 #ifdef RUNTIME_PREFIX
 	static const char *prefix;
@@ -16,7 +16,7 @@ const char *system_path(const char *path)
 	struct strbuf d = STRBUF_INIT;
 
 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);
 
 #ifdef RUNTIME_PREFIX
 	assert(argv0_path);
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
 #endif
 
 	strbuf_addf(&d, "%s/%s", prefix, path);
-	path = strbuf_detach(&d, NULL);
-	return path;
+	return d.buf;
 }
 
 const char *git_extract_argv0_path(const char *argv0)
@@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
 
 
 /* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
 {
 	const char *env;
 
 	if (argv_exec_path)
-		return argv_exec_path;
+		return strdup(argv_exec_path);
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
 	if (env && *env) {
-		return env;
+		return strdup(env);
 	}
 
 	return system_path(GIT_EXEC_PATH);
@@ -95,8 +94,10 @@ void setup_path(void)
 {
 	const char *old_path = getenv("PATH");
 	struct strbuf new_path = STRBUF_INIT;
+	char* exec_path = git_exec_path();
 
-	add_path(&new_path, git_exec_path());
+	add_path(&new_path, exec_path);
+	free(exec_path);
 	add_path(&new_path, argv0_path);
 
 	if (old_path)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@
 
 extern void git_set_argv_exec_path(const char *exec_path);
 extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
 LAST_ARG_MUST_BE_NULL
 extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
 
 #endif /* GIT_EXEC_CMD_H */
diff --git a/git.c b/git.c
index 82d7a1c..d01c4f1 100644
--- a/git.c
+++ b/git.c
@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			if (*cmd == '=')
 				git_set_argv_exec_path(cmd + 1);
 			else {
-				puts(git_exec_path());
+				char *exec_path = git_exec_path();
+				puts(exec_path);
+				free(exec_path);
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "--html-path")) {
-			puts(system_path(GIT_HTML_PATH));
+			char *git_html_path = system_path(GIT_HTML_PATH);
+			puts(git_html_path);
+			free(git_html_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--man-path")) {
-			puts(system_path(GIT_MAN_PATH));
+			char *git_man_path = system_path(GIT_MAN_PATH);
+			puts(git_man_path);
+			free(git_man_path);
 			exit(0);
 		} else if (!strcmp(cmd, "--info-path")) {
-			puts(system_path(GIT_INFO_PATH));
+			char *git_info_path = system_path(GIT_INFO_PATH);
+			puts(git_info_path);
+			free(git_info_path);
 			exit(0);
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
 			use_pager = 1;
-- 
2.2.0.rc3.205.gea90ae3.dirty

Re: Re*: [PATCH] change contract between system_path and it's callers

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> Ah, you little ahead me, so we do not care about config.c in this way,
> because it takes git_etc_gitconfig() in the same way as
> git_etc_gitattributes

Yes, but looking at the file, you would notice that the
"use_local_config" codepath assigns a string obtained from
git_pathdup() to given_config_source.file, which means that somebody
later in the code path would not know if given_config_source.file is
merely pointing at a piece of memory owned by somebody else
(e.g. came from git_etc_gitconfig()) or if it owns the piece of
memory (e.g. came from git_pathdup()).  In the former case the
memory should never be freed, but not freeing in the latter would
leak the memory.

Assignment to given_config_source.file I see in that function that
borrows (i.e. the current code that does not fee is correct) are:

  - getenv(CONFIG_ENVIRONMENT)
  - NULL ;-) obviously
  - git_etc_gitconfig()
  
All the other assignment to given_config_source.file (including the
ones obtained from a call to home_config_paths()) makes us
responsible to free that piece memory, so they are technically
leaks.

But cmd_config() is a moral equivalent of main() in a typical
program, so it might not be worth worrying about allocating a single
piece of memory that is used throughout the lifetime of that
function and leaving it without freeing.

    

Re: [PATCH 1/1] change contract between system_path and it's callers

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> Now system_path returns path which is allocated string to callers;
> It prevents memory leaks in some places. All callers of system_path
> are owners of path string and they must release it.
>
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---

Comparing this with what I sent out...

>  builtin/help.c | 10 +++++++---
>  exec_cmd.c     | 17 +++++++++--------
>  exec_cmd.h     |  4 ++--
>  git.c          | 16 ++++++++++++----
>  4 files changed, 30 insertions(+), 17 deletions(-)
>
> @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)
>  static void show_info_page(const char *git_cmd)
>  {
>  	const char *page = cmd_to_page(git_cmd);
> -	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
> +	char *git_info_path = system_path(GIT_INFO_PATH);
> +	setenv("INFOPATH", git_info_path, 1);
> +	free(git_info_path);

We are just about to exec; does this warrant the code churn?

>  	execlp("info", "info", "gitman", page, (char *)NULL);
>  	die(_("no info viewer handled the request"));

> @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>  #endif
>  
>  	strbuf_addf(&d, "%s/%s", prefix, path);
> -	path = strbuf_detach(&d, NULL);
> -	return path;
> +	return d.buf;

These happens to be the same with the current strbuf implementation,
but it is a good manner to use strbuf_detach(&d, NULL) here.  We
don't know what other de-initialization tomorrow's implementation of
the strbuf API may have to do in strbuf_detach().

> @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
>  
>  
>  /* Returns the highest-priority, location to look for git programs. */
> -const char *git_exec_path(void)
> +char *git_exec_path(void)
>  {
>  	const char *env;
>  
>  	if (argv_exec_path)
> -		return argv_exec_path;
> +		return strdup(argv_exec_path);
>  
>  	env = getenv(EXEC_PATH_ENVIRONMENT);
>  	if (env && *env) {
> -		return env;
> +		return strdup(env);
>  	}

Now you are making callers of git_exec_path() responsible for
freeing the result they receive.

git_exec_path() may be called quite a lot, which means we may end up
calling system_path() many times during the life of a process
without freeing its return value, so this change may be worth doing,
but this patch is insufficient, isn't it?

You just added load_command_list() in help.c a new leak or two, for
example.  There probably are other callers of this function but I
don't have time to look at all of them myself right now.

> @@ -95,8 +94,10 @@ void setup_path(void)
>  {
>  	const char *old_path = getenv("PATH");
>  	struct strbuf new_path = STRBUF_INIT;
> +	char* exec_path = git_exec_path();
>  
> -	add_path(&new_path, git_exec_path());
> +	add_path(&new_path, exec_path);
> +	free(exec_path);
>  	add_path(&new_path, argv0_path);

This part by itself is good, provided if we make it the caller's
responsiblity to free string returned by git_exec_path().

> diff --git a/git.c b/git.c
> index 82d7a1c..d01c4f1 100644
> --- a/git.c
> +++ b/git.c
> @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			if (*cmd == '=')
>  				git_set_argv_exec_path(cmd + 1);
>  			else {
> -				puts(git_exec_path());
> +				char *exec_path = git_exec_path();
> +				puts(exec_path);
> +				free(exec_path);
>  				exit(0);
>  			}
>  		} else if (!strcmp(cmd, "--html-path")) {
> -			puts(system_path(GIT_HTML_PATH));
> +			char *git_html_path = system_path(GIT_HTML_PATH);
> +			puts(git_html_path);
> +			free(git_html_path);
>  			exit(0);
>  		} else if (!strcmp(cmd, "--man-path")) {
> -			puts(system_path(GIT_MAN_PATH));
> +			char *git_man_path = system_path(GIT_MAN_PATH);
> +			puts(git_man_path);
> +			free(git_man_path);
>  			exit(0);
>  		} else if (!strcmp(cmd, "--info-path")) {
> -			puts(system_path(GIT_INFO_PATH));
> +			char *git_info_path = system_path(GIT_INFO_PATH);
> +			puts(git_info_path);
> +			free(git_info_path);
>  			exit(0);
>  		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
>  			use_pager = 1;

None of these warrant the code churn, I would say.

Re: [PATCH 1/1] change contract between system_path and it's callers

[Alexander Kuleshov]

>
> Comparing this with what I sent out...
>
> >  builtin/help.c | 10 +++++++---
> >  exec_cmd.c     | 17 +++++++++--------
> >  exec_cmd.h     |  4 ++--
> >  git.c          | 16 ++++++++++++----
> >  4 files changed, 30 insertions(+), 17 deletions(-)
> >
> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)
> >  static void show_info_page(const char *git_cmd)
> >  {
> >       const char *page = cmd_to_page(git_cmd);
> > -     setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
> > +     char *git_info_path = system_path(GIT_INFO_PATH);
> > +     setenv("INFOPATH", git_info_path, 1);
> > +     free(git_info_path);
>
> We are just about to exec; does this warrant the code churn?

hmm... Can't understand what's the problem here? We get git_info_path
from system_path which returns pointer which will need to free, set it in
environment var and than free it...

>
> >       execlp("info", "info", "gitman", page, (char *)NULL);
> >       die(_("no info viewer handled the request"));
>
> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)
> >  #endif
> >
> >       strbuf_addf(&d, "%s/%s", prefix, path);
> > -     path = strbuf_detach(&d, NULL);
> > -     return path;
> > +     return d.buf;
>
> These happens to be the same with the current strbuf implementation,
> but it is a good manner to use strbuf_detach(&d, NULL) here.  We
> don't know what other de-initialization tomorrow's implementation of
> the strbuf API may have to do in strbuf_detach().

How to do it in correct way?


    strbuf_addf(&d, "%s/%s", prefix, path);
    path = strbuf_detach(&d, NULL);
    return (char*)path;

Or something else?

>
> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
> >
> >
> >  /* Returns the highest-priority, location to look for git programs. */
> > -const char *git_exec_path(void)
> > +char *git_exec_path(void)
> >  {
> >       const char *env;
> >
> >       if (argv_exec_path)
> > -             return argv_exec_path;
> > +             return strdup(argv_exec_path);
> >
> >       env = getenv(EXEC_PATH_ENVIRONMENT);
> >       if (env && *env) {
> > -             return env;
> > +             return strdup(env);
> >       }
>
> Now you are making callers of git_exec_path() responsible for
> freeing the result they receive.
>
> git_exec_path() may be called quite a lot, which means we may end up
> calling system_path() many times during the life of a process
> without freeing its return value, so this change may be worth doing,
> but this patch is insufficient, isn't it?
>
> You just added load_command_list() in help.c a new leak or two, for
> example.  There probably are other callers of this function but I
> don't have time to look at all of them myself right now.

Yes, need to do that all git_exec_path() callers free result of git_exec_path.

>
> > @@ -95,8 +94,10 @@ void setup_path(void)
> >  {
> >       const char *old_path = getenv("PATH");
> >       struct strbuf new_path = STRBUF_INIT;
> > +     char* exec_path = git_exec_path();
> >
> > -     add_path(&new_path, git_exec_path());
> > +     add_path(&new_path, exec_path);
> > +     free(exec_path);
> >       add_path(&new_path, argv0_path);
>
> This part by itself is good, provided if we make it the caller's
> responsiblity to free string returned by git_exec_path().
>
> > diff --git a/git.c b/git.c
> > index 82d7a1c..d01c4f1 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> >                       if (*cmd == '=')
> >                               git_set_argv_exec_path(cmd + 1);
> >                       else {
> > -                             puts(git_exec_path());
> > +                             char *exec_path = git_exec_path();
> > +                             puts(exec_path);
> > +                             free(exec_path);
> >                               exit(0);
> >                       }
> >               } else if (!strcmp(cmd, "--html-path")) {
> > -                     puts(system_path(GIT_HTML_PATH));
> > +                     char *git_html_path = system_path(GIT_HTML_PATH);
> > +                     puts(git_html_path);
> > +                     free(git_html_path);
> >                       exit(0);
> >               } else if (!strcmp(cmd, "--man-path")) {
> > -                     puts(system_path(GIT_MAN_PATH));
> > +                     char *git_man_path = system_path(GIT_MAN_PATH);
> > +                     puts(git_man_path);
> > +                     free(git_man_path);
> >                       exit(0);
> >               } else if (!strcmp(cmd, "--info-path")) {
> > -                     puts(system_path(GIT_INFO_PATH));
> > +                     char *git_info_path = system_path(GIT_INFO_PATH);
> > +                     puts(git_info_path);
> > +                     free(git_info_path);
> >                       exit(0);
> >               } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
> >                       use_pager = 1;
>
> None of these warrant the code churn, I would say.

Sorry, english is not my first language, what did you mean when saying:
"code churn"? Code duplication or something else?

Re: [PATCH 1/1] change contract between system_path and it's callers

[Alexander Kuleshov]

Maybe we will discard this patch, because i looked on it and tested
with different places, it brings more leaks than before?

2014-11-26 9:53 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
>>
>> Comparing this with what I sent out...
>>
>> >  builtin/help.c | 10 +++++++---
>> >  exec_cmd.c     | 17 +++++++++--------
>> >  exec_cmd.h     |  4 ++--
>> >  git.c          | 16 ++++++++++++----
>> >  4 files changed, 30 insertions(+), 17 deletions(-)
>> >
>> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)
>> >  static void show_info_page(const char *git_cmd)
>> >  {
>> >       const char *page = cmd_to_page(git_cmd);
>> > -     setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
>> > +     char *git_info_path = system_path(GIT_INFO_PATH);
>> > +     setenv("INFOPATH", git_info_path, 1);
>> > +     free(git_info_path);
>>
>> We are just about to exec; does this warrant the code churn?
>
> hmm... Can't understand what's the problem here? We get git_info_path
> from system_path which returns pointer which will need to free, set it in
> environment var and than free it...
>
>>
>> >       execlp("info", "info", "gitman", page, (char *)NULL);
>> >       die(_("no info viewer handled the request"));
>>
>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>> >  #endif
>> >
>> >       strbuf_addf(&d, "%s/%s", prefix, path);
>> > -     path = strbuf_detach(&d, NULL);
>> > -     return path;
>> > +     return d.buf;
>>
>> These happens to be the same with the current strbuf implementation,
>> but it is a good manner to use strbuf_detach(&d, NULL) here.  We
>> don't know what other de-initialization tomorrow's implementation of
>> the strbuf API may have to do in strbuf_detach().
>
> How to do it in correct way?
>
>
>     strbuf_addf(&d, "%s/%s", prefix, path);
>     path = strbuf_detach(&d, NULL);
>     return (char*)path;
>
> Or something else?
>
>>
>> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
>> >
>> >
>> >  /* Returns the highest-priority, location to look for git programs. */
>> > -const char *git_exec_path(void)
>> > +char *git_exec_path(void)
>> >  {
>> >       const char *env;
>> >
>> >       if (argv_exec_path)
>> > -             return argv_exec_path;
>> > +             return strdup(argv_exec_path);
>> >
>> >       env = getenv(EXEC_PATH_ENVIRONMENT);
>> >       if (env && *env) {
>> > -             return env;
>> > +             return strdup(env);
>> >       }
>>
>> Now you are making callers of git_exec_path() responsible for
>> freeing the result they receive.
>>
>> git_exec_path() may be called quite a lot, which means we may end up
>> calling system_path() many times during the life of a process
>> without freeing its return value, so this change may be worth doing,
>> but this patch is insufficient, isn't it?
>>
>> You just added load_command_list() in help.c a new leak or two, for
>> example.  There probably are other callers of this function but I
>> don't have time to look at all of them myself right now.
>
> Yes, need to do that all git_exec_path() callers free result of git_exec_path.
>
>>
>> > @@ -95,8 +94,10 @@ void setup_path(void)
>> >  {
>> >       const char *old_path = getenv("PATH");
>> >       struct strbuf new_path = STRBUF_INIT;
>> > +     char* exec_path = git_exec_path();
>> >
>> > -     add_path(&new_path, git_exec_path());
>> > +     add_path(&new_path, exec_path);
>> > +     free(exec_path);
>> >       add_path(&new_path, argv0_path);
>>
>> This part by itself is good, provided if we make it the caller's
>> responsiblity to free string returned by git_exec_path().
>>
>> > diff --git a/git.c b/git.c
>> > index 82d7a1c..d01c4f1 100644
>> > --- a/git.c
>> > +++ b/git.c
>> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>> >                       if (*cmd == '=')
>> >                               git_set_argv_exec_path(cmd + 1);
>> >                       else {
>> > -                             puts(git_exec_path());
>> > +                             char *exec_path = git_exec_path();
>> > +                             puts(exec_path);
>> > +                             free(exec_path);
>> >                               exit(0);
>> >                       }
>> >               } else if (!strcmp(cmd, "--html-path")) {
>> > -                     puts(system_path(GIT_HTML_PATH));
>> > +                     char *git_html_path = system_path(GIT_HTML_PATH);
>> > +                     puts(git_html_path);
>> > +                     free(git_html_path);
>> >                       exit(0);
>> >               } else if (!strcmp(cmd, "--man-path")) {
>> > -                     puts(system_path(GIT_MAN_PATH));
>> > +                     char *git_man_path = system_path(GIT_MAN_PATH);
>> > +                     puts(git_man_path);
>> > +                     free(git_man_path);
>> >                       exit(0);
>> >               } else if (!strcmp(cmd, "--info-path")) {
>> > -                     puts(system_path(GIT_INFO_PATH));
>> > +                     char *git_info_path = system_path(GIT_INFO_PATH);
>> > +                     puts(git_info_path);
>> > +                     free(git_info_path);
>> >                       exit(0);
>> >               } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
>> >                       use_pager = 1;
>>
>> None of these warrant the code churn, I would say.
>
> Sorry, english is not my first language, what did you mean when saying:
> "code churn"? Code duplication or something else?



-- 
_________________________
0xAX

Re: [PATCH 1/1] change contract between system_path and it's callers

[Alexander Kuleshov]

What do you think if we create int variable, something like
given_config_must_free = 0; and will set up it to 1 in cases where it
will be allocated, and than we can free it in the end of cmd_config?
I'm reading git source code to understanding git internals and found
little memory leak in cat-file, so i fixed it as i described above.

2014-11-26 15:42 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
> Maybe we will discard this patch, because i looked on it and tested
> with different places, it brings more leaks than before?
>
> 2014-11-26 9:53 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>:
>>>
>>> Comparing this with what I sent out...
>>>
>>> >  builtin/help.c | 10 +++++++---
>>> >  exec_cmd.c     | 17 +++++++++--------
>>> >  exec_cmd.h     |  4 ++--
>>> >  git.c          | 16 ++++++++++++----
>>> >  4 files changed, 30 insertions(+), 17 deletions(-)
>>> >
>>> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)
>>> >  static void show_info_page(const char *git_cmd)
>>> >  {
>>> >       const char *page = cmd_to_page(git_cmd);
>>> > -     setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
>>> > +     char *git_info_path = system_path(GIT_INFO_PATH);
>>> > +     setenv("INFOPATH", git_info_path, 1);
>>> > +     free(git_info_path);
>>>
>>> We are just about to exec; does this warrant the code churn?
>>
>> hmm... Can't understand what's the problem here? We get git_info_path
>> from system_path which returns pointer which will need to free, set it in
>> environment var and than free it...
>>
>>>
>>> >       execlp("info", "info", "gitman", page, (char *)NULL);
>>> >       die(_("no info viewer handled the request"));
>>>
>>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>>> >  #endif
>>> >
>>> >       strbuf_addf(&d, "%s/%s", prefix, path);
>>> > -     path = strbuf_detach(&d, NULL);
>>> > -     return path;
>>> > +     return d.buf;
>>>
>>> These happens to be the same with the current strbuf implementation,
>>> but it is a good manner to use strbuf_detach(&d, NULL) here.  We
>>> don't know what other de-initialization tomorrow's implementation of
>>> the strbuf API may have to do in strbuf_detach().
>>
>> How to do it in correct way?
>>
>>
>>     strbuf_addf(&d, "%s/%s", prefix, path);
>>     path = strbuf_detach(&d, NULL);
>>     return (char*)path;
>>
>> Or something else?
>>
>>>
>>> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)
>>> >
>>> >
>>> >  /* Returns the highest-priority, location to look for git programs. */
>>> > -const char *git_exec_path(void)
>>> > +char *git_exec_path(void)
>>> >  {
>>> >       const char *env;
>>> >
>>> >       if (argv_exec_path)
>>> > -             return argv_exec_path;
>>> > +             return strdup(argv_exec_path);
>>> >
>>> >       env = getenv(EXEC_PATH_ENVIRONMENT);
>>> >       if (env && *env) {
>>> > -             return env;
>>> > +             return strdup(env);
>>> >       }
>>>
>>> Now you are making callers of git_exec_path() responsible for
>>> freeing the result they receive.
>>>
>>> git_exec_path() may be called quite a lot, which means we may end up
>>> calling system_path() many times during the life of a process
>>> without freeing its return value, so this change may be worth doing,
>>> but this patch is insufficient, isn't it?
>>>
>>> You just added load_command_list() in help.c a new leak or two, for
>>> example.  There probably are other callers of this function but I
>>> don't have time to look at all of them myself right now.
>>
>> Yes, need to do that all git_exec_path() callers free result of git_exec_path.
>>
>>>
>>> > @@ -95,8 +94,10 @@ void setup_path(void)
>>> >  {
>>> >       const char *old_path = getenv("PATH");
>>> >       struct strbuf new_path = STRBUF_INIT;
>>> > +     char* exec_path = git_exec_path();
>>> >
>>> > -     add_path(&new_path, git_exec_path());
>>> > +     add_path(&new_path, exec_path);
>>> > +     free(exec_path);
>>> >       add_path(&new_path, argv0_path);
>>>
>>> This part by itself is good, provided if we make it the caller's
>>> responsiblity to free string returned by git_exec_path().
>>>
>>> > diff --git a/git.c b/git.c
>>> > index 82d7a1c..d01c4f1 100644
>>> > --- a/git.c
>>> > +++ b/git.c
>>> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>>> >                       if (*cmd == '=')
>>> >                               git_set_argv_exec_path(cmd + 1);
>>> >                       else {
>>> > -                             puts(git_exec_path());
>>> > +                             char *exec_path = git_exec_path();
>>> > +                             puts(exec_path);
>>> > +                             free(exec_path);
>>> >                               exit(0);
>>> >                       }
>>> >               } else if (!strcmp(cmd, "--html-path")) {
>>> > -                     puts(system_path(GIT_HTML_PATH));
>>> > +                     char *git_html_path = system_path(GIT_HTML_PATH);
>>> > +                     puts(git_html_path);
>>> > +                     free(git_html_path);
>>> >                       exit(0);
>>> >               } else if (!strcmp(cmd, "--man-path")) {
>>> > -                     puts(system_path(GIT_MAN_PATH));
>>> > +                     char *git_man_path = system_path(GIT_MAN_PATH);
>>> > +                     puts(git_man_path);
>>> > +                     free(git_man_path);
>>> >                       exit(0);
>>> >               } else if (!strcmp(cmd, "--info-path")) {
>>> > -                     puts(system_path(GIT_INFO_PATH));
>>> > +                     char *git_info_path = system_path(GIT_INFO_PATH);
>>> > +                     puts(git_info_path);
>>> > +                     free(git_info_path);
>>> >                       exit(0);
>>> >               } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
>>> >                       use_pager = 1;
>>>
>>> None of these warrant the code churn, I would say.
>>
>> Sorry, english is not my first language, what did you mean when saying:
>> "code churn"? Code duplication or something else?
>
>
>
> --
> _________________________
> 0xAX



-- 
_________________________
0xAX

Re: [PATCH 1/1] change contract between system_path and it's callers

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

>> > +     setenv("INFOPATH", git_info_path, 1);
>> > +     free(git_info_path);
>> >       execlp("info", "info", "gitman", page, (char *)NULL);
>> >       die(_("no info viewer handled the request"));
>>
>> We are just about to exec; does this warrant the code churn?
>
> hmm... Can't understand what's the problem here?

Adding a new variable to keep the allocated piece of memory so that
you can free after using it, and freeing it.  These are not "problem"
per-se.

But immediately after doing so, execlp() wipes your process and
starts a different program afresh.  When that happens, all the heap
memory you allocated and were using is automatically reclaimed by
the system anyway.

So it may not be "wrong" to free, but doing anything extra is an
unnecessary work.

If you did an "assign to a variable to keep allocated memory, use
it, free it and then exec" sequence in a new code, I do not think it
is worth fixing the code not to do so to reduce that unnecessary
work---it is not worth another round of review exchange.  But in the
same way, make an existing code that is not doing an unnecessary
work to do so is not worth it.

>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>> >  #endif
>> >
>> >       strbuf_addf(&d, "%s/%s", prefix, path);
>> > -     path = strbuf_detach(&d, NULL);
>> > -     return path;
>> > +     return d.buf;
>>
>> These happens to be the same with the current strbuf implementation,
>> but it is a good manner to use strbuf_detach(&d, NULL) here.  We
>> don't know what other de-initialization tomorrow's implementation of
>> the strbuf API may have to do in strbuf_detach().
>
> How to do it in correct way?

Wouldn't "return strbuf_detach(&d, NULL);" work without any
assignment to "path"?

>> > ...
>> > -                     puts(system_path(GIT_INFO_PATH));
>> > +                     char *git_info_path = system_path(GIT_INFO_PATH);
>> > +                     puts(git_info_path);
>> > +                     free(git_info_path);
>> >                       exit(0);
>> >               } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
>> >                       use_pager = 1;
>>
>> None of these warrant the code churn, I would say.
>
> Sorry, english is not my first language, what did you mean when saying:
> "code churn"? Code duplication or something else?

Step back a bit and _think_ why you want to avoid leaks.  When a
program is leaky, your process will bloat by accumulating unused
cruft in your heap.  Step back again and think the reason why you
want to avoid that bloat.

It is because the parts of your program that want to allocate and
use more memory will suffer _after_ leak happens, because leakers
consume and keep memory that would be otherwise useful if they did
not leak and freed instead.  The later callers want to get more
memory, but cannot get it because the program leaked memory before
the control flow got to them.

Now, who do these calls to free() you added help?  Whose desire to
allocate more memory can be harmed by the allocated but not freed
piece of memory held by the return values from system_path()?

Nobody--exactly because the next thing you do is to exit.

It is not wrong per-se to free immediately before exiting; it is
just like freeing immediately before execing.  It is an unnecessary
thing to do, and a change to do so has no or very little value, even
if it is not a wrong thing to do.

Re: [PATCH 1/1] change contract between system_path and it's callers

[Philip Oakley]

From: "Alexander Kuleshov" <kuleshovmail@gmail.com>
Sent: Wednesday, November 26, 2014 3:53 AM
>>
>> None of these warrant the code churn, I would say.
>
> Sorry, english is not my first language, what did you mean when
> saying:
> "code churn"? Code duplication or something else?
> --
Hi Alexander,

The term 'churn' is originally from British butter making.

Churn:
verb
  1.shake (milk or cream) in a machine in order to produce butter.
  "the cream is ripened before it is churned"
        synonyms: stir, agitate;

   2.(with reference to liquid) move or cause to move about vigorously.
  "the seas churned".


For Code (used in a somewhat negative sense), it means that lots of bits 
are moved around a great deal for
little apparent benefit.

In the sense Junio used it, I believe it's suggesting that the balance 
between the amount of change and usefulness of the change had gone 
further than hoped. (Though Junio is usually open to receiving a well 
argued case)

Philip
see also http://en.wikipedia.org/wiki/Churn_rate