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

Alex Kuleshov [off-list ref] writes:

Junio C Hamano [off-list ref] @ 2014-11-25 01:33 ALMT:
...

quoted

quoted

 	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;