{"thread":{"id":"1821","subject":"[PATCH] Plug diff leaks.","startedAt":"2005-09-16T07:38:43Z","lastAt":"2005-09-16T07:38:43Z","messageCount":1,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"8663","messageId":"7v3bo5sbr0.fsf@assigned-by-dhcp.cox.net","threadId":"1821","inReplyTo":null,"subject":"[PATCH] Plug diff leaks.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-16T07:38:43Z","receivedAt":"2005-09-16T07:38:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It is a bit embarrassing that it took this long for a fix since the\nproblem was first reported on Aug 13th.\n\n    Message-ID: <87y876gl1r.wl@mail2.atmark-techno.com>\n    From: Yasushi SHOJI <yashi@atmark-techno.com>\n    Newsgroups: gmane.comp.version-control.git\n    Subject: [patch] possible memory leak in diff.c::diff_free_filepair()\n    Date: Sat, 13 Aug 2005 19:58:56 +0900\n\nThis time I used valgrind to make sure that it does not overeagerly\ndiscard memory that is still being used.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n\n---\n\n * this will hopefully be in 0.99.7\n\n diff-tree.c       |    1 +\n diff.c            |    5 ++++-\n diffcore-rename.c |   56 ++++++++++++++++++++++++++++++-----------------------\n 3 files changed, 37 insertions(+), 25 deletions(-)\n\n5098bafb756de69d03882707a3382899c0cb7dd1\ndiff --git a/diff-tree.c b/diff-tree.c\n--- a/diff-tree.c\n+++ b/diff-tree.c\n@@ -370,6 +370,7 @@ static int diff_tree_commit(const unsign\n \t\t}\n \t\toffset += 48;\n \t}\n+\tfree(buf);\n \treturn 0;\n }\n \ndiff --git a/diff.c b/diff.c\n--- a/diff.c\n+++ b/diff.c\n@@ -762,7 +762,8 @@ struct diff_filepair *diff_queue(struct \n \tdp->status = 0;\n \tdp->source_stays = 0;\n \tdp->broken_pair = 0;\n-\tdiff_q(queue, dp);\n+\tif (queue)\n+\t\tdiff_q(queue, dp);\n \treturn dp;\n }\n \n@@ -770,6 +771,8 @@ void diff_free_filepair(struct diff_file\n {\n \tdiff_free_filespec_data(p->one);\n \tdiff_free_filespec_data(p->two);\n+\tfree(p->one);\n+\tfree(p->two);\n \tfree(p);\n }\n \ndiff --git a/diffcore-rename.c b/diffcore-rename.c\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -47,7 +47,8 @@ static struct diff_rename_dst *locate_re\n \tif (first < rename_dst_nr)\n \t\tmemmove(rename_dst + first + 1, rename_dst + first,\n \t\t\t(rename_dst_nr - first - 1) * sizeof(*rename_dst));\n-\trename_dst[first].two = two;\n+\trename_dst[first].two = alloc_filespec(two->path);\n+\tfill_filespec(rename_dst[first].two, two->sha1, two->mode);\n \trename_dst[first].pair = NULL;\n \treturn &(rename_dst[first]);\n }\n@@ -201,8 +202,7 @@ static int estimate_similarity(struct di\n \treturn score;\n }\n \n-static void record_rename_pair(struct diff_queue_struct *renq,\n-\t\t\t       int dst_index, int src_index, int score)\n+static void record_rename_pair(int dst_index, int src_index, int score)\n {\n \tstruct diff_filespec *one, *two, *src, *dst;\n \tstruct diff_filepair *dp;\n@@ -218,7 +218,7 @@ static void record_rename_pair(struct di\n \ttwo = alloc_filespec(dst->path);\n \tfill_filespec(two, dst->sha1, dst->mode);\n \n-\tdp = diff_queue(renq, one, two);\n+\tdp = diff_queue(NULL, one, two);\n \tdp->score = score;\n \tdp->source_stays = rename_src[src_index].src_path_left;\n \trename_dst[dst_index].pair = dp;\n@@ -252,15 +252,14 @@ static int compute_stays(struct diff_que\n void diffcore_rename(int detect_rename, int minimum_score)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tstruct diff_queue_struct renq, outq;\n+\tstruct diff_queue_struct outq;\n \tstruct diff_score *mx;\n-\tint i, j;\n+\tint i, j, rename_count;\n \tint num_create, num_src, dst_cnt;\n \n \tif (!minimum_score)\n \t\tminimum_score = DEFAULT_RENAME_SCORE;\n-\trenq.queue = NULL;\n-\trenq.nr = renq.alloc = 0;\n+\trename_count = 0;\n \n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n@@ -292,19 +291,19 @@ void diffcore_rename(int detect_rename, \n \t\t\tstruct diff_filespec *one = rename_src[j].one;\n \t\t\tif (!is_exact_match(one, two))\n \t\t\t\tcontinue;\n-\t\t\trecord_rename_pair(&renq, i, j, MAX_SCORE);\n+\t\t\trecord_rename_pair(i, j, MAX_SCORE);\n+\t\t\trename_count++;\n \t\t\tbreak; /* we are done with this entry */\n \t\t}\n \t}\n-\tdiff_debug_queue(\"done detecting exact\", &renq);\n \n \t/* Have we run out the created file pool?  If so we can avoid\n \t * doing the delta matrix altogether.\n \t */\n-\tif (renq.nr == rename_dst_nr)\n+\tif (rename_count == rename_dst_nr)\n \t\tgoto cleanup;\n \n-\tnum_create = (rename_dst_nr - renq.nr);\n+\tnum_create = (rename_dst_nr - rename_count);\n \tnum_src = rename_src_nr;\n \tmx = xmalloc(sizeof(*mx) * num_create * num_src);\n \tfor (dst_cnt = i = 0; i < rename_dst_nr; i++) {\n@@ -330,14 +329,14 @@ void diffcore_rename(int detect_rename, \n \t\t\tcontinue; /* already done, either exact or fuzzy. */\n \t\tif (mx[i].score < minimum_score)\n \t\t\tbreak; /* there is no more usable pair. */\n-\t\trecord_rename_pair(&renq, mx[i].dst, mx[i].src, mx[i].score);\n+\t\trecord_rename_pair(mx[i].dst, mx[i].src, mx[i].score);\n+\t\trename_count++;\n \t}\n \tfree(mx);\n-\tdiff_debug_queue(\"done detecting fuzzy\", &renq);\n \n  cleanup:\n \t/* At this point, we have found some renames and copies and they\n-\t * are kept in renq.  The original list is still in *q.\n+\t * are recorded in rename_dst.  The original list is still in *q.\n \t */\n \toutq.queue = NULL;\n \toutq.nr = outq.alloc = 0;\n@@ -372,9 +371,9 @@ void diffcore_rename(int detect_rename, \n \t\t\t *\n \t\t\t * (1) this is a broken delete and the counterpart\n \t\t\t *     broken create remains in the output; or\n-\t\t\t * (2) this is not a broken delete, and renq does\n-\t\t\t *     not have a rename/copy to move p->one->path\n-\t\t\t *     out.\n+\t\t\t * (2) this is not a broken delete, and rename_dst\n+\t\t\t *     does not have a rename/copy to move p->one->path\n+\t\t\t *     out of existence.\n \t\t\t *\n \t\t\t * Otherwise, the counterpart broken create\n \t\t\t * has been turned into a rename-edit; or\n@@ -390,11 +389,16 @@ void diffcore_rename(int detect_rename, \n \t\t\t\t\tpair_to_free = p;\n \t\t\t}\n \t\t\telse {\n-\t\t\t\tfor (j = 0; j < renq.nr; j++)\n-\t\t\t\t\tif (!strcmp(renq.queue[j]->one->path,\n-\t\t\t\t\t\t    p->one->path))\n-\t\t\t\t\t\tbreak;\n-\t\t\t\tif (j < renq.nr)\n+\t\t\t\tfor (j = 0; j < rename_dst_nr; j++) {\n+\t\t\t\t\tif (!rename_dst[j].pair)\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\tif (strcmp(rename_dst[j].pair->\n+\t\t\t\t\t\t   one->path,\n+\t\t\t\t\t\t   p->one->path))\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\tif (j < rename_dst_nr)\n \t\t\t\t\t/* this path remains */\n \t\t\t\t\tpair_to_free = p;\n \t\t\t}\n@@ -416,7 +420,6 @@ void diffcore_rename(int detect_rename, \n \t}\n \tdiff_debug_queue(\"done copying original\", &outq);\n \n-\tfree(renq.queue);\n \tfree(q->queue);\n \t*q = outq;\n \tdiff_debug_queue(\"done collapsing\", q);\n@@ -438,6 +441,11 @@ void diffcore_rename(int detect_rename, \n \t\t}\n \t}\n \n+\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\tdiff_free_filespec_data(rename_dst[i].two);\n+\t\tfree(rename_dst[i].two);\n+\t}\n+\n \tfree(rename_dst);\n \trename_dst = NULL;\n \trename_dst_nr = rename_dst_alloc = 0;\n"}]}