{"thread":{"id":"24677","subject":"[PATCH] memory leak reported by valgrind","startedAt":"2010-08-09T09:48:45Z","lastAt":"2010-10-06T22:17:48Z","messageCount":33,"participants":["Ivan Kanis","Junio C Hamano","Erik Faye-Lund","Alex Riesen","Jonathan Nieder","Andreas Ericsson","Sverre Rabbelier","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"147490","messageId":"wes62zknmki.fsf@kanis.fr","threadId":"24677","inReplyTo":null,"subject":"[PATCH] memory leak reported by valgrind","fromName":"Ivan Kanis","fromEmail":"expire-by-2010-08-14@kanis.fr","sentAt":"2010-08-09T09:48:45Z","receivedAt":"2010-08-09T09:48:45Z","isPatch":true,"sender":{"key":"expire-by-2010-08-14@kanis.fr","avatar":null},"body":"Hello,\n\nThis patches the following memory leak reported by valgrind:\n\n512 bytes in 1 blocks are definitely lost in loss record 5 of 8\n  at 0x4C203E4: calloc (vg_replace_malloc.c:397)\n  by 0x4C5F9D: xcalloc (wrapper.c:96)\n  by 0x445741: cmd_pack_objects (pack-objects.c:2117)\n  by 0x4048EE: handle_internal_command (git.c:270)\n  by 0x404B03: main (git.c:470)\n\n\ndiff -u git-1.7.2.1/builtin/pack-objects.c git.ivan/builtin/pack-objects.c\n--- git-1.7.2.1/builtin/pack-objects.c 2010-07-28 19:03:43.000000000 +0200\n+++ git.ivan/builtin/pack-objects.c 2010-08-09 11:38:40.000000000 +0200\n@@ -2341,5 +2341,6 @@\n fprintf(stderr, \"Total %\"PRIu32\" (delta %\"PRIu32\"),\"\n \" reused %\"PRIu32\" (delta %\"PRIu32\")\\n\",\n written, written_delta, reused, reused_delta);\n+ free(rp_av);\n return 0;\n }\n\n-- \nIvan Kanis\n\nWhen you're looking at life\nIn a strange new room\nMaybe drowning soon\nIs this the start of it all?\n -- Ian Curtis \n"},{"id":"147539","messageId":"7v1va760ip.fsf@alter.siamese.dyndns.org","threadId":"24677","inReplyTo":"wes62zknmki.fsf@kanis.fr","subject":"Re: [PATCH] memory leak reported by valgrind","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-09T19:37:02Z","receivedAt":"2010-08-09T19:37:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2)\nto clean after us?\n"},{"id":"147548","messageId":"AANLkTimrNv0VT_Nrmz_R0sFbL8QfMKPQjXhVJD95=KbE@mail.gmail.com","threadId":"24677","inReplyTo":"7v1va760ip.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] memory leak reported by valgrind","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2010-08-09T20:19:47Z","receivedAt":"2010-08-09T20:19:47Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Aug 9, 2010 at 9:37 PM, Junio C Hamano wrote:\n> Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2)\n> to clean after us?\n\nI don't know if you consider this relevant, but in Windows 9x (and ME)\nallocated memory isn't returned to the global heap on process\ntermination unless explicitly free'd.\n"},{"id":"147549","messageId":"AANLkTikg5Hcsz+COYrDtxc01tNay_D97aVHoMP06aO1J@mail.gmail.com","threadId":"24677","inReplyTo":"AANLkTimrNv0VT_Nrmz_R0sFbL8QfMKPQjXhVJD95=KbE@mail.gmail.com","subject":"Re: [PATCH] memory leak reported by valgrind","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-08-09T20:29:02Z","receivedAt":"2010-08-09T20:29:02Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Mon, Aug 9, 2010 at 22:19, Erik Faye-Lund wrote:\n>\n> On Mon, Aug 9, 2010 at 9:37 PM, Junio C Hamano wrote:\n> > Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2)\n> > to clean after us?\n>\n> I don't know if you consider this relevant, but in Windows 9x (and ME)\n> allocated memory isn't returned to the global heap on process\n> termination unless explicitly free'd.\n\n...and nothing but DOS, Windows 9x and ME does that.\n"},{"id":"147603","messageId":"20100810032647.GA2386@burratino","threadId":"24677","inReplyTo":"7v1va760ip.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] memory leak reported by valgrind","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-10T03:26:48Z","receivedAt":"2010-08-10T03:26:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Thanks, but doesn't it essentially sit at the end of main(), only for _exit(2)\n> to clean after us?\n\nRight. This is the case for many leaks reported by valgrind for git; it\nis hard to separate the ones the matter.\n\nExample: the lockfile records. There are so few per process as not to\nmatter except for the sake of valgrind, and they cannot be removed\nbefore exiting because the list they are inserted in is singly-linked.\nI am cooking a patch to free them at exit optionally, somewhat like\nPierre’s leaky()[1]. Could something like leaky() be used to insert\nan artificial stack frame at allocation time to make suppressions\nsimpler to write?\n\nWell, that’s a separate story. Here are a few run-of-the-mill leaks.\n\nJonathan Nieder (3):\n core: Stop leaking ondisk_cache_entrys\n write-tree: Avoid leak when index refers to an invalid object\n read-tree: stop leaking tree objects\n\n cache-tree.c | 4 +++-\n read-cache.c | 5 ++++-\n unpack-trees.c | 7 ++++++-\n 3 files changed, 13 insertions(+), 3 deletions(-)\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/86138\n"},{"id":"147604","messageId":"20100810032807.GB2386@burratino","threadId":"24677","inReplyTo":"20100810032647.GA2386@burratino","subject":"[PATCH/RFC 1/3] core: Stop leaking ondisk_cache_entrys","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-10T03:28:07Z","receivedAt":"2010-08-10T03:28:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Noticed with valgrind.\n\nSigned-off-by: Jonathan Nieder \n---\n read-cache.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex f1f789b..1f42473 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1516,6 +1516,7 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)\n \tint size = ondisk_ce_size(ce);\n \tstruct ondisk_cache_entry *ondisk = xcalloc(1, size);\n \tchar *name;\n+\tint result;\n \n \tondisk->ctime.sec = htonl(ce->ce_ctime.sec);\n \tondisk->mtime.sec = htonl(ce->ce_mtime.sec);\n@@ -1539,7 +1540,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)\n \t\tname = ondisk->name;\n \tmemcpy(name, ce->name, ce_namelen(ce));\n \n-\treturn ce_write(c, fd, ondisk, size);\n+\tresult = ce_write(c, fd, ondisk, size);\n+\tfree(ondisk);\n+\treturn result;\n }\n \n int write_index(struct index_state *istate, int newfd)\n-- \n1.7.2.1.544\n"},{"id":"147605","messageId":"20100810033211.GC2386@burratino","threadId":"24677","inReplyTo":"20100810032647.GA2386@burratino","subject":"[PATCH/RFC 2/3] write-tree: Avoid leak when index refers to an invalid object","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-10T03:32:11Z","receivedAt":"2010-08-10T03:32:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Noticed by valgrind during test t0000.35 “writing this tree without\n--missing-ok”.\n\nEven in the cherry-pick foo..bar code path, such an error is the\nend of the line. But maybe some day an interactive porcelain will\nwant to link to libgit, making this matter.\n\nSigned-off-by: Jonathan Nieder \n---\n cache-tree.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex d917437..c60cf91 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -328,9 +328,11 @@ static int update_one(struct cache_tree *it,\n \t\t\tmode = ce->ce_mode;\n \t\t\tentlen = pathlen - baselen;\n \t\t}\n-\t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))\n+\t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {\n+\t\t\tstrbuf_release(&buffer);\n \t\t\treturn error(\"invalid object %06o %s for '%.*s'\",\n \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\n+\t\t}\n \n \t\tif (ce->ce_flags & CE_REMOVE)\n \t\t\tcontinue; /* entry being removed */\n-- \n1.7.2.1.544.ga752d.dirty\n"},{"id":"147606","messageId":"20100810033344.GD2386@burratino","threadId":"24677","inReplyTo":"20100810032647.GA2386@burratino","subject":"[PATCH/RFC 3/3] read-tree: stop leaking tree objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-10T03:33:44Z","receivedAt":"2010-08-10T03:33:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The underlying problem is that the fill_tree_descriptor()\nAPI is easy to misuse, and this patch does not fix that.\n\nSigned-off-by: Jonathan Nieder \n---\n unpack-trees.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8cf0da3..f561d88 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -329,6 +329,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long\n {\n \tint i, ret, bottom;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n+\tvoid *buf[MAX_UNPACK_TREES];\n \tstruct traverse_info newinfo;\n \tstruct name_entry *p;\n \n@@ -346,12 +347,16 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long\n \t\tconst unsigned char *sha1 = NULL;\n \t\tif (dirmask & 1)\n \t\t\tsha1 = names[i].sha1;\n-\t\tfill_tree_descriptor(t+i, sha1);\n+\t\tbuf[i] = fill_tree_descriptor(t+i, sha1);\n \t}\n \n \tbottom = switch_cache_bottom(&newinfo);\n \tret = traverse_trees(n, t, &newinfo);\n \trestore_cache_bottom(&newinfo, bottom);\n+\n+\tfor (i = 0; i < n; i++)\n+\t\tfree(buf[i]);\n+\n \treturn ret;\n }\n \n-- \n1.7.2.1.544.ga752d.dirty\n"},{"id":"152279","messageId":"20101002082752.GA29638@burratino","threadId":"24677","inReplyTo":"20100810032647.GA2386@burratino","subject":"[PATCH/RFC 0/7] Re: [PATCH] memory leak reported by valgrind","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:27:52Z","receivedAt":"2010-10-02T08:27:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Could something like leaky() be used to insert\n> an artificial stack frame at allocation time to make suppressions\n> simpler to write?\n\nYes, I think it could.\n\nThanks to Pierre Habouzit for the initial idea and David Barr for\nadvice on the execution.\n\nThoughts? Bugs? Improvements?\n\nJonathan Nieder (7):\n init: plug tiny memory leak\n lockfile: introduce alloc_lock_file() to avoid valgrind noise\n environment.c: remove unused variable\n setup: make sure git dir path is in a permanent buffer\n Introduce malloc/strdup/pathdup variants for permanent allocations\n environment: use alloc_permanent() for computed git_dir and co\n commit-tree: free commit message before exiting\n\n Documentation/technical/api-lockfile.txt | 9 ++++++++\n builtin/commit-tree.c | 12 ++++++----\n builtin/init-db.c | 32 +++++++++++++++++------------\n builtin/update-index.c | 4 +--\n cache-tree.c | 7 +-----\n cache.h | 3 ++\n config.c | 2 +-\n environment.c | 14 ++++++------\n git-compat-util.h | 2 +\n lockfile.c | 6 +++++\n path.c | 10 +++++++++\n t/valgrind/default.supp | 25 +++++++++++++++++++++++\n wrapper.c | 14 +++++++++++++\n 13 files changed, 105 insertions(+), 35 deletions(-)\n\n-- \n1.7.2.3\n"},{"id":"152280","messageId":"20101002083122.GB29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 1/7] init: plug tiny one-time memory leak","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:31:22Z","receivedAt":"2010-10-02T08:31:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The buffer used to construct paths like \".git/objects/info\" and\n\".git/objects/pack\" is allocated on the heap and never freed.\n\nSo free it. While at it, factor out the relevant code into its own\nfunction to make it more obvious that this data is not used elsewhere.\n\nNoticed by valgrind while setting up tests (in test-lib).\n\nSigned-off-by: Jonathan Nieder \n---\n builtin/init-db.c | 32 +++++++++++++++++++-------------\n 1 files changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 0271285..a9c54ea 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -294,11 +294,26 @@ static int create_default_files(const char *template_path)\n \treturn reinit;\n }\n \n+static void create_sha1_dir(void)\n+{\n+\tconst char *sha1_dir = get_object_directory();\n+\tint len = strlen(sha1_dir);\n+\tchar *path = xmalloc(len + 40);\n+\n+\tmemcpy(path, sha1_dir, len);\n+\n+\tsafe_create_dir(sha1_dir, 1);\n+\tstrcpy(path+len, \"/pack\");\n+\tsafe_create_dir(path, 1);\n+\tstrcpy(path+len, \"/info\");\n+\tsafe_create_dir(path, 1);\n+\n+\tfree(path);\n+}\n+\n int init_db(const char *template_dir, unsigned int flags)\n {\n-\tconst char *sha1_dir;\n-\tchar *path;\n-\tint len, reinit;\n+\tint reinit;\n \n \tsafe_create_dir(get_git_dir(), 0);\n \n@@ -313,16 +328,7 @@ int init_db(const char *template_dir, unsigned int flags)\n \n \treinit = create_default_files(template_dir);\n \n-\tsha1_dir = get_object_directory();\n-\tlen = strlen(sha1_dir);\n-\tpath = xmalloc(len + 40);\n-\tmemcpy(path, sha1_dir, len);\n-\n-\tsafe_create_dir(sha1_dir, 1);\n-\tstrcpy(path+len, \"/pack\");\n-\tsafe_create_dir(path, 1);\n-\tstrcpy(path+len, \"/info\");\n-\tsafe_create_dir(path, 1);\n+\tcreate_sha1_dir();\n \n \tif (shared_repository) {\n \t\tchar buf[10];\n-- \n1.7.2.3\n"},{"id":"152281","messageId":"20101002083216.GC29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:32:16Z","receivedAt":"2010-10-02T08:32:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The current lockfile API often requires leaking memory:\n\n - the list of lockfiles to remove at exit is maintained in a\n singly linked list, so one cannot easily remove a lockfile\n record when it is no longer needed\n\n - some lockfile records are statically allocated and others are on\n the heap, so the remove_lock_file() atexit handler cannot easily\n free them at exit (not to mention that it would be a waste of time\n since _exit() takes care of freeing them anyway)\n\nAs the git libification effort proceeds, maybe this will become worth\nfixing. For now, since there are so few lock files used by a given\nprocess, it is simpler to tolerate the leak.\n\nAdd a alloc_lock_file() helper to make such tolerance easier.\nAllocations made through this helper function will not be reported as\nleaks by valgrind even if there is no corresponding free(). The\n((optimize(\"-fno-optimize-sibling-calls\"))) attribute is needed on\nplatforms with GCC to ensure that the stack frame for\nalloc_lock_file() is not replaced by the stack frame for xcalloc().\n\nFor illustration, this patch updates a few example xcalloc(1,\nsizeof(struct lockfile)) call sites to use the new wrapper.\n\nSigned-off-by: Jonathan Nieder \n---\n Documentation/technical/api-lockfile.txt | 9 +++++++++\n builtin/update-index.c | 4 +---\n cache-tree.c | 7 +------\n cache.h | 1 +\n config.c | 2 +-\n lockfile.c | 6 ++++++\n t/valgrind/default.supp | 8 ++++++++\n 7 files changed, 27 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex dd89404..8eac22a 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -22,6 +22,15 @@ The lockfile API serves two purposes:\n The functions\n -------------\n \n+alloc_lock_file::\n+\n+\tAllocate and clear a `struct lock_file`. Because the\n+\tstructure is used in an `atexit(3)` handler, its\n+\tstorage has to stay throughout the life of the\n+\tprogram. Using this allocation function instead of\n+\t`malloc` allows profiling tools like valgrind to keep\n+\ttrack of such storage and avoid reporting it as a leak.\n+\n hold_lock_file_for_update::\n \n \tTake a pointer to `struct lock_file`, the filename of\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 3ab214d..7697631 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -591,9 +591,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n-\t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n+\tlock_file = alloc_lock_file();\n \tnewfd = hold_locked_index(lock_file, 0);\n \tif (newfd < 0)\n \t\tlock_error = errno;\ndiff --git a/cache-tree.c b/cache-tree.c\nindex f755590..81cf785 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -550,12 +550,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \tint entries, was_valid, newfd;\n \tstruct lock_file *lock_file;\n \n-\t/*\n-\t * We can't free this memory, it becomes part of a linked list\n-\t * parsed atexit()\n-\t */\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\n+\tlock_file = alloc_lock_file();\n \tnewfd = hold_locked_index(lock_file, 1);\n \n \tentries = read_cache();\ndiff --git a/cache.h b/cache.h\nindex 2ef2fa3..9ee61a2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -517,6 +517,7 @@ struct lock_file {\n #define LOCK_NODEREF 2\n extern int unable_to_lock_error(const char *path, int err);\n extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n+extern struct lock_file *alloc_lock_file(void);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\ndiff --git a/config.c b/config.c\nindex 4b0a820..3f30e2b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1158,7 +1158,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tlock = xcalloc(sizeof(struct lock_file), 1);\n+\tlock = alloc_lock_file();\n \tfd = hold_lock_file_for_update(lock, config_filename, 0);\n \tif (fd < 0) {\n \t\terror(\"could not lock config file %s: %s\", config_filename, strerror(errno));\ndiff --git a/lockfile.c b/lockfile.c\nindex b0d74cd..161dea8 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -184,6 +184,12 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)\n \tdie(\"%s\", unable_to_lock_message(path, err));\n }\n \n+__attribute__((optimize(\"-fno-optimize-sibling-calls\")))\n+struct lock_file *alloc_lock_file(void)\n+{\n+\treturn xcalloc(1, sizeof(struct lock_file));\n+}\n+\n int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd = lock_file(lk, path, flags);\ndiff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\nindex 9e013fa..7789edd 100644\n--- a/t/valgrind/default.supp\n+++ b/t/valgrind/default.supp\n@@ -43,3 +43,11 @@\n \tfun:write_buffer\n \tfun:write_loose_object\n }\n+\n+{\n+\tsee api-lockfile.txt\n+\tMemcheck:Leak\n+\tfun:calloc\n+\tfun:xcalloc\n+\tfun:alloc_lock_file\n+}\n-- \n1.7.2.3\n"},{"id":"152282","messageId":"20101002083529.GD29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 3/7] environment.c: remove unused variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:35:29Z","receivedAt":"2010-10-02T08:35:29Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"After v1.6.0-rc0~230^2^ (environment.c: remove unused function,\n2008-06-19), git_refs_dir is not used any more.\n\nSigned-off-by: Jonathan Nieder \n---\n environment.c | 4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex 2d0c315..c44a30b 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -64,7 +64,7 @@ char *git_work_tree_cfg;\n static char *work_tree;\n \n static const char *git_dir;\n-static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;\n+static char *git_object_dir, *git_index_file, *git_graft_file;\n \n /*\n * Repository-local GIT_* environment variables\n@@ -96,8 +96,6 @@ static void setup_git_env(void)\n \t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\n \t\tsprintf(git_object_dir, \"%s/objects\", git_dir);\n \t}\n-\tgit_refs_dir = xmalloc(strlen(git_dir) + 6);\n-\tsprintf(git_refs_dir, \"%s/refs\", git_dir);\n \tgit_index_file = getenv(INDEX_ENVIRONMENT);\n \tif (!git_index_file) {\n \t\tgit_index_file = xmalloc(strlen(git_dir) + 7);\n-- \n1.7.2.3\n"},{"id":"152283","messageId":"20101002083652.GE29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 4/7] setup: make sure git dir path is in a permanent buffer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:36:52Z","receivedAt":"2010-10-02T08:36:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If setup_git_env() is run before the usual repository discovery\nsequence and .git is a file with the text\n\n\tgitdir: \n\n(with any string) then the in-core git_dir variable is set to\nthe result of converting to an absolute path using\nmake_absolute_path().\n\nUnfortunately make_absolute_path() returns its result in a static\nbuffer that is overwritten by later calls. Such a call could cause\nlater accesses to git_dir (from git_pathdup(), for example) to read\nthe wrong path, leaving git very confused.\n\nIt is not obvious whether any existing code in git will trigger the\nproblem, but in any case, it is worth a few dozen bytes to copy the\nreturn value from make_absolute_path() for some added peace of mind.\n\nSigned-off-by: Jonathan Nieder \n---\n environment.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex c44a30b..de5581f 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -87,8 +87,10 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {\n static void setup_git_env(void)\n {\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n-\tif (!git_dir)\n+\tif (!git_dir) {\n \t\tgit_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);\n+\t\tgit_dir = git_dir ? xstrdup(git_dir) : NULL;\n+\t}\n \tif (!git_dir)\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \tgit_object_dir = getenv(DB_ENVIRONMENT);\n-- \n1.7.2.3\n"},{"id":"152284","messageId":"20101002083851.GF29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 5/7] Introduce malloc/strdup/pathdup variants for permanent allocations","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:38:51Z","receivedAt":"2010-10-02T08:38:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"In git (option parsing and some other places), we sometimes leak tiny\nbits of memory on purpose. Introduce a way to declare such\nallocations, to make the lives of people searching for real memory\nleaks easier.\n\nCurrently these are thin wrappers around xmalloc and xstrdup. The\nKEEP_WRAPPER attribute avoids a tail call to ensure their stack frame\ndoes not get replaced with the true xmalloc/xstrdup, so tools like\nvalgrind can easily distinguish them and know that the memory they\nreturn is not meant to be freed.\n\nIn the long run it might be nice to tighten the semantics of these\nfunctions and use memory from a large pool that is never freed (to\navoid fragmentation and save some allocation overhead).\n\nSigned-off-by: Jonathan Nieder \n---\n cache.h | 2 ++\n git-compat-util.h | 2 ++\n path.c | 10 ++++++++++\n t/valgrind/default.supp | 17 +++++++++++++++++\n wrapper.c | 14 ++++++++++++++\n 5 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 9ee61a2..bbf32ff 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -638,6 +638,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n extern char *git_pathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n+extern char *git_pathdup_permanent(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n \n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 81883e7..f00ee33 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -380,7 +380,9 @@ typedef void (*try_to_free_t)(size_t);\n extern try_to_free_t set_try_to_free_routine(try_to_free_t);\n \n extern char *xstrdup(const char *str);\n+extern char *strdup_permanent(const char *str);\n extern void *xmalloc(size_t size);\n+extern void *alloc_permanent(size_t size);\n extern void *xmallocz(size_t size);\n extern void *xmemdupz(const void *data, size_t len);\n extern char *xstrndup(const char *str, size_t len);\ndiff --git a/path.c b/path.c\nindex a2c9d1e..332c903 100644\n--- a/path.c\n+++ b/path.c\n@@ -87,6 +87,16 @@ char *git_pathdup(const char *fmt, ...)\n \treturn xstrdup(path);\n }\n \n+char *git_pathdup_permanent(const char *fmt, ...)\n+{\n+\tchar path[PATH_MAX];\n+\tva_list args;\n+\tva_start(args, fmt);\n+\t(void)git_vsnpath(path, sizeof(path), fmt, args);\n+\tva_end(args);\n+\treturn strdup_permanent(path);\n+}\n+\n char *mkpath(const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\nindex 7789edd..d237210 100644\n--- a/t/valgrind/default.supp\n+++ b/t/valgrind/default.supp\n@@ -45,6 +45,23 @@\n }\n \n {\n+\tleaky malloc for option parsing and configuration\n+\tMemcheck:Leak\n+\tfun:malloc\n+\tfun:xmalloc\n+\tfun:alloc_permanent\n+}\n+\n+{\n+\tleaky strdup for option parsing and configuration\n+\tMemcheck:Leak\n+\tfun:malloc\n+\tfun:strdup\n+\tfun:xstrdup\n+\tfun:strdup_permanent\n+}\n+\n+{\n \tsee api-lockfile.txt\n \tMemcheck:Leak\n \tfun:calloc\ndiff --git a/wrapper.c b/wrapper.c\nindex fd8ead3..88ca3dd 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -3,6 +3,8 @@\n */\n #include \"cache.h\"\n \n+#define KEEP_WRAPPER __attribute__((optimize(\"-fno-optimize-sibling-calls\")))\n+\n static void try_to_free_builtin(size_t size)\n {\n \trelease_pack_memory(size, -1);\n@@ -17,6 +19,12 @@ try_to_free_t set_try_to_free_routine(try_to_free_t routine)\n \treturn old;\n }\n \n+KEEP_WRAPPER\n+char *strdup_permanent(const char *str)\n+{\n+\treturn xstrdup(str);\n+}\n+\n char *xstrdup(const char *str)\n {\n \tchar *ret = strdup(str);\n@@ -29,6 +37,12 @@ char *xstrdup(const char *str)\n \treturn ret;\n }\n \n+KEEP_WRAPPER\n+void *alloc_permanent(size_t size)\n+{\n+\treturn xmalloc(size);\n+}\n+\n void *xmalloc(size_t size)\n {\n \tvoid *ret = malloc(size);\n-- \n1.7.2.3\n"},{"id":"152285","messageId":"20101002083958.GG29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 6/7] environment: use alloc_permanent() for computed git_dir and co","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:39:58Z","receivedAt":"2010-10-02T08:39:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The in-core variables to match $GIT_DIR, $GIT_OBJECT_DIRECTORY, etc\ncome from three sources:\n\n a) environment variables [getenv(\"GIT_INDEX_FILE\")]\n b) computation [git_pathdup(\"objects\")]\n c) static strings [\".git\"]\n\nOnly in case (b) are they malloc()'d; even in that case, it\nwould not make sense to free the values until program termination,\nwhen _exit() takes care of freeing all memory already.\n\nSo mark the allocations in case (b) as permanent so no one wastes\ntime trying to fix the leak.\n\nSigned-off-by: Jonathan Nieder \n---\n environment.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex de5581f..9bd18c7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -89,23 +89,23 @@ static void setup_git_env(void)\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n \tif (!git_dir) {\n \t\tgit_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);\n-\t\tgit_dir = git_dir ? xstrdup(git_dir) : NULL;\n+\t\tgit_dir = git_dir ? strdup_permanent(git_dir) : NULL;\n \t}\n \tif (!git_dir)\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n \tgit_object_dir = getenv(DB_ENVIRONMENT);\n \tif (!git_object_dir) {\n-\t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\n+\t\tgit_object_dir = alloc_permanent(strlen(git_dir) + 9);\n \t\tsprintf(git_object_dir, \"%s/objects\", git_dir);\n \t}\n \tgit_index_file = getenv(INDEX_ENVIRONMENT);\n \tif (!git_index_file) {\n-\t\tgit_index_file = xmalloc(strlen(git_dir) + 7);\n+\t\tgit_index_file = alloc_permanent(strlen(git_dir) + 7);\n \t\tsprintf(git_index_file, \"%s/index\", git_dir);\n \t}\n \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n \tif (!git_graft_file)\n-\t\tgit_graft_file = git_pathdup(\"info/grafts\");\n+\t\tgit_graft_file = git_pathdup_permanent(\"info/grafts\");\n \tif (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))\n \t\tread_replace_refs = 0;\n }\n-- \n1.7.2.3\n"},{"id":"152286","messageId":"20101002084100.GH29638@burratino","threadId":"24677","inReplyTo":"20101002082752.GA29638@burratino","subject":"[PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T08:41:00Z","receivedAt":"2010-10-02T08:41:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This buffer is freed by the C runtime when commit-tree exits moments\nlater, but freeing it explicitly should hopefully make this code\neasier to reuse (in addition to making valgrind quieter).\n\nSigned-off-by: Jonathan Nieder \n---\nThat's the end of the series. Thanks for reading.\n\n builtin/commit-tree.c | 12 +++++++-----\n 1 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 87f0591..732f895 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -56,10 +56,12 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \tif (strbuf_read(&buffer, 0, 0) < 0)\n \t\tdie_errno(\"git commit-tree: failed to read\");\n \n-\tif (!commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {\n-\t\tprintf(\"%s\\n\", sha1_to_hex(commit_sha1));\n-\t\treturn 0;\n-\t}\n-\telse\n+\tif (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {\n+\t\tstrbuf_release(&buffer);\n \t\treturn 1;\n+\t}\n+\n+\tprintf(\"%s\\n\", sha1_to_hex(commit_sha1));\n+\tstrbuf_release(&buffer);\n+\treturn 0;\n }\n-- \n1.7.2.3\n"},{"id":"152291","messageId":"4CA75E21.1090407@op5.se","threadId":"24677","inReplyTo":"20101002083216.GC29638@burratino","subject":"Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2010-10-02T16:30:25Z","receivedAt":"2010-10-02T16:30:25Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 10/02/2010 10:32 AM, Jonathan Nieder wrote:\n> \n> Add a alloc_lock_file() helper to make such tolerance easier.\n> Allocations made through this helper function will not be reported as\n> leaks by valgrind even if there is no corresponding free(). The\n> ((optimize(\"-fno-optimize-sibling-calls\"))) attribute is needed on\n> platforms with GCC to ensure that the stack frame for\n> alloc_lock_file() is not replaced by the stack frame for xcalloc().\n> \n\nSo we're basically increasing runtime to shut up a leakchecking tool\nand also making that leakchecking tool falsely not report positives.\nHmm...\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"152292","messageId":"20101002164325.GA19675@burratino","threadId":"24677","inReplyTo":"4CA75E21.1090407@op5.se","subject":"Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T16:43:25Z","receivedAt":"2010-10-02T16:43:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andreas Ericsson wrote:\n\n> So we're basically increasing runtime to shut up a leakchecking tool\n> and also making that leakchecking tool falsely not report positives.\n\nYep, nice summary. Probably I should have also mentioned:\n\n - An extra stack frame with no locals is not a lot of overhead, but\n in any case these are by design not in performance-critical places.\n\n - By using a special function like this, we make instances nicely\n grep-able and give the leak prominence in t/valgrind/default.supp.\n So a person can discover, for example, that writing a lot of trees\n in a single process (like cherry-pick -n foo..bar currently does)\n is going to be leaky.\n\n - valgrind would be most useful if it can be used to identify\n _regressions_ by running the test suite with --valgrind. git is\n deliberately leaky in a lot of places; it is not useful to record\n that.\n\n - valgrind does have the ability to turn suppressions off if you want.\n"},{"id":"152293","messageId":"AANLkTikZ5cn8biMvMPzXz6U2J=3yg1KJXayHD-7Cx8Me@mail.gmail.com","threadId":"24677","inReplyTo":"20101002084100.GH29638@burratino","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-10-02T18:14:35Z","receivedAt":"2010-10-02T18:14:35Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Oct 2, 2010 at 10:41, Jonathan Nieder wrote:\n> That's the end of the series.  Thanks for reading.\n\nThanks, I've used git under valgrind a couple of times and always had\nto resort to running 'regular' git, looking at what valgrind reports\nfor it, and then running my modified git, and try to make sure that I\ndidn't introduce any new leaks. How does the current valgrind output\nlook like, are we close to being leak free, or is this just a drop in\n(or perhaps out of :P) the bucket?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"152295","messageId":"20101002182600.GB24523@burratino","threadId":"24677","inReplyTo":"AANLkTikZ5cn8biMvMPzXz6U2J=3yg1KJXayHD-7Cx8Me@mail.gmail.com","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T18:26:00Z","receivedAt":"2010-10-02T18:26:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n> On Sat, Oct 2, 2010 at 10:41, Jonathan Nieder wrote:\n\n>> That's the end of the series.  Thanks for reading.\n>\n> Thanks, I've used git under valgrind a couple of times and always had\n> to resort to running 'regular' git, looking at what valgrind reports\n> for it, and then running my modified git, and try to make sure that I\n> didn't introduce any new leaks. How does the current valgrind output\n> look like, are we close to being leak free, or is this just a drop in\n> (or perhaps out of :P) the bucket?\n\nIt is a drop in the bucket. With some extra suppressions[1], I am\nonly able to run t0000 through valgrind.\n\nProbably this series should cook outside mainline until the test suite\nruns without reporting any unsuppressed leaks. I still would be\ninterested in more comments on the approach, though. If KEEP_WRAPPER\nwere conditionally defined to be empty (so by default people _would_\nfind the tail calls in those sentinel functions eliminated --- i.e.,\nno performance impact unless you ask for it), would that be more\nacceptable?\n\n[1]\n {\n\tcache entry leak\n\tMemcheck:Leak\n\tfun:calloc\n\tfun:xcalloc\n\tfun:add_one_path\n }\n {\n\tcache entry leak 2\n\tMemcheck:Leak\n\tfun:calloc\n\tfun:xcalloc\n\tfun:unpack_callback\n }\n {\n\tnobody seems to free tree buffers\n\tMemcheck:Leak\n\tfun:malloc\n\tfun:xmalloc\n\tfun:xmallocz\n\tfun:unpack_sha1_file\n\tfun:read_object\n\tfun:read_sha1_file_repl\n\tfun:parse_object\n\tfun:parse_tree_indirect\n }\n {\n\treally, nobody seems to free tree buffers\n\tMemcheck:Leak\n\tfun:malloc\n\tfun:xmalloc\n\tfun:xmallocz\n\tfun:unpack_sha1_file\n\tfun:read_object\n\tfun:read_sha1_file_repl\n\tfun:parse_tree\n }\n"},{"id":"152296","messageId":"AANLkTim6hA5ShAYWm63vyybyTqRxgKVjv3EFcQ5dZWCW@mail.gmail.com","threadId":"24677","inReplyTo":"20101002182600.GB24523@burratino","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-10-02T20:12:15Z","receivedAt":"2010-10-02T20:12:15Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Oct 2, 2010 at 20:26, Jonathan Nieder wrote:\n> Probably this series should cook outside mainline until the test suite\n> runs without reporting any unsuppressed leaks.\n\nThat would be very nice :).\n\n> I still would be\n> interested in more comments on the approach, though.  If KEEP_WRAPPER\n> were conditionally defined to be empty (so by default people _would_\n> find the tail calls in those sentinel functions eliminated --- i.e.,\n> no performance impact unless you ask for it), would that be more\n> acceptable?\n\nI suppose developers that are interested can easily turn it on in\ntheir config.mak, and I think it's definitely useful to have this\navailable to make sure we don't introduce any new leaks.\n\nThe question of course is, is there anyone interested in cleaning up\nthis mess? Do you mean to keep it up and get the test suite to the\npoint where there are no leak warnings?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"152297","messageId":"20101002205044.GA30541@burratino","threadId":"24677","inReplyTo":"AANLkTim6hA5ShAYWm63vyybyTqRxgKVjv3EFcQ5dZWCW@mail.gmail.com","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-02T20:50:44Z","receivedAt":"2010-10-02T20:50:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Sverre Rabbelier wrote:\n\n> The question of course is, is there anyone interested in cleaning up\n> this mess? Do you mean to keep it up and get the test suite to the\n> point where there are no leak warnings?\n\nI was thinking more along the lines of maintaining a branch with\n--valgrind set up to do leak checking, and collecting patches to deal\nwith leak warnings as people write them.\n\nMy current itch is sussing out cherry-pick leaks. So at the moment I\nonly need to be able to run a small subset of the test suite.\n"},{"id":"152437","messageId":"7vpqvqdelx.fsf@alter.siamese.dyndns.org","threadId":"24677","inReplyTo":"20101002083122.GB29638@burratino","subject":"Re: [PATCH 1/7] init: plug tiny one-time memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-03T23:46:18Z","receivedAt":"2010-10-03T23:46:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The buffer used to construct paths like \".git/objects/info\" and\n> \".git/objects/pack\" is allocated on the heap and never freed.\n\nWhile at it it might be a good idea to rename its outdated name to\nsomething saner, e.g. object_dir, perhaps?\n\nIn v0.99 days, it make some sense to call that sha1_dir because we\nprepopulated 256 subdirectories 00-ff in it, and as an afterthought we\nreused the buffer to create an extra subdirectory \"/pack\" there. But we\ndo not create the fan-out in advance since 9106c09 (Create object\nsubdirectories on demand (phase II), 2005-10-09); sha1_dir is not a very\ngood name for the buffer to hold the path to \".git/objects\" anymore.\n"},{"id":"152438","messageId":"7vlj6edcic.fsf@alter.siamese.dyndns.org","threadId":"24677","inReplyTo":"20101002084100.GH29638@burratino","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-04T00:31:39Z","receivedAt":"2010-10-04T00:31:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> This buffer is freed by the C runtime when commit-tree exits moments\n> later, but freeing it explicitly should hopefully make this code\n> easier to reuse (in addition to making valgrind quieter).\n\nI'd agree this is a change in a good direction because it will reduce\nfalse positives from valgrind. I do not believe it would make it any\neasier to reuse, though. Read what the code does (e.g. calling\ngit-config, showing usage or dying on sick arguments, etc.). So I have to\nsay that the proposed commit log message advertises a benefit that it does\nnot bring.\n\nBy the way, is there an easy and concise way to tell valgrind to\n\n (1) treat allocations made in main() that are not freed as non-issues;\n and\n\n (2) treat functions whose name begin with cmd_ as if they are all their\n own main(), and apply the above special casing to them?\n"},{"id":"152455","messageId":"20101004041834.GD24884@burratino","threadId":"24677","inReplyTo":"7vlj6edcic.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T04:18:34Z","receivedAt":"2010-10-04T04:18:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Read what the code does (e.g. calling\n> git-config, showing usage or dying on sick arguments, etc.). So I have to\n> say that the proposed commit log message advertises a benefit that it does\n> not bring.\n\nYep, that was sloppy of me. commit-tree has already been libified.\n\n> By the way, is there an easy and concise way to tell valgrind to\n> \n> (1) treat allocations made in main() that are not freed as non-issues;\n> and\n> \n> (2) treat functions whose name begin with cmd_ as if they are all their\n> own main(), and apply the above special casing to them?\n\nAs long as the term \"allocation\" is defined clearly. For example\n(written in the modern syntax for conciseness):\n\n\t{\n\t\txmalloc from main()\n\t\tMemcheck:Leak\n\t\t...\n\t\tfun:xmalloc\n\t\tfun:main\n\t}\n\t{\n\t\txmalloc from built-in main\n\t\tMemcheck:Leak\n\t\t...\n\t\tfun:xmalloc\n\t\tfun:cmd_*\n\t}\n\nEven better would be to exit() instead of returning from cmd_\nfunctions. Valgrind will not consider any memory that still has a\npointer to it at exit time as a leak.\n"},{"id":"152456","messageId":"20101004043427.GE24884@burratino","threadId":"24677","inReplyTo":"7vpqvqdelx.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] init: plug tiny one-time memory leak","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T04:34:27Z","receivedAt":"2010-10-04T04:34:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The buffer used to construct paths like \".git/objects/info\" and\n\".git/objects/pack\" is allocated on the heap and never freed.\n\nSo free it. While at it, factor out the relevant code into its own\nfunction and rename the sha1_dir variable to object_directory (to\nmatch the change in everyday usage after the renaming of\nSHA1_FILE_DIRECTORY in v0.99~603^2~7, 2005).\n\nNoticed by valgrind while setting up tests (in test-lib).\n\nSigned-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n\n> While at it it might be a good idea to rename its outdated name to\n> something saner, e.g. object_dir, perhaps?\n\nYes.\n\n builtin/init-db.c | 32 +++++++++++++++++++-------------\n 1 files changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 0271285..9d4886c 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -294,11 +294,26 @@ static int create_default_files(const char *template_path)\n \treturn reinit;\n }\n \n+static void create_object_directory(void)\n+{\n+\tconst char *object_directory = get_object_directory();\n+\tint len = strlen(object_directory);\n+\tchar *path = xmalloc(len + 40);\n+\n+\tmemcpy(path, object_directory, len);\n+\n+\tsafe_create_dir(object_directory, 1);\n+\tstrcpy(path+len, \"/pack\");\n+\tsafe_create_dir(path, 1);\n+\tstrcpy(path+len, \"/info\");\n+\tsafe_create_dir(path, 1);\n+\n+\tfree(path);\n+}\n+\n int init_db(const char *template_dir, unsigned int flags)\n {\n-\tconst char *sha1_dir;\n-\tchar *path;\n-\tint len, reinit;\n+\tint reinit;\n \n \tsafe_create_dir(get_git_dir(), 0);\n \n@@ -313,16 +328,7 @@ int init_db(const char *template_dir, unsigned int flags)\n \n \treinit = create_default_files(template_dir);\n \n-\tsha1_dir = get_object_directory();\n-\tlen = strlen(sha1_dir);\n-\tpath = xmalloc(len + 40);\n-\tmemcpy(path, sha1_dir, len);\n-\n-\tsafe_create_dir(sha1_dir, 1);\n-\tstrcpy(path+len, \"/pack\");\n-\tsafe_create_dir(path, 1);\n-\tstrcpy(path+len, \"/info\");\n-\tsafe_create_dir(path, 1);\n+\tcreate_object_directory();\n \n \tif (shared_repository) {\n \t\tchar buf[10];\n-- \n1.7.2.3\n"},{"id":"152487","messageId":"7vtyl28k8k.fsf@alter.siamese.dyndns.org","threadId":"24677","inReplyTo":"20101004041834.GD24884@burratino","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-04T07:55:55Z","receivedAt":"2010-10-04T07:55:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Even better would be to exit() instead of returning from cmd_\n> functions. Valgrind will not consider any memory that still has a\n> pointer to it at exit time as a leak.\n\nHmm, is there a way to tell valgrind that in these functions pretend as if\nthey left the scope by calling exit() when they return?\n"},{"id":"152489","messageId":"20101004075613.GL24884@burratino","threadId":"24677","inReplyTo":"7vtyl28k8k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] commit-tree: free commit message before exiting","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-04T07:56:13Z","receivedAt":"2010-10-04T07:56:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Even better would be to exit() instead of returning from cmd_\n>> functions. Valgrind will not consider any memory that still has a\n>> pointer to it at exit time as a leak.\n>\n> Hmm, is there a way to tell valgrind that in these functions pretend as if\n> they left the scope by calling exit() when they return?\n\nYou mean a way to tell it to trap the return from these functions?\nNo, I don't think so.\n"},{"id":"152497","messageId":"AANLkTinmmtVCed9A0uwJ1kc-VF3J=oZKjUxM7YmCbDix@mail.gmail.com","threadId":"24677","inReplyTo":"20101002083652.GE29638@burratino","subject":"Re: [PATCH 4/7] setup: make sure git dir path is in a permanent buffer","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-04T09:25:00Z","receivedAt":"2010-10-04T09:25:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Oct 2, 2010 at 3:36 PM, Jonathan Nieder wrote:\n> It is not obvious whether any existing code in git will trigger the\n> problem, but in any case, it is worth a few dozen bytes to copy the\n> return value from make_absolute_path() for some added peace of mind.\n\nIt looks like make_absolute_path() is only used when git_dir has not\nbeen set in stone yet. Anyway, yes yes yes! I don't want to pull my\nhair when I find out this bug hits me.\n-- \nDuy\n"},{"id":"152777","messageId":"7vocb7yte4.fsf@alter.siamese.dyndns.org","threadId":"24677","inReplyTo":"20101002083216.GC29638@burratino","subject":"Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-06T20:10:27Z","receivedAt":"2010-10-06T20:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> ...The\n> ((optimize(\"-fno-optimize-sibling-calls\"))) attribute is needed on\n> platforms with GCC to ensure that the stack frame for\n> alloc_lock_file() is not replaced by the stack frame for xcalloc().\n\nHmm, I am getting\n\n cc1: warnings being treated as errors\n lockfile.c:189: error: 'optimize' attribute directive ignored\n make: *** [lockfile.o] Error 1\n\nfrom this patch with gcc (Debian 4.3.2-1.1) 4.3.2\n\nAren't \"struct lock_file\" instances supposed to be reachable from the\nlinked list, i.e. lock_file_list? Why does valgrind consider that\nelements on that list are leaked in the first place?\n\nWe keep loaded objects in *obj_hash[] and they sure are \"leaked\" by the\nsame definition of leakage, no? How are we squelching valgrind on them?\n"},{"id":"152778","messageId":"20101006202130.GB2118@burratino","threadId":"24677","inReplyTo":"7vocb7yte4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-06T20:21:30Z","receivedAt":"2010-10-06T20:21:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Hmm, I am getting\n> \n> cc1: warnings being treated as errors\n> lockfile.c:189: error: 'optimize' attribute directive ignored\n> make: *** [lockfile.o] Error 1\n> \n> from this patch with gcc (Debian 4.3.2-1.1) 4.3.2\n\nUnfortunate. With gcc 4.5 it works, but that isn't too useful.\n\n> Aren't \"struct lock_file\" instances supposed to be reachable from the\n> linked list, i.e. lock_file_list? Why does valgrind consider that\n> elements on that list are leaked in the first place?\n\nAt exit, we walk the lock file list and clear it in the process.\nWhich suggests a cleaner workaround (thanks!):\n\n static void remove_lock_file(void)\n {\n\tpid_t me = getpid();\n+\tstruct lock_file *p = lock_file_list;\n\n-\twhile (lock_file_list) {\n-\t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->filename[0]) {\n-\t\t\tif (lock_file_list->fd >= 0)\n-\t\t\t\tclose(lock_file_list->fd);\n-\t\t\tunlink_or_warn(lock_file_list->filename);\n-\t\t}\n-\t\tlock_file_list = lock_file_list->next;\n+\twhile (p) {\n+\t\tif (p->owner == me &&\n+\t\t p->filename[0]) {\n+\t\t\tif (p->fd >= 0)\n+\t\t\t\tclose(p->fd);\n+\t\t\tunlink_or_warn(p->filename);\n+\t\t}\n+\t\tp = lock_file_list->next;\n\t}\n"},{"id":"152783","messageId":"7vhbgzyozx.fsf@alter.siamese.dyndns.org","threadId":"24677","inReplyTo":"20101006202130.GB2118@burratino","subject":"Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-06T21:45:22Z","receivedAt":"2010-10-06T21:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> Hmm, I am getting\n>> \n>> cc1: warnings being treated as errors\n>> lockfile.c:189: error: 'optimize' attribute directive ignored\n>> make: *** [lockfile.o] Error 1\n>> \n>> from this patch with gcc (Debian 4.3.2-1.1) 4.3.2\n>\n> Unfortunate. With gcc 4.5 it works, but that isn't too useful.\n>\n>> Aren't \"struct lock_file\" instances supposed to be reachable from the\n>> linked list, i.e. lock_file_list? Why does valgrind consider that\n>> elements on that list are leaked in the first place?\n>\n> At exit, we walk the lock file list and clear it in the process.\n> Which suggests a cleaner workaround (thanks!):\n>\n> static void remove_lock_file(void)\n> {\n> \tpid_t me = getpid();\n> +\tstruct lock_file *p = lock_file_list;\n>\n> -\twhile (lock_file_list) {\n> -\t\tif (lock_file_list->owner == me &&\n> -\t\t lock_file_list->filename[0]) {\n> -\t\t\tif (lock_file_list->fd >= 0)\n> -\t\t\t\tclose(lock_file_list->fd);\n> -\t\t\tunlink_or_warn(lock_file_list->filename);\n> -\t\t}\n> -\t\tlock_file_list = lock_file_list->next;\n> +\twhile (p) {\n> +\t\tif (p->owner == me &&\n> +\t\t p->filename[0]) {\n> +\t\t\tif (p->fd >= 0)\n> +\t\t\t\tclose(p->fd);\n> +\t\t\tunlink_or_warn(p->filename);\n> +\t\t}\n> +\t\tp = lock_file_list->next;\n> \t}\n\nHeh, shouldn't the last one assign from p->next?\n"},{"id":"152807","messageId":"20101006221748.GB31818@burratino","threadId":"24677","inReplyTo":"7vhbgzyozx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/7] lockfile: introduce alloc_lock_file() to avoid valgrind noise","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-06T22:17:48Z","receivedAt":"2010-10-06T22:17:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> -\twhile (lock_file_list) {\n>> -\t\tif (lock_file_list->owner == me &&\n>> -\t\t lock_file_list->filename[0]) {\n>> -\t\t\tif (lock_file_list->fd >= 0)\n>> -\t\t\t\tclose(lock_file_list->fd);\n>> -\t\t\tunlink_or_warn(lock_file_list->filename);\n>> -\t\t}\n>> -\t\tlock_file_list = lock_file_list->next;\n>> +\twhile (p) {\n>> +\t\tif (p->owner == me &&\n>> +\t\t p->filename[0]) {\n>> +\t\t\tif (p->fd >= 0)\n>> +\t\t\t\tclose(p->fd);\n>> +\t\t\tunlink_or_warn(p->filename);\n>> +\t\t}\n>> +\t\tp = lock_file_list->next;\n>> \t}\n>\n> Heh, shouldn't the last one assign from p->next?\n\nYes. And I also should have mentioned: this is untested.\n"}]}