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

[Jeff King <peff@peff.net>]

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