Re: [PATCH] exec_cmd: system_path memory leak fix

> 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.