[PATCH] revision.c: Correctly dereference interesting_cache

[PATCH] revision.c: Correctly dereference interesting_cache

[Stefan Beller]

This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid
quadratic behavior from still_interesting), which
also introduced the check a few lines before, which already dereferences
`interesting_cache`. So at this point `interesting_cache` is guaranteed to
be not NULL. The code is called referencing the address of a local
variable, so `interesting_cache` can actually never be NULL and trigger a
segmentation fault by dereferencing it a few lines before this.

I think the right thing is to check for `*interesting_cache` as that
can become NULL actually.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Hi Jeff,

I found this possible defect via coverity id 1295352.
As I have had limited exposure to revision.c code until now,
the commit message may or may not be bogus.

Thanks,
Stefan


 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 3ff8723..d1f0f07 100644
--- a/revision.c
+++ b/revision.c
@@ -347,35 +347,35 @@ static struct commit *handle_commit(struct rev_info *revs,
 
 static int everybody_uninteresting(struct commit_list *orig,
 				   struct commit **interesting_cache)
 {
 	struct commit_list *list = orig;
 
 	if (*interesting_cache) {
 		struct commit *commit = *interesting_cache;
 		if (!(commit->object.flags & UNINTERESTING))
 			return 0;
 	}
 
 	while (list) {
 		struct commit *commit = list->item;
 		list = list->next;
 		if (commit->object.flags & UNINTERESTING)
 			continue;
-		if (interesting_cache)
+		if (*interesting_cache)
 			*interesting_cache = commit;
 		return 0;
 	}
 	return 1;
 }
 
 /*
  * A definition of "relevant" commit that we can use to simplify limited graphs
  * by eliminating side branches.
  *
  * A "relevant" commit is one that is !UNINTERESTING (ie we are including it
  * in our list), or that is a specified BOTTOM commit. Then after computing
  * a limited list, during processing we can generally ignore boundary merges
  * coming from outside the graph, (ie from irrelevant parents), and treat
  * those merges as if they were single-parent. TREESAME is defined to consider
  * only relevant parents, if any. If we are TREESAME to our on-graph parents,
  * we don't care if we were !TREESAME to non-graph parents.
-- 
2.4.1.345.gab207b6.dirty

Re: [PATCH] revision.c: Correctly dereference interesting_cache

[Jeff King]

On Fri, Jun 19, 2015 at 12:01:23PM -0700, Stefan Beller wrote:

> This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid
> quadratic behavior from still_interesting), which
> also introduced the check a few lines before, which already dereferences
> `interesting_cache`. So at this point `interesting_cache` is guaranteed to
> be not NULL. The code is called referencing the address of a local
> variable, so `interesting_cache` can actually never be NULL and trigger a
> segmentation fault by dereferencing it a few lines before this.

Yeah, I agree it can never be NULL here or we would have already
segfaulted. Thanks for digging into it.

> I think the right thing is to check for `*interesting_cache` as that
> can become NULL actually.

I don't think this is right. We have found the interesting commit, so we
want to write it into the cache unconditionally, not only if there is
nothing else in the cache (we know if we got here that either there was
no current cache item, or it already became UNINTERESTING).

I think it is simply a misguided defensive measure to make sure that the
caller passed in a cache slot to us. But there is only one caller, and
they always pass a cache, so the first part of the function was lazy and
not defensive.

>  	while (list) {
>  		struct commit *commit = list->item;
>  		list = list->next;
>  		if (commit->object.flags & UNINTERESTING)
>  			continue;
> -		if (interesting_cache)
> +		if (*interesting_cache)
>  			*interesting_cache = commit;

So I think the right solution is just to drop the conditional entirely.
The current code is not wrong (it is always a noop). What you have here
actually misbehaves; it does not update the cache slot when it has
become UNINTERESTING. That does not produce wrong results, but it loses
the benefit of the cache in some cases.

-Peff

Re: [PATCH] revision.c: Correctly dereference interesting_cache

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid
> quadratic behavior from still_interesting), which
> also introduced the check a few lines before, which already dereferences
> `interesting_cache`. So at this point `interesting_cache` is guaranteed to
> be not NULL.

The above is the rationale for the coverity warning, but it does not
explain why this change is safe.

>                The code is called referencing the address of a local
> variable, so `interesting_cache` can actually never be NULL and trigger a
> segmentation fault by dereferencing it a few lines before this.

I'm having trouble parsing this sentence.  Do you mean that limit_list()
only calls still_interesting() (and thus, indirectly,
everybody_uninteresting()), with the second parameter equal to the
address of the local interesting_cache variable, so it can never be
NULL?

That makes sense, but I had to look at the code and reread the above
sentence a few times before I understood.

Do you know what this code is trying to check for?  What does it mean
for *interesting_cache to be NULL?

Should there be

	if (!interesting_cache)
		die("BUG: &interesting_cache == NULL");

checks at the top of still_interesting and everybody_uninteresting to
futureproof this?

What does the *interesting_cache variable represent, anyway?

This code seems to be underdocumented.

Thanks and hope that helps,
Jonathan