Re: [PATCH 5/7] grep: allocate subrepos on heap
From: Emily Shaffer <hidden>
Date: 2021-08-11 21:50:51
On Tue, Aug 10, 2021 at 11:28:43AM -0700, Jonathan Tan wrote:
quoted hunk ↗ jump to hunk
diff --git a/builtin/grep.c b/builtin/grep.c index 9e61c7c993..5a40e18e47 100644 --- a/builtin/grep.c +++ b/builtin/grep.c@@ -65,6 +65,9 @@ static int todo_done; /* Has all work items been added? */ static int all_work_added; +static struct repository **repos_to_free; +static size_t repos_to_free_nr, repos_to_free_alloc;
One thing I was curious about was whether it would make more sense to use an existing utility library in Git to handle this. But I think we kind of are, since ALLOC_GROW is used, so this is idiomatic for Git project. Ok, fine, I guess this is life at C ;)
quoted hunk ↗ jump to hunk
+ /* This lock protects all the variables above. */ static pthread_mutex_t grep_mutex;@@ -168,6 +171,17 @@ static void work_done(struct work_item *w) grep_unlock(); } +static void free_repos(void) +{ + int i; + + for (i = 0; i < repos_to_free_nr; i++) { + repo_clear(repos_to_free[i]); + free(repos_to_free[i]); + } + free(repos_to_free); +}
Should repos_to_free_nr be reset here? I guess it doesn't make sense to, since we'd be trying to use-after-free the initial repos_to_free head pointer too if we wanted to reuse this array.
quoted hunk ↗ jump to hunk
+ static void *run(void *arg) { int hit = 0;@@ -415,19 +429,24 @@ static int grep_submodule(struct grep_opt *opt, const struct object_id *oid, const char *filename, const char *path, int cached) { - struct repository subrepo; + struct repository *subrepo; struct repository *superproject = opt->repo; const struct submodule *sub; struct grep_opt subopt; - int hit; + int hit = 0; sub = submodule_from_path(superproject, null_oid(), path); if (!is_submodule_active(superproject, path)) return 0; - if (repo_submodule_init(&subrepo, superproject, sub)) + subrepo = xmalloc(sizeof(*subrepo)); + if (repo_submodule_init(subrepo, superproject, sub)) { + free(subrepo); return 0; + } + ALLOC_GROW(repos_to_free, repos_to_free_nr + 1, repos_to_free_alloc); + repos_to_free[repos_to_free_nr++] = subrepo;
Is this the only place we add to the repos_to_free array? It looks like yes, so I guess this doesn't need a helper.
quoted hunk ↗ jump to hunk
/* * NEEDSWORK: repo_read_gitmodules() might call@@ -438,7 +457,7 @@ static int grep_submodule(struct grep_opt *opt, * subrepo's odbs to the in-memory alternates list. */ obj_read_lock(); - repo_read_gitmodules(&subrepo, 0); + repo_read_gitmodules(subrepo, 0); /* * NEEDSWORK: This adds the submodule's object directory to the list of@@ -450,11 +469,11 @@ static int grep_submodule(struct grep_opt *opt, * store is no longer global and instead is a member of the repository * object. */ - add_submodule_odb_by_path(subrepo.objects->odb->path); + add_submodule_odb_by_path(subrepo->objects->odb->path); obj_read_unlock(); memcpy(&subopt, opt, sizeof(subopt)); - subopt.repo = &subrepo; + subopt.repo = subrepo; if (oid) { enum object_type object_type;@@ -464,9 +483,9 @@ static int grep_submodule(struct grep_opt *opt, struct strbuf base = STRBUF_INIT; obj_read_lock(); - object_type = oid_object_info(&subrepo, oid, NULL); + object_type = oid_object_info(subrepo, oid, NULL); obj_read_unlock(); - data = read_object_with_reference(&subrepo, + data = read_object_with_reference(subrepo, oid, tree_type, &size, NULL); if (!data)@@ -484,7 +503,6 @@ static int grep_submodule(struct grep_opt *opt, hit = grep_cache(&subopt, pathspec, cached); } - repo_clear(&subrepo); return hit; }@@ -1182,5 +1200,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) run_pager(&opt, prefix); clear_pathspec(&pathspec); free_grep_patterns(&opt); + free_repos(); return !hit; }
The rest of the diff seems to be a pretty clear object-becomes-reference conversion, so... Reviewed-by: Emily Shaffer <redacted>