Re: [PATCH v2 17/18] commit-reach: make can_all_from_reach... linear

From: Jeff King <peff@peff.net>
Date: Sep 12, 2018, 04:29 UTC
Message-ID: <20180912042938.GA15876@sigill.intra.peff.net>
In-Reply-To: <20180912041425.GA10472@sigill.intra.peff.net>

On Wed, Sep 12, 2018 at 12:14:25AM -0400, Jeff King wrote:

Show 26 quoted lines

> > +	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

---
 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);
 	}

-- 
2.19.0.600.ga229f7d059