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

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