Re: [PATCH 02/23] pack-bitmap: fix header size check

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 12, 2020, 17:39 UTC
Message-ID: <CAN0heSoVS0MhEN_91da5Uv=jimLRcEu+EX1mocm2+quB559vsQ@mail.gmail.com>
In-Reply-To: <c59fcbcc67556c5c9c5a22a2ee745a2f58234efd.1605123652.git.me@ttaylorr.com>

Hi Taylor/Peff,

On Wed, 11 Nov 2020 at 20:43, Taylor Blau <me@ttaylorr.com> wrote:

Show 6 quoted lines

>
> This meant we were overly strict about the header size (requiring room
> for a 32-byte worst-case hash, when sha1 is only 20 bytes). But in
> practice it didn't matter because bitmap files tend to have at least 12
> bytes of actual data anyway, so it was unlikely for a valid file to be
> caught by this.

Good catch.

Show 5 quoted lines

> +       size_t header_size = sizeof(*header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz;
>
> -       if (index->map_size < sizeof(*header) + the_hash_algo->rawsz)
> +       if (index->map_size < header_size)
>                 return error("Corrupted bitmap index (missing header data)");

I wondered if the "12" in the commit message shouldn't be "32". We used to count the hash bytes twice: first 32 that are included in the `sizeof()` and then another 20 or 32 on top of that. So we'd always count 32 too many.

Except, what the addition of `the_hash_algo->rawsz` tries to account for is the hash aaaaall the way at the end of the file -- not the one at the end of the header. That's my reading of the state before 0f4d6cada8 ("pack-bitmap: make bitmap header handling hash agnostic", 2019-02-19), anyway. So with that in mind, "12" makes sense.

I think we should actually check that we have room for the footer hash. I'll comment more on the next patch.

> -       index->map_pos += sizeof(*header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz;
> +       index->map_pos += header_size;

Makes sense.

Martin