Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible

[Jeff King <peff@peff.net>]

On Fri, Mar 24, 2023 at 12:43:46PM -0700, Junio C Hamano wrote:

> > But given that there is only one spot that calls these, that kind of
> > refactoring might not be worth it (right now it just uses the magic
> > number "6" right before grabbing the data).
> 
> Yeah, it seems most of the callers with SEEK_SET are "I find the
> next offset from a table and jump there in preparation for doing
> something".  I suspect callers with SEEK_CUR would fit in the
> read_X() pattern better?  From that angle, it smells that the two
> kinds of seek functions may want to be split into two different
> helpers.

Yes, I think the SEEK_SET cases really do need to be doing more
checking. AFAICT they are blindly trusting the offsets in the file
(which is locally generated, so it's more of a corruption problem than a
security one, but still). And this series improves that, which is good
(but I still think it should be a die() and not a BUG()).

The SEEK_CUR cases in theory could all look like the nice read_be32() I
showed earlier, but I think in practice there are a lot of variants
(skipping read of index_pos, advancing past size given by
ewah_read_mmap(), and so on). And the current code, while ugly, does
give more specific error messages (e.g., telling on _which_ commit we
found the truncated data). So I dunno.

Certainly there could be more consistency in the magic numbers. E.g., in
this code:

                if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {
                        error(_("corrupt ewah bitmap: truncated header for bitmap of commit \"%s\""),
                                oid_to_hex(&xor_item->oid));
                        goto corrupt;
                }

                bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);
                xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);
                bitmap = read_bitmap_1(bitmap_git);

There is an assumption that sizeof(uint32_t) + sizeof(uint8_t) is equal
to bitmap_header_size - 1. That's not wrong, but it's hard to verify
that it's doing the right thing, and it's potentially fragile to changes
(though such changes seem unlikely).

-Peff