Hi Taylor/Peff,
On Wed, 11 Nov 2020 at 20:43, Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> 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.
> + 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
