Subject: Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean
To:     Jeff King <peff@peff.net>,
        =?UTF-8?Q?Carlo_Marcelo_Arenas_Bel=c3=b3n?= <carenas@gmail.com>
Cc:     git@vger.kernel.org
References: <20181104004957.52913-1-carenas@gmail.com>
 <20181104022746.GA30936@sigill.intra.peff.net>
From:   Derrick Stolee <stolee@gmail.com>
Message-ID: <0ace7b35-e967-6230-8733-e9cacebc75c1@gmail.com>
Date:   Mon, 5 Nov 2018 11:48:17 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:64.0) Gecko/20100101
 Thunderbird/64.0
MIME-Version: 1.0
In-Reply-To: <20181104022746.GA30936@sigill.intra.peff.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: git-owner@vger.kernel.org
Precedence: bulk

On 11/3/2018 10:27 PM, Jeff King wrote:
> On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:
>
>> introduced in 662148c435 ("midx: write object offsets", 2018-07-12)
>> but included on all previous versions as well.
>>
>> midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]
>>
>> likely an oversight as the information needed to iterate over is
>> embedded in nr_large_offset
> I've been preparing a series to make the whole code base compile with
> -Wunused-parameter, and I handled this case a bit differently.
>
> -- >8 --
> Subject: [PATCH] midx: double-check large object write loop
>
> The write_midx_large_offsets() function takes an array of object
> entries, the number of entries in the array (nr_objects), and the number
> of entries with large offsets (nr_large_offset). But we never actually
> use nr_objects; instead we keep walking down the array and counting down
> nr_large_offset until we've seen all of the large entries.
>
> This is correct, but we can be a bit more defensive. If there were ever
> a mismatch between nr_large_offset and the actual set of large-offset
> objects, we'd walk off the end of the array.
>
> Since we know the size of the array, we can use nr_objects to make sure
> we don't walk too far.
>
> Signed-off-by: Jeff King <peff@peff.net>

Thanks, both, for catching this. I prefer the approach that adds defenses.

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>

> ---
>   midx.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/midx.c b/midx.c
> index 4fac0cd08a..ecd583666a 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee
>   static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,
>   				       struct pack_midx_entry *objects, uint32_t nr_objects)
>   {
> -	struct pack_midx_entry *list = objects;
> +	struct pack_midx_entry *list = objects, *end = objects + nr_objects;
>   	size_t written = 0;
>   
>   	while (nr_large_offset) {
> -		struct pack_midx_entry *obj = list++;
> -		uint64_t offset = obj->offset;
> +		struct pack_midx_entry *obj;
> +		uint64_t offset;
> +
> +		if (list >= end)
> +			BUG("too many large-offset objects");
> +
> +		obj = list++;
> +		offset = obj->offset;
>   
>   		if (!(offset >> 31))
>   			continue;