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

From: Jeff King <peff@peff.net>
Date: Jan 12, 2021, 09:11 UTC
Message-ID: <X/1n36/HtqAoKXrH@coredump.intra.peff.net>
In-Reply-To: <eab7ab1f35fa9703f56a99fa539839869fe4e54c.1610129796.git.me@ttaylorr.com>

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

Show 5 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

>     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