Re: [PATCH 13/20] packed_object_info(): convert to new revindex API

On Fri, Jan 08, 2021 at 01:17:34PM -0500, Taylor Blau wrote:

Convert another call of 'find_pack_revindex()' to its replacement
'pack_pos_to_offset()'. Likewise:

  - Avoid manipulating `struct packed_git`'s `revindex` pointer directly
    by removing the pointer-as-array indexing.

Good.

  - Add an additional guard to check that the offset 'obj_offset()'
    points to a real object. This should be the case with well-behaved
    callers to 'packed_object_info()', but isn't guarenteed.

    Other blocks that fill in various other values from the 'struct
    object_info' request handle bad inputs by setting the type to
    'OBJ_BAD' and jumping to 'out'. Do the same when given a bad offset
    here.

Also good. I wonder if we need to call error() here, too. The caller
will probably say something like "bad object" or whatever, but the user
will have no clue that it's related to the revindex.

That would match other parts of the function (e.g., calling into
unpack_entry() can generate lots of descriptive errors about exactly
what went wrong).

    The previous code would have segfaulted when given a bad
    'obj_offset' value, since 'find_pack_revindex()' would return
    'NULL', and then the line that fills 'oi->disk_sizep' would try to
    access 'NULL[1]' with a stride of 16 bytes (the width of 'struct
    revindex_entry)'.

Yep. Again, I'm really happy to see these "should never happen" cases
converted to real errors or even BUG()s.

-Peff