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

[Junio C Hamano <gitster@pobox.com>]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> But if we still static const char* for git_etc_gitattributes and will
> not free it in bootstrap_attr_stack there will no memory leak, just
> tested it, so i'll remove free for it.

Leak or no leak, freeing there is wrong.  It will free the piece of
memory the next caller will obtain from the git_etc_gitattributes()
function.  In other words, the return value from that function is
owned by git_etc_gitattributes(), not by the caller.

As to "leak", in the strictest sense of the word, i.e. "do we
allocate on the heap and exit without freeing?", it _is_ leaking,
and your above "just tested it" probably means "the tool I used did
not find/report it".  But as I said, it is not something worth
bothering about [*1*].

Think of it this way.

The git_etc_gitattributes() function goes like this with your patch
(and there is no fundamental difference in the original version,
which uses "const char *" where appropriate):

         static char *git_etc_gitattributes(void)
         {
                static char *system_wide;
                if (!system_wide)
                        system_wide = system_path(ETC_GITATTRIBUTES);
                return system_wide;
         }

If you knew that the pathname for /etc/gitattributes substitute will
never be longer than 256 bytes, you may have coded the above like so
instead:

        static char system_wide[256];
        static char *git_etc_gitattributes(void)
        {
                if (!system_wide[0]) {
                        char *ret = system_path(ETC_GITATTRIBUTES);
                        if (strncpy(system_wide, ret, 256) >= 256)
                                die("ETC_GITATTRIBUTES too long ");
                }       
                return system_wide;
        }

Even though we used the memory occupied by system_wide[] and did not
clean up before exit(), nobody would complain about leaking.

The existing code is the moral equivalent of the "up to 256 bytes"
illustration, but for a string whose size is not known at compile
time.  It is using and keeping the memory until program exit.
Nobody should complain about leaking.

That is, unless (1) the held memory is expected to be very large and
(2) you can prove that after the point you decide to insert free(),
nobody will ever need that information again.


[Footnote]

*1* The leaks we care about are of this form:

    void silly_leaker(void)
    {
        printf("%s\n", system_path(ETC_GITATTRIBUTES));
    }

    where each invocation allocates memory, uses it and then loses
    the reference to it without doing anything to clean-up.  You can
    call such a function unbounded number of times and waste
    unbounded amount of memory.

    The implementation of git_etc_gitattributes() is not of that
    kind.  An invocation of it allocates, uses and keeps.  The
    second and subsequent invocation does not allocate.  When you
    call it unbounded number of times, it does not waste unbounded
    amount of memory.  It just keeps what it needs to answer the
    next caller with.

    The pattern needs to be made thread-safe, but that is a separate
    topic.