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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2023, 21:38 UTC
Message-ID: <xmqqcz4xesom.fsf@gitster.g>
In-Reply-To: <20230324203737.GA549549@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> 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()).

Yes, I think by mistake I merged the topic way too early than it has been discussed sufficiently. I haven't reverted the merge into 'next' but it may not be a bad idea if the concensus is that the seek-like whence interface is too ugly to live. BUG() that triggers on data errors should be updated to die(), whether we do it as a follow-on patch or with a replacement iteration.

Show 17 quoted lines

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

Yeah, that was the part of the code I was looking at when I wrote the message you are responding to X-<.

Thanks.