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

Commit Message

Taylor Blau Nov. 11, 2020, 7:41 p.m. UTC

From: Jeff King <[e-mail]>

When we parse a .bitmap header, we first check that we have enough bytes
to make a valid header. We do that based on sizeof(struct
bitmap_disk_header). However, as of 0f4d6cada8 (pack-bitmap: make bitmap
header handling hash agnostic, 2019-02-19), that struct oversizes its
checksum member to GIT_MAX_RAWSZ. That means we need to adjust for the
difference between that constant and the size of the actual hash we're
using. That commit adjusted the code which moves our pointer forward,
but forgot to update the size check.

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.

Let's fix it by pulling the header size into a separate variable and
using it in both spots. That fixes the bug and simplifies the code to make
it harder to have a mismatch like this in the future. It will also come
in handy in the next patch for more bounds checking.

Signed-off-by: Jeff King <[e-mail]>
Signed-off-by: Taylor Blau <[e-mail]>
---
 pack-bitmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Martin Ågren Nov. 12, 2020, 5:39 p.m. UTC | #1

Hi Taylor/Peff,

On Wed, 11 Nov 2020 at 20:43, Taylor Blau <[e-mail]> 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

Patch

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 4077e731e8..cea3bb88bf 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -138,8 +138,9 @@  static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
 static int load_bitmap_header(struct bitmap_index *index)
 {
 	struct bitmap_disk_header *header = (void *)index->map;
+	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)");
 
 	if (memcmp(header->magic, BITMAP_IDX_SIGNATURE, sizeof(BITMAP_IDX_SIGNATURE)) != 0)
@@ -164,7 +165,7 @@  static int load_bitmap_header(struct bitmap_index *index)
 	}
 
 	index->entry_count = ntohl(header->entry_count);
-	index->map_pos += sizeof(*header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz;
+	index->map_pos += header_size;
 	return 0;
 }