On 9/12/2018 12:29 AM, Jeff King wrote:
On Wed, Sep 12, 2018 at 12:14:25AM -0400, Jeff King wrote:
+ ALLOC_ARRAY(list, from->nr);
for (i = 0; i < from->nr; i++) {
- struct object *from_one = from->objects[i].item;
+ list[i] = (struct commit *)from->objects[i].item;
Some of the objects in my array are not commits, but rather tags, so
this is a bogus cast.
You can see that the original code peeled them and threw away
non-commits:
- if (from_one->flags & assign_flag)
- continue;
- from_one = deref_tag(the_repository, from_one, "a from object", 0);
- if (!from_one || from_one->type != OBJ_COMMIT) {
- /* no way to tell if this is reachable by
- * looking at the ancestry chain alone, so
- * leave a note to ourselves not to worry about
- * this object anymore.
- */
- from->objects[i].item->flags |= assign_flag;
- continue;
- }
So presumably we'd need to do something similar.
This patch seems to fix it for me. It's more or less a reversion of the
hunk above, though I didn't dig into whether I'm violating some other
assumption in your new code.
I think this function leaks "list" both from the location I noted here,
as well as from normal exit
Thanks for the report and the fix. I'll try to create test that
demonstrates this and then push up a full patch.
---
commit-reach.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/commit-reach.c b/commit-reach.c
index 622eeb313d..abe90a2f55 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -547,20 +547,31 @@ int can_all_from_reach_with_flag(struct object_array *from,
{
struct commit **list = NULL;
int i;
+ int nr_commits;
int result = 1;
ALLOC_ARRAY(list, from->nr);
+ nr_commits = 0;
for (i = 0; i < from->nr; i++) {
- list[i] = (struct commit *)from->objects[i].item;
+ struct object *from_one = from->objects[i].item;
- if (parse_commit(list[i]) ||
- list[i]->generation < min_generation)
- return 0;
+ from_one = deref_tag(the_repository, from_one,
+ "a from object", 0);
+ if (!from_one || from_one->type != OBJ_COMMIT) {
+ from->objects[i].item->flags |= assign_flag;
+ continue;
+ }
+
+ list[nr_commits] = (struct commit *)from_one;
+ if (parse_commit(list[nr_commits]) ||
+ list[nr_commits]->generation < min_generation)
+ return 0; /* is this a leak? */
+ nr_commits++;
}
- QSORT(list, from->nr, compare_commits_by_gen);
+ QSORT(list, nr_commits, compare_commits_by_gen);
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
/* DFS from list[i] */
struct commit_list *stack = NULL;
@@ -603,7 +614,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < from->nr; i++) {
+ for (i = 0; i < nr_commits; i++) {
clear_commit_marks(list[i], RESULT);
clear_commit_marks(list[i], assign_flag);
}
