* [PATCH 1/7] init: plug tiny one-time memory leak
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
@ 2010-10-02 8:31 ` Jonathan Nieder
2010-10-03 23:46 ` Junio C Hamano
2010-10-02 8:32 ` [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise Jonathan Nieder
` (5 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ivan Kanis, git, Pierre Habouzit, David Barr
The buffer used to construct paths like ".git/objects/info" and
".git/objects/pack" is allocated on the heap and never freed.
So free it. While at it, factor out the relevant code into its own
function to make it more obvious that this data is not used elsewhere.
Noticed by valgrind while setting up tests (in test-lib).
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/init-db.c | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0271285..a9c54ea 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -294,11 +294,26 @@ static int create_default_files(const char *template_path)
return reinit;
}
+static void create_sha1_dir(void)
+{
+ const char *sha1_dir = get_object_directory();
+ int len = strlen(sha1_dir);
+ char *path = xmalloc(len + 40);
+
+ memcpy(path, sha1_dir, len);
+
+ safe_create_dir(sha1_dir, 1);
+ strcpy(path+len, "/pack");
+ safe_create_dir(path, 1);
+ strcpy(path+len, "/info");
+ safe_create_dir(path, 1);
+
+ free(path);
+}
+
int init_db(const char *template_dir, unsigned int flags)
{
- const char *sha1_dir;
- char *path;
- int len, reinit;
+ int reinit;
safe_create_dir(get_git_dir(), 0);
@@ -313,16 +328,7 @@ int init_db(const char *template_dir, unsigned int flags)
reinit = create_default_files(template_dir);
- sha1_dir = get_object_directory();
- len = strlen(sha1_dir);
- path = xmalloc(len + 40);
- memcpy(path, sha1_dir, len);
-
- safe_create_dir(sha1_dir, 1);
- strcpy(path+len, "/pack");
- safe_create_dir(path, 1);
- strcpy(path+len, "/info");
- safe_create_dir(path, 1);
+ create_sha1_dir();
if (shared_repository) {
char buf[10];
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] init: plug tiny one-time memory leak
2010-10-02 8:31 ` [PATCH 1/7] init: plug tiny one-time memory leak Jonathan Nieder
@ 2010-10-03 23:46 ` Junio C Hamano
2010-10-04 4:34 ` [PATCH v2] " Jonathan Nieder
0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2010-10-03 23:46 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ivan Kanis, git, Pierre Habouzit, David Barr
Jonathan Nieder <jrnieder@gmail.com> writes:
> The buffer used to construct paths like ".git/objects/info" and
> ".git/objects/pack" is allocated on the heap and never freed.
While at it it might be a good idea to rename its outdated name to
something saner, e.g. object_dir, perhaps?
In v0.99 days, it make some sense to call that sha1_dir because we
prepopulated 256 subdirectories 00-ff in it, and as an afterthought we
reused the buffer to create an extra subdirectory "/pack" there. But we
do not create the fan-out in advance since 9106c09 (Create object
subdirectories on demand (phase II), 2005-10-09); sha1_dir is not a very
good name for the buffer to hold the path to ".git/objects" anymore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2] init: plug tiny one-time memory leak
2010-10-03 23:46 ` Junio C Hamano
@ 2010-10-04 4:34 ` Jonathan Nieder
0 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-04 4:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ivan Kanis, git, Pierre Habouzit, David Barr
The buffer used to construct paths like ".git/objects/info" and
".git/objects/pack" is allocated on the heap and never freed.
So free it. While at it, factor out the relevant code into its own
function and rename the sha1_dir variable to object_directory (to
match the change in everyday usage after the renaming of
SHA1_FILE_DIRECTORY in v0.99~603^2~7, 2005).
Noticed by valgrind while setting up tests (in test-lib).
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:
> While at it it might be a good idea to rename its outdated name to
> something saner, e.g. object_dir, perhaps?
Yes.
builtin/init-db.c | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0271285..9d4886c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -294,11 +294,26 @@ static int create_default_files(const char *template_path)
return reinit;
}
+static void create_object_directory(void)
+{
+ const char *object_directory = get_object_directory();
+ int len = strlen(object_directory);
+ char *path = xmalloc(len + 40);
+
+ memcpy(path, object_directory, len);
+
+ safe_create_dir(object_directory, 1);
+ strcpy(path+len, "/pack");
+ safe_create_dir(path, 1);
+ strcpy(path+len, "/info");
+ safe_create_dir(path, 1);
+
+ free(path);
+}
+
int init_db(const char *template_dir, unsigned int flags)
{
- const char *sha1_dir;
- char *path;
- int len, reinit;
+ int reinit;
safe_create_dir(get_git_dir(), 0);
@@ -313,16 +328,7 @@ int init_db(const char *template_dir, unsigned int flags)
reinit = create_default_files(template_dir);
- sha1_dir = get_object_directory();
- len = strlen(sha1_dir);
- path = xmalloc(len + 40);
- memcpy(path, sha1_dir, len);
-
- safe_create_dir(sha1_dir, 1);
- strcpy(path+len, "/pack");
- safe_create_dir(path, 1);
- strcpy(path+len, "/info");
- safe_create_dir(path, 1);
+ create_object_directory();
if (shared_repository) {
char buf[10];
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
2010-10-02 8:31 ` [PATCH 1/7] init: plug tiny one-time memory leak Jonathan Nieder
@ 2010-10-02 8:32 ` Jonathan Nieder
2010-10-02 16:30 ` Andreas Ericsson
2010-10-06 20:10 ` Junio C Hamano
2010-10-02 8:35 ` [PATCH 3/7] environment.c: remove unused variable Jonathan Nieder
` (4 subsequent siblings)
6 siblings, 2 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pierre Habouzit, David Barr
The current lockfile API often requires leaking memory:
- the list of lockfiles to remove at exit is maintained in a
singly linked list, so one cannot easily remove a lockfile
record when it is no longer needed
- some lockfile records are statically allocated and others are on
the heap, so the remove_lock_file() atexit handler cannot easily
free them at exit (not to mention that it would be a waste of time
since _exit() takes care of freeing them anyway)
As the git libification effort proceeds, maybe this will become worth
fixing. For now, since there are so few lock files used by a given
process, it is simpler to tolerate the leak.
Add a alloc_lock_file() helper to make such tolerance easier.
Allocations made through this helper function will not be reported as
leaks by valgrind even if there is no corresponding free(). The
((optimize("-fno-optimize-sibling-calls"))) attribute is needed on
platforms with GCC to ensure that the stack frame for
alloc_lock_file() is not replaced by the stack frame for xcalloc().
For illustration, this patch updates a few example xcalloc(1,
sizeof(struct lockfile)) call sites to use the new wrapper.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Documentation/technical/api-lockfile.txt | 9 +++++++++
builtin/update-index.c | 4 +---
cache-tree.c | 7 +------
cache.h | 1 +
config.c | 2 +-
lockfile.c | 6 ++++++
t/valgrind/default.supp | 8 ++++++++
7 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index dd89404..8eac22a 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -22,6 +22,15 @@ The lockfile API serves two purposes:
The functions
-------------
+alloc_lock_file::
+
+ Allocate and clear a `struct lock_file`. Because the
+ structure is used in an `atexit(3)` handler, its
+ storage has to stay throughout the life of the
+ program. Using this allocation function instead of
+ `malloc` allows profiling tools like valgrind to keep
+ track of such storage and avoid reporting it as a leak.
+
hold_lock_file_for_update::
Take a pointer to `struct lock_file`, the filename of
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..7697631 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -591,9 +591,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
- /* We can't free this memory, it becomes part of a linked list parsed atexit() */
- lock_file = xcalloc(1, sizeof(struct lock_file));
-
+ lock_file = alloc_lock_file();
newfd = hold_locked_index(lock_file, 0);
if (newfd < 0)
lock_error = errno;
diff --git a/cache-tree.c b/cache-tree.c
index f755590..81cf785 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -550,12 +550,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
int entries, was_valid, newfd;
struct lock_file *lock_file;
- /*
- * We can't free this memory, it becomes part of a linked list
- * parsed atexit()
- */
- lock_file = xcalloc(1, sizeof(struct lock_file));
-
+ lock_file = alloc_lock_file();
newfd = hold_locked_index(lock_file, 1);
entries = read_cache();
diff --git a/cache.h b/cache.h
index 2ef2fa3..9ee61a2 100644
--- a/cache.h
+++ b/cache.h
@@ -517,6 +517,7 @@ struct lock_file {
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern struct lock_file *alloc_lock_file(void);
extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
extern int commit_lock_file(struct lock_file *);
diff --git a/config.c b/config.c
index 4b0a820..3f30e2b 100644
--- a/config.c
+++ b/config.c
@@ -1158,7 +1158,7 @@ int git_config_set_multivar(const char *key, const char *value,
* The lock serves a purpose in addition to locking: the new
* contents of .git/config will be written into it.
*/
- lock = xcalloc(sizeof(struct lock_file), 1);
+ lock = alloc_lock_file();
fd = hold_lock_file_for_update(lock, config_filename, 0);
if (fd < 0) {
error("could not lock config file %s: %s", config_filename, strerror(errno));
diff --git a/lockfile.c b/lockfile.c
index b0d74cd..161dea8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -184,6 +184,12 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)
die("%s", unable_to_lock_message(path, err));
}
+__attribute__((optimize("-fno-optimize-sibling-calls")))
+struct lock_file *alloc_lock_file(void)
+{
+ return xcalloc(1, sizeof(struct lock_file));
+}
+
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
{
int fd = lock_file(lk, path, flags);
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 9e013fa..7789edd 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -43,3 +43,11 @@
fun:write_buffer
fun:write_loose_object
}
+
+{
+ see api-lockfile.txt
+ Memcheck:Leak
+ fun:calloc
+ fun:xcalloc
+ fun:alloc_lock_file
+}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-02 8:32 ` [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise Jonathan Nieder
@ 2010-10-02 16:30 ` Andreas Ericsson
2010-10-02 16:43 ` Jonathan Nieder
2010-10-06 20:10 ` Junio C Hamano
1 sibling, 1 reply; 33+ messages in thread
From: Andreas Ericsson @ 2010-10-02 16:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Pierre Habouzit, David Barr
On 10/02/2010 10:32 AM, Jonathan Nieder wrote:
>
> Add a alloc_lock_file() helper to make such tolerance easier.
> Allocations made through this helper function will not be reported as
> leaks by valgrind even if there is no corresponding free(). The
> ((optimize("-fno-optimize-sibling-calls"))) attribute is needed on
> platforms with GCC to ensure that the stack frame for
> alloc_lock_file() is not replaced by the stack frame for xcalloc().
>
So we're basically increasing runtime to shut up a leakchecking tool
and also making that leakchecking tool falsely not report positives.
Hmm...
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-02 16:30 ` Andreas Ericsson
@ 2010-10-02 16:43 ` Jonathan Nieder
0 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 16:43 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Junio C Hamano, git, Pierre Habouzit, David Barr
Andreas Ericsson wrote:
> So we're basically increasing runtime to shut up a leakchecking tool
> and also making that leakchecking tool falsely not report positives.
Yep, nice summary. Probably I should have also mentioned:
- An extra stack frame with no locals is not a lot of overhead, but
in any case these are by design not in performance-critical places.
- By using a special function like this, we make instances nicely
grep-able and give the leak prominence in t/valgrind/default.supp.
So a person can discover, for example, that writing a lot of trees
in a single process (like cherry-pick -n foo..bar currently does)
is going to be leaky.
- valgrind would be most useful if it can be used to identify
_regressions_ by running the test suite with --valgrind. git is
deliberately leaky in a lot of places; it is not useful to record
that.
- valgrind does have the ability to turn suppressions off if you want.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-02 8:32 ` [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise Jonathan Nieder
2010-10-02 16:30 ` Andreas Ericsson
@ 2010-10-06 20:10 ` Junio C Hamano
2010-10-06 20:21 ` Jonathan Nieder
1 sibling, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2010-10-06 20:10 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Pierre Habouzit, David Barr
Jonathan Nieder <jrnieder@gmail.com> writes:
> ...The
> ((optimize("-fno-optimize-sibling-calls"))) attribute is needed on
> platforms with GCC to ensure that the stack frame for
> alloc_lock_file() is not replaced by the stack frame for xcalloc().
Hmm, I am getting
cc1: warnings being treated as errors
lockfile.c:189: error: 'optimize' attribute directive ignored
make: *** [lockfile.o] Error 1
from this patch with gcc (Debian 4.3.2-1.1) 4.3.2
Aren't "struct lock_file" instances supposed to be reachable from the
linked list, i.e. lock_file_list? Why does valgrind consider that
elements on that list are leaked in the first place?
We keep loaded objects in *obj_hash[] and they sure are "leaked" by the
same definition of leakage, no? How are we squelching valgrind on them?
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-06 20:10 ` Junio C Hamano
@ 2010-10-06 20:21 ` Jonathan Nieder
2010-10-06 21:45 ` Junio C Hamano
0 siblings, 1 reply; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-06 20:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pierre Habouzit, David Barr
Junio C Hamano wrote:
> Hmm, I am getting
>
> cc1: warnings being treated as errors
> lockfile.c:189: error: 'optimize' attribute directive ignored
> make: *** [lockfile.o] Error 1
>
> from this patch with gcc (Debian 4.3.2-1.1) 4.3.2
Unfortunate. With gcc 4.5 it works, but that isn't too useful.
> Aren't "struct lock_file" instances supposed to be reachable from the
> linked list, i.e. lock_file_list? Why does valgrind consider that
> elements on that list are leaked in the first place?
At exit, we walk the lock file list and clear it in the process.
Which suggests a cleaner workaround (thanks!):
static void remove_lock_file(void)
{
pid_t me = getpid();
+ struct lock_file *p = lock_file_list;
- while (lock_file_list) {
- if (lock_file_list->owner == me &&
- lock_file_list->filename[0]) {
- if (lock_file_list->fd >= 0)
- close(lock_file_list->fd);
- unlink_or_warn(lock_file_list->filename);
- }
- lock_file_list = lock_file_list->next;
+ while (p) {
+ if (p->owner == me &&
+ p->filename[0]) {
+ if (p->fd >= 0)
+ close(p->fd);
+ unlink_or_warn(p->filename);
+ }
+ p = lock_file_list->next;
}
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-06 20:21 ` Jonathan Nieder
@ 2010-10-06 21:45 ` Junio C Hamano
2010-10-06 22:17 ` Jonathan Nieder
0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2010-10-06 21:45 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Pierre Habouzit, David Barr
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>
>> Hmm, I am getting
>>
>> cc1: warnings being treated as errors
>> lockfile.c:189: error: 'optimize' attribute directive ignored
>> make: *** [lockfile.o] Error 1
>>
>> from this patch with gcc (Debian 4.3.2-1.1) 4.3.2
>
> Unfortunate. With gcc 4.5 it works, but that isn't too useful.
>
>> Aren't "struct lock_file" instances supposed to be reachable from the
>> linked list, i.e. lock_file_list? Why does valgrind consider that
>> elements on that list are leaked in the first place?
>
> At exit, we walk the lock file list and clear it in the process.
> Which suggests a cleaner workaround (thanks!):
>
> static void remove_lock_file(void)
> {
> pid_t me = getpid();
> + struct lock_file *p = lock_file_list;
>
> - while (lock_file_list) {
> - if (lock_file_list->owner == me &&
> - lock_file_list->filename[0]) {
> - if (lock_file_list->fd >= 0)
> - close(lock_file_list->fd);
> - unlink_or_warn(lock_file_list->filename);
> - }
> - lock_file_list = lock_file_list->next;
> + while (p) {
> + if (p->owner == me &&
> + p->filename[0]) {
> + if (p->fd >= 0)
> + close(p->fd);
> + unlink_or_warn(p->filename);
> + }
> + p = lock_file_list->next;
> }
Heh, shouldn't the last one assign from p->next?
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise
2010-10-06 21:45 ` Junio C Hamano
@ 2010-10-06 22:17 ` Jonathan Nieder
0 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-06 22:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Pierre Habouzit, David Barr
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> - while (lock_file_list) {
>> - if (lock_file_list->owner == me &&
>> - lock_file_list->filename[0]) {
>> - if (lock_file_list->fd >= 0)
>> - close(lock_file_list->fd);
>> - unlink_or_warn(lock_file_list->filename);
>> - }
>> - lock_file_list = lock_file_list->next;
>> + while (p) {
>> + if (p->owner == me &&
>> + p->filename[0]) {
>> + if (p->fd >= 0)
>> + close(p->fd);
>> + unlink_or_warn(p->filename);
>> + }
>> + p = lock_file_list->next;
>> }
>
> Heh, shouldn't the last one assign from p->next?
Yes. And I also should have mentioned: this is untested.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/7] environment.c: remove unused variable
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
2010-10-02 8:31 ` [PATCH 1/7] init: plug tiny one-time memory leak Jonathan Nieder
2010-10-02 8:32 ` [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise Jonathan Nieder
@ 2010-10-02 8:35 ` Jonathan Nieder
2010-10-02 8:36 ` [PATCH 4/7] setup: make sure git dir path is in a permanent buffer Jonathan Nieder
` (3 subsequent siblings)
6 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Shawn Pearce
After v1.6.0-rc0~230^2^ (environment.c: remove unused function,
2008-06-19), git_refs_dir is not used any more.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
environment.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/environment.c b/environment.c
index 2d0c315..c44a30b 100644
--- a/environment.c
+++ b/environment.c
@@ -64,7 +64,7 @@ char *git_work_tree_cfg;
static char *work_tree;
static const char *git_dir;
-static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
+static char *git_object_dir, *git_index_file, *git_graft_file;
/*
* Repository-local GIT_* environment variables
@@ -96,8 +96,6 @@ static void setup_git_env(void)
git_object_dir = xmalloc(strlen(git_dir) + 9);
sprintf(git_object_dir, "%s/objects", git_dir);
}
- git_refs_dir = xmalloc(strlen(git_dir) + 6);
- sprintf(git_refs_dir, "%s/refs", git_dir);
git_index_file = getenv(INDEX_ENVIRONMENT);
if (!git_index_file) {
git_index_file = xmalloc(strlen(git_dir) + 7);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 4/7] setup: make sure git dir path is in a permanent buffer
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
` (2 preceding siblings ...)
2010-10-02 8:35 ` [PATCH 3/7] environment.c: remove unused variable Jonathan Nieder
@ 2010-10-02 8:36 ` Jonathan Nieder
2010-10-04 9:25 ` Nguyen Thai Ngoc Duy
2010-10-02 8:38 ` [PATCH 5/7] Introduce malloc/strdup/pathdup variants for permanent allocations Jonathan Nieder
` (2 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Lars Hjemli
If setup_git_env() is run before the usual repository discovery
sequence and .git is a file with the text
gitdir: <path>
(with <path> any string) then the in-core git_dir variable is set to
the result of converting <path> to an absolute path using
make_absolute_path().
Unfortunately make_absolute_path() returns its result in a static
buffer that is overwritten by later calls. Such a call could cause
later accesses to git_dir (from git_pathdup(), for example) to read
the wrong path, leaving git very confused.
It is not obvious whether any existing code in git will trigger the
problem, but in any case, it is worth a few dozen bytes to copy the
return value from make_absolute_path() for some added peace of mind.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
environment.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/environment.c b/environment.c
index c44a30b..de5581f 100644
--- a/environment.c
+++ b/environment.c
@@ -87,8 +87,10 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
static void setup_git_env(void)
{
git_dir = getenv(GIT_DIR_ENVIRONMENT);
- if (!git_dir)
+ if (!git_dir) {
git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+ git_dir = git_dir ? xstrdup(git_dir) : NULL;
+ }
if (!git_dir)
git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
git_object_dir = getenv(DB_ENVIRONMENT);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 4/7] setup: make sure git dir path is in a permanent buffer
2010-10-02 8:36 ` [PATCH 4/7] setup: make sure git dir path is in a permanent buffer Jonathan Nieder
@ 2010-10-04 9:25 ` Nguyen Thai Ngoc Duy
0 siblings, 0 replies; 33+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2010-10-04 9:25 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Lars Hjemli
On Sat, Oct 2, 2010 at 3:36 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> It is not obvious whether any existing code in git will trigger the
> problem, but in any case, it is worth a few dozen bytes to copy the
> return value from make_absolute_path() for some added peace of mind.
It looks like make_absolute_path() is only used when git_dir has not
been set in stone yet. Anyway, yes yes yes! I don't want to pull my
hair when I find out this bug hits me.
--
Duy
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 5/7] Introduce malloc/strdup/pathdup variants for permanent allocations
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
` (3 preceding siblings ...)
2010-10-02 8:36 ` [PATCH 4/7] setup: make sure git dir path is in a permanent buffer Jonathan Nieder
@ 2010-10-02 8:38 ` Jonathan Nieder
2010-10-02 8:39 ` [PATCH 6/7] environment: use alloc_permanent() for computed git_dir and co Jonathan Nieder
2010-10-02 8:41 ` [PATCH 7/7] commit-tree: free commit message before exiting Jonathan Nieder
6 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In git (option parsing and some other places), we sometimes leak tiny
bits of memory on purpose. Introduce a way to declare such
allocations, to make the lives of people searching for real memory
leaks easier.
Currently these are thin wrappers around xmalloc and xstrdup. The
KEEP_WRAPPER attribute avoids a tail call to ensure their stack frame
does not get replaced with the true xmalloc/xstrdup, so tools like
valgrind can easily distinguish them and know that the memory they
return is not meant to be freed.
In the long run it might be nice to tighten the semantics of these
functions and use memory from a large pool that is never freed (to
avoid fragmentation and save some allocation overhead).
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
cache.h | 2 ++
git-compat-util.h | 2 ++
path.c | 10 ++++++++++
t/valgrind/default.supp | 17 +++++++++++++++++
wrapper.c | 14 ++++++++++++++
5 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index 9ee61a2..bbf32ff 100644
--- a/cache.h
+++ b/cache.h
@@ -638,6 +638,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
+extern char *git_pathdup_permanent(const char *fmt, ...)
+ __attribute__((format (printf, 1, 2)));
/* Return a statically allocated filename matching the sha1 signature */
extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
diff --git a/git-compat-util.h b/git-compat-util.h
index 81883e7..f00ee33 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -380,7 +380,9 @@ typedef void (*try_to_free_t)(size_t);
extern try_to_free_t set_try_to_free_routine(try_to_free_t);
extern char *xstrdup(const char *str);
+extern char *strdup_permanent(const char *str);
extern void *xmalloc(size_t size);
+extern void *alloc_permanent(size_t size);
extern void *xmallocz(size_t size);
extern void *xmemdupz(const void *data, size_t len);
extern char *xstrndup(const char *str, size_t len);
diff --git a/path.c b/path.c
index a2c9d1e..332c903 100644
--- a/path.c
+++ b/path.c
@@ -87,6 +87,16 @@ char *git_pathdup(const char *fmt, ...)
return xstrdup(path);
}
+char *git_pathdup_permanent(const char *fmt, ...)
+{
+ char path[PATH_MAX];
+ va_list args;
+ va_start(args, fmt);
+ (void)git_vsnpath(path, sizeof(path), fmt, args);
+ va_end(args);
+ return strdup_permanent(path);
+}
+
char *mkpath(const char *fmt, ...)
{
va_list args;
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 7789edd..d237210 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -45,6 +45,23 @@
}
{
+ leaky malloc for option parsing and configuration
+ Memcheck:Leak
+ fun:malloc
+ fun:xmalloc
+ fun:alloc_permanent
+}
+
+{
+ leaky strdup for option parsing and configuration
+ Memcheck:Leak
+ fun:malloc
+ fun:strdup
+ fun:xstrdup
+ fun:strdup_permanent
+}
+
+{
see api-lockfile.txt
Memcheck:Leak
fun:calloc
diff --git a/wrapper.c b/wrapper.c
index fd8ead3..88ca3dd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,6 +3,8 @@
*/
#include "cache.h"
+#define KEEP_WRAPPER __attribute__((optimize("-fno-optimize-sibling-calls")))
+
static void try_to_free_builtin(size_t size)
{
release_pack_memory(size, -1);
@@ -17,6 +19,12 @@ try_to_free_t set_try_to_free_routine(try_to_free_t routine)
return old;
}
+KEEP_WRAPPER
+char *strdup_permanent(const char *str)
+{
+ return xstrdup(str);
+}
+
char *xstrdup(const char *str)
{
char *ret = strdup(str);
@@ -29,6 +37,12 @@ char *xstrdup(const char *str)
return ret;
}
+KEEP_WRAPPER
+void *alloc_permanent(size_t size)
+{
+ return xmalloc(size);
+}
+
void *xmalloc(size_t size)
{
void *ret = malloc(size);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 6/7] environment: use alloc_permanent() for computed git_dir and co
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
` (4 preceding siblings ...)
2010-10-02 8:38 ` [PATCH 5/7] Introduce malloc/strdup/pathdup variants for permanent allocations Jonathan Nieder
@ 2010-10-02 8:39 ` Jonathan Nieder
2010-10-02 8:41 ` [PATCH 7/7] commit-tree: free commit message before exiting Jonathan Nieder
6 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
The in-core variables to match $GIT_DIR, $GIT_OBJECT_DIRECTORY, etc
come from three sources:
a) environment variables [getenv("GIT_INDEX_FILE")]
b) computation [git_pathdup("objects")]
c) static strings [".git"]
Only in case (b) are they malloc()'d; even in that case, it
would not make sense to free the values until program termination,
when _exit() takes care of freeing all memory already.
So mark the allocations in case (b) as permanent so no one wastes
time trying to fix the leak.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
environment.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/environment.c b/environment.c
index de5581f..9bd18c7 100644
--- a/environment.c
+++ b/environment.c
@@ -89,23 +89,23 @@ static void setup_git_env(void)
git_dir = getenv(GIT_DIR_ENVIRONMENT);
if (!git_dir) {
git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
- git_dir = git_dir ? xstrdup(git_dir) : NULL;
+ git_dir = git_dir ? strdup_permanent(git_dir) : NULL;
}
if (!git_dir)
git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
git_object_dir = getenv(DB_ENVIRONMENT);
if (!git_object_dir) {
- git_object_dir = xmalloc(strlen(git_dir) + 9);
+ git_object_dir = alloc_permanent(strlen(git_dir) + 9);
sprintf(git_object_dir, "%s/objects", git_dir);
}
git_index_file = getenv(INDEX_ENVIRONMENT);
if (!git_index_file) {
- git_index_file = xmalloc(strlen(git_dir) + 7);
+ git_index_file = alloc_permanent(strlen(git_dir) + 7);
sprintf(git_index_file, "%s/index", git_dir);
}
git_graft_file = getenv(GRAFT_ENVIRONMENT);
if (!git_graft_file)
- git_graft_file = git_pathdup("info/grafts");
+ git_graft_file = git_pathdup_permanent("info/grafts");
if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
read_replace_refs = 0;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-02 8:27 ` [PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind Jonathan Nieder
` (5 preceding siblings ...)
2010-10-02 8:39 ` [PATCH 6/7] environment: use alloc_permanent() for computed git_dir and co Jonathan Nieder
@ 2010-10-02 8:41 ` Jonathan Nieder
2010-10-02 18:14 ` Sverre Rabbelier
2010-10-04 0:31 ` Junio C Hamano
6 siblings, 2 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 8:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
This buffer is freed by the C runtime when commit-tree exits moments
later, but freeing it explicitly should hopefully make this code
easier to reuse (in addition to making valgrind quieter).
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series. Thanks for reading.
builtin/commit-tree.c | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 87f0591..732f895 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -56,10 +56,12 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
if (strbuf_read(&buffer, 0, 0) < 0)
die_errno("git commit-tree: failed to read");
- if (!commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
- printf("%s\n", sha1_to_hex(commit_sha1));
- return 0;
- }
- else
+ if (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
+ strbuf_release(&buffer);
return 1;
+ }
+
+ printf("%s\n", sha1_to_hex(commit_sha1));
+ strbuf_release(&buffer);
+ return 0;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-02 8:41 ` [PATCH 7/7] commit-tree: free commit message before exiting Jonathan Nieder
@ 2010-10-02 18:14 ` Sverre Rabbelier
2010-10-02 18:26 ` Jonathan Nieder
2010-10-04 0:31 ` Junio C Hamano
1 sibling, 1 reply; 33+ messages in thread
From: Sverre Rabbelier @ 2010-10-02 18:14 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
Heya,
On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder <jrnieder@gmail.com> wrote:
> That's the end of the series. Thanks for reading.
Thanks, I've used git under valgrind a couple of times and always had
to resort to running 'regular' git, looking at what valgrind reports
for it, and then running my modified git, and try to make sure that I
didn't introduce any new leaks. How does the current valgrind output
look like, are we close to being leak free, or is this just a drop in
(or perhaps out of :P) the bucket?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-02 18:14 ` Sverre Rabbelier
@ 2010-10-02 18:26 ` Jonathan Nieder
2010-10-02 20:12 ` Sverre Rabbelier
0 siblings, 1 reply; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 18:26 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Junio C Hamano, git
Sverre Rabbelier wrote:
> On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> That's the end of the series. Thanks for reading.
>
> Thanks, I've used git under valgrind a couple of times and always had
> to resort to running 'regular' git, looking at what valgrind reports
> for it, and then running my modified git, and try to make sure that I
> didn't introduce any new leaks. How does the current valgrind output
> look like, are we close to being leak free, or is this just a drop in
> (or perhaps out of :P) the bucket?
It is a drop in the bucket. With some extra suppressions[1], I am
only able to run t0000 through valgrind.
Probably this series should cook outside mainline until the test suite
runs without reporting any unsuppressed leaks. I still would be
interested in more comments on the approach, though. If KEEP_WRAPPER
were conditionally defined to be empty (so by default people _would_
find the tail calls in those sentinel functions eliminated --- i.e.,
no performance impact unless you ask for it), would that be more
acceptable?
[1]
{
cache entry leak
Memcheck:Leak
fun:calloc
fun:xcalloc
fun:add_one_path
}
{
cache entry leak 2
Memcheck:Leak
fun:calloc
fun:xcalloc
fun:unpack_callback
}
{
nobody seems to free tree buffers
Memcheck:Leak
fun:malloc
fun:xmalloc
fun:xmallocz
fun:unpack_sha1_file
fun:read_object
fun:read_sha1_file_repl
fun:parse_object
fun:parse_tree_indirect
}
{
really, nobody seems to free tree buffers
Memcheck:Leak
fun:malloc
fun:xmalloc
fun:xmallocz
fun:unpack_sha1_file
fun:read_object
fun:read_sha1_file_repl
fun:parse_tree
}
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-02 18:26 ` Jonathan Nieder
@ 2010-10-02 20:12 ` Sverre Rabbelier
2010-10-02 20:50 ` Jonathan Nieder
0 siblings, 1 reply; 33+ messages in thread
From: Sverre Rabbelier @ 2010-10-02 20:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
Heya,
On Sat, Oct 2, 2010 at 20:26, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Probably this series should cook outside mainline until the test suite
> runs without reporting any unsuppressed leaks.
That would be very nice :).
> I still would be
> interested in more comments on the approach, though. If KEEP_WRAPPER
> were conditionally defined to be empty (so by default people _would_
> find the tail calls in those sentinel functions eliminated --- i.e.,
> no performance impact unless you ask for it), would that be more
> acceptable?
I suppose developers that are interested can easily turn it on in
their config.mak, and I think it's definitely useful to have this
available to make sure we don't introduce any new leaks.
The question of course is, is there anyone interested in cleaning up
this mess? Do you mean to keep it up and get the test suite to the
point where there are no leak warnings?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-02 20:12 ` Sverre Rabbelier
@ 2010-10-02 20:50 ` Jonathan Nieder
0 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-02 20:50 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Junio C Hamano, git
Sverre Rabbelier wrote:
> The question of course is, is there anyone interested in cleaning up
> this mess? Do you mean to keep it up and get the test suite to the
> point where there are no leak warnings?
I was thinking more along the lines of maintaining a branch with
--valgrind set up to do leak checking, and collecting patches to deal
with leak warnings as people write them.
My current itch is sussing out cherry-pick leaks. So at the moment I
only need to be able to run a small subset of the test suite.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-02 8:41 ` [PATCH 7/7] commit-tree: free commit message before exiting Jonathan Nieder
2010-10-02 18:14 ` Sverre Rabbelier
@ 2010-10-04 0:31 ` Junio C Hamano
2010-10-04 4:18 ` Jonathan Nieder
1 sibling, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2010-10-04 0:31 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git
Jonathan Nieder <jrnieder@gmail.com> writes:
> This buffer is freed by the C runtime when commit-tree exits moments
> later, but freeing it explicitly should hopefully make this code
> easier to reuse (in addition to making valgrind quieter).
I'd agree this is a change in a good direction because it will reduce
false positives from valgrind. I do not believe it would make it any
easier to reuse, though. Read what the code does (e.g. calling
git-config, showing usage or dying on sick arguments, etc.). So I have to
say that the proposed commit log message advertises a benefit that it does
not bring.
By the way, is there an easy and concise way to tell valgrind to
(1) treat allocations made in main() that are not freed as non-issues;
and
(2) treat functions whose name begin with cmd_ as if they are all their
own main(), and apply the above special casing to them?
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-04 0:31 ` Junio C Hamano
@ 2010-10-04 4:18 ` Jonathan Nieder
2010-10-04 7:55 ` Junio C Hamano
0 siblings, 1 reply; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-04 4:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sverre Rabbelier, Andreas Ericsson
Junio C Hamano wrote:
> Read what the code does (e.g. calling
> git-config, showing usage or dying on sick arguments, etc.). So I have to
> say that the proposed commit log message advertises a benefit that it does
> not bring.
Yep, that was sloppy of me. commit-tree has already been libified.
> By the way, is there an easy and concise way to tell valgrind to
>
> (1) treat allocations made in main() that are not freed as non-issues;
> and
>
> (2) treat functions whose name begin with cmd_ as if they are all their
> own main(), and apply the above special casing to them?
As long as the term "allocation" is defined clearly. For example
(written in the modern syntax for conciseness):
{
xmalloc from main()
Memcheck:Leak
...
fun:xmalloc
fun:main
}
{
xmalloc from built-in main
Memcheck:Leak
...
fun:xmalloc
fun:cmd_*
}
Even better would be to exit() instead of returning from cmd_
functions. Valgrind will not consider any memory that still has a
pointer to it at exit time as a leak.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-04 4:18 ` Jonathan Nieder
@ 2010-10-04 7:55 ` Junio C Hamano
2010-10-04 7:56 ` Jonathan Nieder
0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2010-10-04 7:55 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Sverre Rabbelier, Andreas Ericsson
Jonathan Nieder <jrnieder@gmail.com> writes:
> Even better would be to exit() instead of returning from cmd_
> functions. Valgrind will not consider any memory that still has a
> pointer to it at exit time as a leak.
Hmm, is there a way to tell valgrind that in these functions pretend as if
they left the scope by calling exit() when they return?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] commit-tree: free commit message before exiting
2010-10-04 7:55 ` Junio C Hamano
@ 2010-10-04 7:56 ` Jonathan Nieder
0 siblings, 0 replies; 33+ messages in thread
From: Jonathan Nieder @ 2010-10-04 7:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sverre Rabbelier, Andreas Ericsson
Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Even better would be to exit() instead of returning from cmd_
>> functions. Valgrind will not consider any memory that still has a
>> pointer to it at exit time as a leak.
>
> Hmm, is there a way to tell valgrind that in these functions pretend as if
> they left the scope by calling exit() when they return?
You mean a way to tell it to trap the return from these functions?
No, I don't think so.
^ permalink raw reply [flat|nested] 33+ messages in thread