Junio C Hamano <gitster@xxxxxxxxx> @ 2014-11-24 00:51 ALMT:
> 0xAX <kuleshovmail@xxxxxxxxx> writes:
>
>> Signed-off-by: 0xAX <kuleshovmail@xxxxxxxxx>
>> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html