On Fri, Nov 13, 2020 at 04:29:54PM -0500, Taylor Blau wrote: > > which is the header plus the trailer. We want to readjust for the > > MAX_RAWSZ part of the header, so it should be: > > > > size_t header_size = sizeof(*header) - GIT_MAX_RAWSZ + the_hash_algo->rawsz; > > if (index->map_size < sizeof(*header) + the_hash_algo->rawsz) > > I'm not sure that I follow. If you apply this to the second patch in > this series, the only thing that changes is that it factors out: > > index->map_pos += ...; > > into > > size_t header_size = ...; > // ... > index->map_pos += header_size; > > What am I missing here? 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. 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). -Peff