{"thread":{"id":"51601","subject":"[PATCH] commit-graph: release strbufs after use","startedAt":"2019-08-07T11:15:14Z","lastAt":"2019-08-07T19:25:13Z","messageCount":3,"participants":["René Scharfe","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380063","messageId":"c3be25bf-6487-52ce-217d-d6ee93b3a16f@web.de","threadId":"51601","inReplyTo":null,"subject":"[PATCH] commit-graph: release strbufs after use","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-07T11:15:02Z","receivedAt":"2019-08-07T11:15:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: René Scharfe <l.s.r@web.de>\n---\nPatch generated with --function-context for easier review.  That makes\nit look a lot bigger than it actually is, though.\n\nThe plugged leaks were added after v2.22.0 (2019-06-07) by the following\ncommits:\n\n5c84b3396c 2019-06-18 commit-graph: load commit-graph chains\nef5b83f2cf 2019-06-12 commit-graph: extract fill_oids_from_packs()\n8d84097f96 2019-06-18 commit-graph: expire commit-graph files\n\n commit-graph.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex b3c4de79b6..680c549f0f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -372,68 +372,69 @@ static int add_graph_to_chain(struct commit_graph *g,\n static struct commit_graph *load_commit_graph_chain(struct repository *r, const char *obj_dir)\n {\n \tstruct commit_graph *graph_chain = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct stat st;\n \tstruct object_id *oids;\n \tint i = 0, valid = 1, count;\n \tchar *chain_name = get_chain_filename(obj_dir);\n \tFILE *fp;\n \tint stat_res;\n\n \tfp = fopen(chain_name, \"r\");\n \tstat_res = stat(chain_name, &st);\n \tfree(chain_name);\n\n \tif (!fp ||\n \t    stat_res ||\n \t    st.st_size <= the_hash_algo->hexsz)\n \t\treturn NULL;\n\n \tcount = st.st_size / (the_hash_algo->hexsz + 1);\n \toids = xcalloc(count, sizeof(struct object_id));\n\n \tprepare_alt_odb(r);\n\n \tfor (i = 0; i < count; i++) {\n \t\tstruct object_directory *odb;\n\n \t\tif (strbuf_getline_lf(&line, fp) == EOF)\n \t\t\tbreak;\n\n \t\tif (get_oid_hex(line.buf, &oids[i])) {\n \t\t\twarning(_(\"invalid commit-graph chain: line '%s' not a hash\"),\n \t\t\t\tline.buf);\n \t\t\tvalid = 0;\n \t\t\tbreak;\n \t\t}\n\n \t\tvalid = 0;\n \t\tfor (odb = r->objects->odb; odb; odb = odb->next) {\n \t\t\tchar *graph_name = get_split_graph_filename(odb->path, line.buf);\n \t\t\tstruct commit_graph *g = load_commit_graph_one(graph_name);\n\n \t\t\tfree(graph_name);\n\n \t\t\tif (g) {\n \t\t\t\tg->obj_dir = odb->path;\n\n \t\t\t\tif (add_graph_to_chain(g, graph_chain, oids, i)) {\n \t\t\t\t\tgraph_chain = g;\n \t\t\t\t\tvalid = 1;\n \t\t\t\t}\n\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n\n \t\tif (!valid) {\n \t\t\twarning(_(\"unable to find all commit-graph files\"));\n \t\t\tbreak;\n \t\t}\n \t}\n\n \tfree(oids);\n \tfclose(fp);\n+\tstrbuf_release(&line);\n\n \treturn graph_chain;\n }\n@@ -1150,44 +1151,44 @@ int write_commit_graph_reachable(const char *obj_dir, unsigned int flags,\n static int fill_oids_from_packs(struct write_commit_graph_context *ctx,\n \t\t\t\tstruct string_list *pack_indexes)\n {\n \tuint32_t i;\n \tstruct strbuf progress_title = STRBUF_INIT;\n \tstruct strbuf packname = STRBUF_INIT;\n \tint dirlen;\n\n \tstrbuf_addf(&packname, \"%s/pack/\", ctx->obj_dir);\n \tdirlen = packname.len;\n \tif (ctx->report_progress) {\n \t\tstrbuf_addf(&progress_title,\n \t\t\t    Q_(\"Finding commits for commit graph in %d pack\",\n \t\t\t       \"Finding commits for commit graph in %d packs\",\n \t\t\t       pack_indexes->nr),\n \t\t\t    pack_indexes->nr);\n \t\tctx->progress = start_delayed_progress(progress_title.buf, 0);\n \t\tctx->progress_done = 0;\n \t}\n \tfor (i = 0; i < pack_indexes->nr; i++) {\n \t\tstruct packed_git *p;\n \t\tstrbuf_setlen(&packname, dirlen);\n \t\tstrbuf_addstr(&packname, pack_indexes->items[i].string);\n \t\tp = add_packed_git(packname.buf, packname.len, 1);\n \t\tif (!p) {\n \t\t\terror(_(\"error adding pack %s\"), packname.buf);\n \t\t\treturn -1;\n \t\t}\n \t\tif (open_pack_index(p)) {\n \t\t\terror(_(\"error opening index for %s\"), packname.buf);\n \t\t\treturn -1;\n \t\t}\n \t\tfor_each_object_in_pack(p, add_packed_commits, ctx,\n \t\t\t\t\tFOR_EACH_OBJECT_PACK_ORDER);\n \t\tclose_pack(p);\n \t\tfree(p);\n \t}\n\n \tstop_progress(&ctx->progress);\n-\tstrbuf_reset(&progress_title);\n+\tstrbuf_release(&progress_title);\n \tstrbuf_release(&packname);\n\n \treturn 0;\n }\n@@ -1695,56 +1696,57 @@ static void mark_commit_graphs(struct write_commit_graph_context *ctx)\n static void expire_commit_graphs(struct write_commit_graph_context *ctx)\n {\n \tstruct strbuf path = STRBUF_INIT;\n \tDIR *dir;\n \tstruct dirent *de;\n \tsize_t dirnamelen;\n \ttimestamp_t expire_time = time(NULL);\n\n \tif (ctx->split_opts && ctx->split_opts->expire_time)\n \t\texpire_time -= ctx->split_opts->expire_time;\n \tif (!ctx->split) {\n \t\tchar *chain_file_name = get_chain_filename(ctx->obj_dir);\n \t\tunlink(chain_file_name);\n \t\tfree(chain_file_name);\n \t\tctx->num_commit_graphs_after = 0;\n \t}\n\n \tstrbuf_addstr(&path, ctx->obj_dir);\n \tstrbuf_addstr(&path, \"/info/commit-graphs\");\n \tdir = opendir(path.buf);\n\n-\tif (!dir) {\n-\t\tstrbuf_release(&path);\n-\t\treturn;\n-\t}\n+\tif (!dir)\n+\t\tgoto out;\n\n \tstrbuf_addch(&path, '/');\n \tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n \t\tstruct stat st;\n \t\tuint32_t i, found = 0;\n\n \t\tstrbuf_setlen(&path, dirnamelen);\n \t\tstrbuf_addstr(&path, de->d_name);\n\n \t\tstat(path.buf, &st);\n\n \t\tif (st.st_mtime > expire_time)\n \t\t\tcontinue;\n \t\tif (path.len < 6 || strcmp(path.buf + path.len - 6, \".graph\"))\n \t\t\tcontinue;\n\n \t\tfor (i = 0; i < ctx->num_commit_graphs_after; i++) {\n \t\t\tif (!strcmp(ctx->commit_graph_filenames_after[i],\n \t\t\t\t    path.buf)) {\n \t\t\t\tfound = 1;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n\n \t\tif (!found)\n \t\t\tunlink(path.buf);\n \t}\n+\n+out:\n+\tstrbuf_release(&path);\n }\n\n int write_commit_graph(const char *obj_dir,\n--\n2.22.0\n"},{"id":"380071","messageId":"a2f30b81-ba93-579c-98ec-07f5df1043d7@gmail.com","threadId":"51601","inReplyTo":"c3be25bf-6487-52ce-217d-d6ee93b3a16f@web.de","subject":"Re: [PATCH] commit-graph: release strbufs after use","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-08-07T13:16:24Z","receivedAt":"2019-08-07T13:16:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/7/2019 7:15 AM, René Scharfe wrote:\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Patch generated with --function-context for easier review.  That makes\n> it look a lot bigger than it actually is, though.\n\nThanks for the --function-context. It really does clarify what's going on,\nespecially in the case with the \"out:\" label.\n \n> The plugged leaks were added after v2.22.0 (2019-06-07) by the following\n> commits:\n> \n> 5c84b3396c 2019-06-18 commit-graph: load commit-graph chains\n> ef5b83f2cf 2019-06-12 commit-graph: extract fill_oids_from_packs()\n> 8d84097f96 2019-06-18 commit-graph: expire commit-graph files\n\nYour changes look good to me.\n\n> -\tstrbuf_reset(&progress_title);\n> +\tstrbuf_release(&progress_title);\n\nThis line confused me as I'm sure I adapted it from another place in code,\nand sure enough in the old code, progress_title was re-used between multiple\nstages. That's why it was a 'reset' when it should have been a 'release'.\n\nThanks!\n-Stolee\n"},{"id":"380092","messageId":"xmqqsgqchj73.fsf@gitster-ct.c.googlers.com","threadId":"51601","inReplyTo":"a2f30b81-ba93-579c-98ec-07f5df1043d7@gmail.com","subject":"Re: [PATCH] commit-graph: release strbufs after use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-07T19:25:04Z","receivedAt":"2019-08-07T19:25:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 8/7/2019 7:15 AM, René Scharfe wrote:\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> Patch generated with --function-context for easier review.  That makes\n>> it look a lot bigger than it actually is, though.\n>\n> Thanks for the --function-context. It really does clarify what's going on,\n> especially in the case with the \"out:\" label.\n\nYes in general, but it can cut both ways ;-)\n\n>> The plugged leaks were added after v2.22.0 (2019-06-07) by the following\n>> commits:\n>> \n>> 5c84b3396c 2019-06-18 commit-graph: load commit-graph chains\n>> ef5b83f2cf 2019-06-12 commit-graph: extract fill_oids_from_packs()\n>> 8d84097f96 2019-06-18 commit-graph: expire commit-graph files\n>\n> Your changes look good to me.\n\nThanks.\n\n>\n>> -\tstrbuf_reset(&progress_title);\n>> +\tstrbuf_release(&progress_title);\n>\n> This line confused me as I'm sure I adapted it from another place in code,\n> and sure enough in the old code, progress_title was re-used between multiple\n> stages. That's why it was a 'reset' when it should have been a 'release'.\n>\n> Thanks!\n> -Stolee\n"}]}