On Fri, Nov 13, 2020 at 04:39:42PM -0500, Jeff King wrote:
> The problem is this hunk from patch 2:
>
> > + 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)");
>
> The header struct contains a field for the hash of the pack. So the
> original code as taking that full header, and adding in another
> current-algo rawsz to account for the trailer.
>
> Afterwards, we adjust header_size to swap out the MAX_RAWSZ for the
> current-algo rawsz. So header_size is a correct substitution for
> sizeof(*header) now. But we still have to add back in
> the_hash_algo->rawsz to account for the trailer. The second "+" line is
> wrong to have removed it.
Thanks for your patient explanation. This hunk should instead read:
+ 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 + the_hash_algo->rawsz)
return error("Corrupted bitmap index (missing header data)");
That error might not necessarily be right (it could say "missing header
or trailer data"), though. I'm open to if you think it should be
changed or not.
Since we didn't realize this bug at the time, the rest of the patch
message is worded correctly, I believe.
> The later line we adjust:
>
> > - index->map_pos += sizeof(*header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz;
> > + index->map_pos += header_size;
>
> is correct. It's just skipping past the header, and doesn't care about
> the trailer at all (and confusing the two is probably what led me to
> write the bug in the first place).
Right, makes sense.
> -Peff
Thanks,
Taylor
