threads / patch / 39680

patchrevision.c: Correctly dereference interesting_cache

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

## tl;dr

3 messages between Jun 19, 2015 and Jun 19, 2015. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Stefan Beller· Jun 19, 2015, 19:01 UTC · lore

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(-)
Show changes to revision.c +1 −1
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
Jeff King· Jun 19, 2015, 20:49 UTC · re: Stefan Beller · lore

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

On Fri, Jun 19, 2015 at 12:01:23PM -0700, Stefan Beller wrote:
Show 7 quoted lines
> 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.

Show 8 quoted lines
>  	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
Jonathan Nieder· Jun 19, 2015, 21:00 UTC · re: Stefan Beller · lore

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

Hi,
Stefan Beller wrote:
Show 5 quoted lines
> 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

← back to recent threads