Ah, you little ahead me, so we do not care about config.c in this way,
because it takes git_etc_gitconfig() in the same way as
git_etc_gitattributes
2014-11-25 23:55 GMT+06:00 Junio C Hamano <gits...@pobox.com>:
> Alexander Kuleshov <kuleshovm...@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.
--
_________________________
0xAX
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
