From: Jeff King <peff@peff.net>
Date: Fri, 28 Mar 2025 05:49:53 GMT
Subject: Re: [PATCH v2 2/2] read-cache: check range before dereferencing an array element
Message-ID: <20250328054953.GA3590654@coredump.intra.peff.net>
In-Reply-To: <73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com>

On Thu, Mar 27, 2025 at 11:05:57AM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Before accessing an array element at a given index, we should make sure
> that the index is within the desired bounds, otherwise it makes little
> sense to access the array element in the first place.
> 
> In this instance, testing whether `ce->name[common]` is the trailing NUL
> byte is technically different from testing whether `common` is within
> the bounds of `previous_name`. It is also redundant, as the range-check
> guarantees that `previous_name->buf[common]` cannot be NUL and therefore
> the condition `ce->name[common] == previous_name->buf[common]` would not
> be met if `ce->name[common]` evaluated to NUL.
> 
> However, in the interest of reducing the cognitive load to reason about
> the correctness of this loop (so that I can focus on interesting
> projects again), I'll simply move the range-check to the beginning of
> the loop condition and keep the redundant NUL check.

Thanks, I think this explanation works, and the patch looks fine. (I
didn't dig deeply into patch 1, but I agree with Junio's analysis that
it is also a false positive).

-Peff