Re: Re*: [PATCH] change contract between system_path and it's callers
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.
--
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
Re: Re*: [PATCH] change contract between system_path and it's callers
Hello Junio,
I'm working on this patch and builtin/config.c, i have a little question:
Look, here - https://github.com/git/git/blob/master/builtin/config.c#L509
and in the next if clauses we get allocated path, but if i test it
with given config file with git config -f
(https://github.com/git/git/blob/master/builtin/config.c#L513) i have
prefix NULL and next if clause doesn't execute, so if i'll put
free(given_config_source.file) after config action i'll get error
because given_config_source.file wasn't allocated. How to be with
this?
Thank you.
2014-11-25 13:04 GMT+06:00 Alexander Kuleshov kuleshovm...@gmail.com:
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.
2014-11-25 12:45 GMT+06:00 Alexander Kuleshov kuleshovm...@gmail.com:
Hello Junio,
I returned static to git_etc_gitattributes return value, but now i
can't understand where to free 'system_wide'. As i understand
correctly attr.c has following API for querying attributes:
git_attr
git_check_attr
And as i see in code git_attr doesn't use git_attr_check, so now we
have only git_check_attr and git_all_stars functions which are through
prepare_attr_stack and bootstrap_attr_stack in it uses path which
returned
by system_path. So you're right if i free etc_attributes like this
and git_etc_gitattributes will return static value we'll have access
to freed memory if it will called again. But we have no access to
etc_attributes outside bootstrap_attr_stack so where will be best
place to free it?
Thank you
2014-11-25 2:50 GMT+06:00 Junio C Hamano gits...@pobox.com:
Alex Kuleshov kuleshovm...@gmail.com writes:
One thing to note is that this illustration does not consider memory
pointed at by the system_wide variable here (from attr.c)
static const char *git_etc_gitattributes(void)
{
static const char *system_wide;
if (!system_wide)
system_wide = system_path(ETC_GITATTRIBUTES);
return system_wide;
}
at the point of process exit as a leak.
But why? We allocated memory to system_wide with system_path, next git
will exit somewhere with die, but system_wide didn't free... Or i'm
wrong here too?
It is in the same league as static const char *git_dir and friends
that appear in the file-scope-static of environment.c. Keeping small
things around to be cleaned up by exit() is not a crime.
--
_
0xAX
--
_
0xAX
--
_
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
Re: Re*: [PATCH] change contract between system_path and it's callers
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
Re: Re*: [PATCH] change contract between system_path and it's callers
Alexander Kuleshov kuleshovm...@gmail.com writes: 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 Yes, but looking at the file, you would notice that the use_local_config codepath assigns a string obtained from git_pathdup() to given_config_source.file, which means that somebody later in the code path would not know if given_config_source.file is merely pointing at a piece of memory owned by somebody else (e.g. came from git_etc_gitconfig()) or if it owns the piece of memory (e.g. came from git_pathdup()). In the former case the memory should never be freed, but not freeing in the latter would leak the memory. Assignment to given_config_source.file I see in that function that borrows (i.e. the current code that does not fee is correct) are: - getenv(CONFIG_ENVIRONMENT) - NULL ;-) obviously - git_etc_gitconfig() All the other assignment to given_config_source.file (including the ones obtained from a call to home_config_paths()) makes us responsible to free that piece memory, so they are technically leaks. But cmd_config() is a moral equivalent of main() in a typical program, so it might not be worth worrying about allocating a single piece of memory that is used throughout the lifetime of that function and leaving it without freeing. -- 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
Re: Re*: [PATCH] change contract between system_path and it's callers
Junio C Hamano gits...@pobox.com @ 2014-11-25 01:33 ALMT:
-static const char *git_etc_gitattributes(void)
+static char *git_etc_gitattributes(void)
Hmph, I think this should keep returning const char *, as the
caller is not expected to free the pointer or write into the memory
held by the returned string. The same applies to the change of the
type in struct git_config_source.
The change in the semantics of system_path() made the get once and
keep returning it this function does safer and correct, but this
function itself did not change any behaviour from its caller's point
of view.
Understand, will fix it.
{
-static const char *system_wide;
+static char *system_wide;
if (!system_wide)
system_wide = system_path(ETC_GITATTRIBUTES);
return system_wide;
@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)
attr_stack = elem;
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?
diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..266d42b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
if (given_config_source.blob)
die(editing blobs is not supported);
git_config(git_default_config, NULL);
-config_file = xstrdup(given_config_source.file ?
- given_config_source.file :
git_path(config));
+
+if (use_system_config)
+config_file = given_config_source.file ?
+
given_config_source.file : git_path(config);
+else
+config_file = xstrdup(given_config_source.file ?
+
given_config_source.file : git_path(config));
+
Sorry, but I do not understand this change.
Why do you need if use-system-config, do not allocate here and
then later everywhere if use-system-config, free it? I would
understand if the change were earlier we had mixture of borrowed
and owned strings, but we make sure we always own the string by
running xstrdup() in the caller when we used to let this function
borrow, so that we can always free() before returning from here,
or something.
For example, in the original code, use_local_config codepath uses
git_pathdup(), which is an allocated piece of memory, to initialize
given_config_source.file. Doesn't it need to be freed?
Even if it is not in the use_system_config mode, if
given_config_source.file is not an absolute path, we store a copy of
prefix-filename result to given_config_source.file. Doesn't it need
to be freed?
I think the implementation strategy you took makes the result
unnecessarily messy and error prone. Let's step back a bit.
When you have code that sometimes borrows memory and sometimes owns
memory, writing the clean-up part like this is error prone:
char *var;
if (A)
var = borrowed memory;
else if (B)
var = borrowed memory;
else if (C)
var = xstrdup(borrowed memory);
else
var = borrowed memory;
use var; /* a loong piece of code in reality */
if (C)
free(var);
because the set-up part can and will change over time as the code
evolves. If written this way:
char *var, *to_free = NULL;
if (A)
var = borrowed memory;
else if (B)
var = borrowed memory;
else if (C)
to_free = var = xstrdup(borrowed memory);
else
var = borrowed memory;
use var; /* a loong piece of code in reality */
free(to_free);
the clean-up part would not have to worry how the set-up part
decided when to borrow memory and when to own memory. Another way
to do so would obviously be:
char *var;
if (A)
var = xstrdup(borrowed memory);
else if (B)
var = xstrdup(borrowed memory);
else if (C)
var = xstrdup(borrowed memory);
else
var = xstrdup(borrowed memory);
use var; /* a loong piece of code in reality */
Re: Re*: [PATCH] change contract between system_path and it's callers
Alex Kuleshov kuleshovm...@gmail.com writes:
Junio C Hamano gits...@pobox.com @ 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re*: [PATCH] change contract between system_path and it's callers
Alex Kuleshov kuleshovm...@gmail.com writes:
One thing to note is that this illustration does not consider memory
pointed at by the system_wide variable here (from attr.c)
static const char *git_etc_gitattributes(void)
{
static const char *system_wide;
if (!system_wide)
system_wide = system_path(ETC_GITATTRIBUTES);
return system_wide;
}
at the point of process exit as a leak.
But why? We allocated memory to system_wide with system_path, next git
will exit somewhere with die, but system_wide didn't free... Or i'm
wrong here too?
It is in the same league as static const char *git_dir and friends
that appear in the file-scope-static of environment.c. Keeping small
things around to be cleaned up by exit() is not a crime.
--
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
Re: Re*: [PATCH] change contract between system_path and it's callers
Hello Junio,
I returned static to git_etc_gitattributes return value, but now i
can't understand where to free 'system_wide'. As i understand
correctly attr.c has following API for querying attributes:
git_attr
git_check_attr
And as i see in code git_attr doesn't use git_attr_check, so now we
have only git_check_attr and git_all_stars functions which are through
prepare_attr_stack and bootstrap_attr_stack in it uses path which
returned
by system_path. So you're right if i free etc_attributes like this
and git_etc_gitattributes will return static value we'll have access
to freed memory if it will called again. But we have no access to
etc_attributes outside bootstrap_attr_stack so where will be best
place to free it?
Thank you
2014-11-25 2:50 GMT+06:00 Junio C Hamano gits...@pobox.com:
Alex Kuleshov kuleshovm...@gmail.com writes:
One thing to note is that this illustration does not consider memory
pointed at by the system_wide variable here (from attr.c)
static const char *git_etc_gitattributes(void)
{
static const char *system_wide;
if (!system_wide)
system_wide = system_path(ETC_GITATTRIBUTES);
return system_wide;
}
at the point of process exit as a leak.
But why? We allocated memory to system_wide with system_path, next git
will exit somewhere with die, but system_wide didn't free... Or i'm
wrong here too?
It is in the same league as static const char *git_dir and friends
that appear in the file-scope-static of environment.c. Keeping small
things around to be cleaned up by exit() is not a crime.
--
_
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
Re: Re*: [PATCH] change contract between system_path and it's callers
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.
2014-11-25 12:45 GMT+06:00 Alexander Kuleshov kuleshovm...@gmail.com:
Hello Junio,
I returned static to git_etc_gitattributes return value, but now i
can't understand where to free 'system_wide'. As i understand
correctly attr.c has following API for querying attributes:
git_attr
git_check_attr
And as i see in code git_attr doesn't use git_attr_check, so now we
have only git_check_attr and git_all_stars functions which are through
prepare_attr_stack and bootstrap_attr_stack in it uses path which
returned
by system_path. So you're right if i free etc_attributes like this
and git_etc_gitattributes will return static value we'll have access
to freed memory if it will called again. But we have no access to
etc_attributes outside bootstrap_attr_stack so where will be best
place to free it?
Thank you
2014-11-25 2:50 GMT+06:00 Junio C Hamano gits...@pobox.com:
Alex Kuleshov kuleshovm...@gmail.com writes:
One thing to note is that this illustration does not consider memory
pointed at by the system_wide variable here (from attr.c)
static const char *git_etc_gitattributes(void)
{
static const char *system_wide;
if (!system_wide)
system_wide = system_path(ETC_GITATTRIBUTES);
return system_wide;
}
at the point of process exit as a leak.
But why? We allocated memory to system_wide with system_path, next git
will exit somewhere with die, but system_wide didn't free... Or i'm
wrong here too?
It is in the same league as static const char *git_dir and friends
that appear in the file-scope-static of environment.c. Keeping small
things around to be cleaned up by exit() is not a crime.
--
_
0xAX
--
_
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
