Jeff King <peff@xxxxxxxx> writes:
> 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.
> 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.
