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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 19, 2015, 21:00 UTC
Message-ID: <20150619210023.GA4865@google.com>
In-Reply-To: <1434740483-31730-1-git-send-email-sbeller@google.com>

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