[PATCH] commit-graph: release strbufs after use

[PATCH] commit-graph: release strbufs after use

[René Scharfe]

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Patch generated with --function-context for easier review.  That makes
it look a lot bigger than it actually is, though.

The plugged leaks were added after v2.22.0 (2019-06-07) by the following
commits:

5c84b3396c 2019-06-18 commit-graph: load commit-graph chains
ef5b83f2cf 2019-06-12 commit-graph: extract fill_oids_from_packs()
8d84097f96 2019-06-18 commit-graph: expire commit-graph files

 commit-graph.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index b3c4de79b6..680c549f0f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -372,68 +372,69 @@ static int add_graph_to_chain(struct commit_graph *g,
 static struct commit_graph *load_commit_graph_chain(struct repository *r, const char *obj_dir)
 {
 	struct commit_graph *graph_chain = NULL;
 	struct strbuf line = STRBUF_INIT;
 	struct stat st;
 	struct object_id *oids;
 	int i = 0, valid = 1, count;
 	char *chain_name = get_chain_filename(obj_dir);
 	FILE *fp;
 	int stat_res;

 	fp = fopen(chain_name, "r");
 	stat_res = stat(chain_name, &st);
 	free(chain_name);

 	if (!fp ||
 	    stat_res ||
 	    st.st_size <= the_hash_algo->hexsz)
 		return NULL;

 	count = st.st_size / (the_hash_algo->hexsz + 1);
 	oids = xcalloc(count, sizeof(struct object_id));

 	prepare_alt_odb(r);

 	for (i = 0; i < count; i++) {
 		struct object_directory *odb;

 		if (strbuf_getline_lf(&line, fp) == EOF)
 			break;

 		if (get_oid_hex(line.buf, &oids[i])) {
 			warning(_("invalid commit-graph chain: line '%s' not a hash"),
 				line.buf);
 			valid = 0;
 			break;
 		}

 		valid = 0;
 		for (odb = r->objects->odb; odb; odb = odb->next) {
 			char *graph_name = get_split_graph_filename(odb->path, line.buf);
 			struct commit_graph *g = load_commit_graph_one(graph_name);

 			free(graph_name);

 			if (g) {
 				g->obj_dir = odb->path;

 				if (add_graph_to_chain(g, graph_chain, oids, i)) {
 					graph_chain = g;
 					valid = 1;
 				}

 				break;
 			}
 		}

 		if (!valid) {
 			warning(_("unable to find all commit-graph files"));
 			break;
 		}
 	}

 	free(oids);
 	fclose(fp);
+	strbuf_release(&line);

 	return graph_chain;
 }
@@ -1150,44 +1151,44 @@ int write_commit_graph_reachable(const char *obj_dir, unsigned int flags,
 static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 				struct string_list *pack_indexes)
 {
 	uint32_t i;
 	struct strbuf progress_title = STRBUF_INIT;
 	struct strbuf packname = STRBUF_INIT;
 	int dirlen;

 	strbuf_addf(&packname, "%s/pack/", ctx->obj_dir);
 	dirlen = packname.len;
 	if (ctx->report_progress) {
 		strbuf_addf(&progress_title,
 			    Q_("Finding commits for commit graph in %d pack",
 			       "Finding commits for commit graph in %d packs",
 			       pack_indexes->nr),
 			    pack_indexes->nr);
 		ctx->progress = start_delayed_progress(progress_title.buf, 0);
 		ctx->progress_done = 0;
 	}
 	for (i = 0; i < pack_indexes->nr; i++) {
 		struct packed_git *p;
 		strbuf_setlen(&packname, dirlen);
 		strbuf_addstr(&packname, pack_indexes->items[i].string);
 		p = add_packed_git(packname.buf, packname.len, 1);
 		if (!p) {
 			error(_("error adding pack %s"), packname.buf);
 			return -1;
 		}
 		if (open_pack_index(p)) {
 			error(_("error opening index for %s"), packname.buf);
 			return -1;
 		}
 		for_each_object_in_pack(p, add_packed_commits, ctx,
 					FOR_EACH_OBJECT_PACK_ORDER);
 		close_pack(p);
 		free(p);
 	}

 	stop_progress(&ctx->progress);
-	strbuf_reset(&progress_title);
+	strbuf_release(&progress_title);
 	strbuf_release(&packname);

 	return 0;
 }
@@ -1695,56 +1696,57 @@ static void mark_commit_graphs(struct write_commit_graph_context *ctx)
 static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 {
 	struct strbuf path = STRBUF_INIT;
 	DIR *dir;
 	struct dirent *de;
 	size_t dirnamelen;
 	timestamp_t expire_time = time(NULL);

 	if (ctx->split_opts && ctx->split_opts->expire_time)
 		expire_time -= ctx->split_opts->expire_time;
 	if (!ctx->split) {
 		char *chain_file_name = get_chain_filename(ctx->obj_dir);
 		unlink(chain_file_name);
 		free(chain_file_name);
 		ctx->num_commit_graphs_after = 0;
 	}

 	strbuf_addstr(&path, ctx->obj_dir);
 	strbuf_addstr(&path, "/info/commit-graphs");
 	dir = opendir(path.buf);

-	if (!dir) {
-		strbuf_release(&path);
-		return;
-	}
+	if (!dir)
+		goto out;

 	strbuf_addch(&path, '/');
 	dirnamelen = path.len;
 	while ((de = readdir(dir)) != NULL) {
 		struct stat st;
 		uint32_t i, found = 0;

 		strbuf_setlen(&path, dirnamelen);
 		strbuf_addstr(&path, de->d_name);

 		stat(path.buf, &st);

 		if (st.st_mtime > expire_time)
 			continue;
 		if (path.len < 6 || strcmp(path.buf + path.len - 6, ".graph"))
 			continue;

 		for (i = 0; i < ctx->num_commit_graphs_after; i++) {
 			if (!strcmp(ctx->commit_graph_filenames_after[i],
 				    path.buf)) {
 				found = 1;
 				break;
 			}
 		}

 		if (!found)
 			unlink(path.buf);
 	}
+
+out:
+	strbuf_release(&path);
 }

 int write_commit_graph(const char *obj_dir,
--
2.22.0

Re: [PATCH] commit-graph: release strbufs after use

[Derrick Stolee]

On 8/7/2019 7:15 AM, René Scharfe wrote:
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Patch generated with --function-context for easier review.  That makes
> it look a lot bigger than it actually is, though.

Thanks for the --function-context. It really does clarify what's going on,
especially in the case with the "out:" label.
 
> The plugged leaks were added after v2.22.0 (2019-06-07) by the following
> commits:
> 
> 5c84b3396c 2019-06-18 commit-graph: load commit-graph chains
> ef5b83f2cf 2019-06-12 commit-graph: extract fill_oids_from_packs()
> 8d84097f96 2019-06-18 commit-graph: expire commit-graph files

Your changes look good to me.

> -	strbuf_reset(&progress_title);
> +	strbuf_release(&progress_title);

This line confused me as I'm sure I adapted it from another place in code,
and sure enough in the old code, progress_title was re-used between multiple
stages. That's why it was a 'reset' when it should have been a 'release'.

Thanks!
-Stolee

Re: [PATCH] commit-graph: release strbufs after use

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 8/7/2019 7:15 AM, René Scharfe wrote:
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>> Patch generated with --function-context for easier review.  That makes
>> it look a lot bigger than it actually is, though.
>
> Thanks for the --function-context. It really does clarify what's going on,
> especially in the case with the "out:" label.

Yes in general, but it can cut both ways ;-)

>> The plugged leaks were added after v2.22.0 (2019-06-07) by the following
>> commits:
>> 
>> 5c84b3396c 2019-06-18 commit-graph: load commit-graph chains
>> ef5b83f2cf 2019-06-12 commit-graph: extract fill_oids_from_packs()
>> 8d84097f96 2019-06-18 commit-graph: expire commit-graph files
>
> Your changes look good to me.

Thanks.

>
>> -	strbuf_reset(&progress_title);
>> +	strbuf_release(&progress_title);
>
> This line confused me as I'm sure I adapted it from another place in code,
> and sure enough in the old code, progress_title was re-used between multiple
> stages. That's why it was a 'reset' when it should have been a 'release'.
>
> Thanks!
> -Stolee