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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 24, 2014, 20:20 UTC
Message-ID: <xmqqk32kwdso.fsf@gitster.dls.corp.google.com>
In-Reply-To: <87ppcc4b2g.fsf@gmail.com>

Alex Kuleshov <kuleshovmail@gmail.com> writes:

Show 17 quoted lines

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