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

From: Jeff King <peff@peff.net>
Date: Jun 19, 2015, 20:49 UTC
Message-ID: <20150619204923.GA2964@peff.net>
In-Reply-To: <1434740483-31730-1-git-send-email-sbeller@google.com>

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