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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Alex Kuleshov <kuleshovmail@xxxxxxxxx> writes:

> Junio C Hamano <gitster@xxxxxxxxx> @ 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;

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]