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

From: Jeff King <peff@peff.net>
Date: Mar 24, 2023, 20:37 UTC
Message-ID: <20230324203737.GA549549@coredump.intra.peff.net>
In-Reply-To: <xmqqr0tedjf1.fsf@gitster.g>

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

Show 10 quoted lines

> > 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