{"thread":{"id":"75","subject":"active_cache leaks","startedAt":"2005-04-17T07:07:06Z","lastAt":"2005-04-17T07:07:06Z","messageCount":1,"participants":["Brad Roberts"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"452","messageId":"Pine.LNX.4.44.0504162358130.2625-200000@bellevue.puremagic.com","threadId":"75","inReplyTo":null,"subject":"active_cache leaks","fromName":"Brad Roberts","fromEmail":"braddr@puremagic.com","sentAt":"2005-04-17T07:07:06Z","receivedAt":"2005-04-17T07:07:06Z","isPatch":false,"sender":{"key":"braddr@puremagic.com","avatar":null},"body":"Ok.. so there's been a couple attempts to patch the leak that were all\nwrong due to mixed memory management for that array.  Here's a seed for\ndiscussion on how to plug that leak.  Some would argue that it's not\nleaking enough to fix, but for those that want to turn git into a library,\nthe lifetime of the cache could end up not being short any more, so it's\nworth discussing how to fix it.\n\nThe q&d fix in this patch isn't elegant, but gets the job done.  More\ninteresting could be to have the entry itself contain a state bit, though\nthat wastes storage space.\n\nTwo basic changes:\n\n1) introduce a set_active_cache() api and change all 'active_cache[i] = ce'\n   calls to use it.\n2) add a active_cache_malloced array to parallel the active_cache array.\n\nI don't like #2, but see that q&d comment. :)\n\nIt's only lightly tested as I'm still trying to wrap my head around how to\nactually use git and git-pasky.\n\nI was tempted to add a get_cache_entry api as well, so that nothing\noutside of read-cache.c touched active_cache directly, but that can come\nnext.\n\nLater,\nBrad\n\n\n\n--- cache.h\n+++ cache.h\t2005-04-16 23:08:37.000000000 -0700\n@@ -88,6 +88,7 @@\n extern int read_cache(void);\n extern int write_cache(int newfd, struct cache_entry **cache, int entries);\n extern int cache_name_pos(const char *name, int namelen);\n+extern int set_cache_entry(struct cache_entry *ce, int pos, int malloced_entry);\n extern int add_cache_entry(struct cache_entry *ce, int ok_to_add);\n extern int remove_file_from_cache(char *path);\n extern int cache_match_stat(struct cache_entry *ce, struct stat *st);\n--- read-cache.c\n+++ read-cache.c\t2005-04-16 23:32:34.000000000 -0700\n@@ -8,6 +8,7 @@\n \n const char *sha1_file_directory = NULL;\n struct cache_entry **active_cache = NULL;\n+static int * active_cache_malloced = NULL;\n unsigned int active_nr = 0, active_alloc = 0;\n \n void usage(const char *err)\n@@ -381,6 +382,15 @@\n \treturn ce_namelen(b) == len && !memcmp(a->name, b->name, len);\n }\n \n+int set_cache_entry(struct cache_entry *ce, int pos, int malloced_entry)\n+{\n+\tif (active_cache_malloced[pos])\n+\t\tfree(active_cache[pos]);\n+\tactive_cache[pos] = ce;\n+\tactive_cache_malloced[pos] = malloced_entry;\n+\treturn 0;\n+}\n+\n int add_cache_entry(struct cache_entry *ce, int ok_to_add)\n {\n \tint pos;\n@@ -389,7 +399,7 @@\n \n \t/* existing match? Just replace it */\n \tif (pos >= 0) {\n-\t\tactive_cache[pos] = ce;\n+\t\tset_cache_entry(ce, pos, 0);\n \t\treturn 0;\n \t}\n \tpos = -pos-1;\n@@ -414,13 +424,16 @@\n \tif (active_nr == active_alloc) {\n \t\tactive_alloc = alloc_nr(active_alloc);\n \t\tactive_cache = realloc(active_cache, active_alloc * sizeof(struct cache_entry *));\n+\t\tactive_cache_malloced = realloc(active_cache, active_alloc * sizeof(int));\n \t}\n \n \t/* Add it in.. */\n \tactive_nr++;\n-\tif (active_nr > pos)\n+\tif (active_nr > pos) {\n \t\tmemmove(active_cache + pos + 1, active_cache + pos, (active_nr - pos - 1) * sizeof(ce));\n-\tactive_cache[pos] = ce;\n+\t\tmemmove(active_cache_malloced + pos + 1, active_cache_malloced + pos, (active_nr - pos - 1) * sizeof(int));\n+\t}\n+\tset_cache_entry(ce, pos, 1);\n \treturn 0;\n }\n \n@@ -482,12 +495,13 @@\n \tactive_nr = ntohl(hdr->hdr_entries);\n \tactive_alloc = alloc_nr(active_nr);\n \tactive_cache = calloc(active_alloc, sizeof(struct cache_entry *));\n+\tactive_cache_malloced = calloc(active_alloc, sizeof(int));\n \n \toffset = sizeof(*hdr);\n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = map + offset;\n \t\toffset = offset + ce_size(ce);\n-\t\tactive_cache[i] = ce;\n+\t\tset_cache_entry(ce, i, 0);\n \t}\n \treturn active_nr;\n \n--- update-cache.c\n+++ update-cache.c\t2005-04-16 23:33:28.000000000 -0700\n@@ -199,11 +199,14 @@\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tstruct cache_entry *new = refresh_entry(ce);\n \n+\t\tif (new == ce)\n+\t\t\tcontinue;\n+\n \t\tif (!new) {\n \t\t\tprintf(\"%s: needs update\\n\", ce->name);\n \t\t\tcontinue;\n \t\t}\n-\t\tactive_cache[i] = new;\n+\t\tset_cache_entry(new, i, 1);\n \t}\n }\n \n"}]}