Re: [PATCH] is_promisor_object(): fix use-after-free of tree buffer

[Jeff King <peff@peff.net>]

On Sun, Aug 14, 2022 at 10:32:12PM -0700, Junio C Hamano wrote:

> > We're in the middle of walking through the entries of a tree object via
> > process_tree_contents(). We see a blob (or it could even be another tree
> > entry) that we don't have, so we call is_promisor_object() to check it.
> > That function loops over all of the objects in the promisor packfile,
> > including the tree we're currently walking.
> 
> I forgot that the above "loops over" happens only once to populate
> the oidset hashtable, and briefly wondered if we are being grossly
> inefficient by scanning pack .idx file each time we encounter a
> missing object.  "Upon first call, that function loops over
> ... walking, to prepare a hashtable to answer if any object id is
> referred to by an object in promisor packs" would have helped ;-).

Right. When you have worked in an area, sometimes it is easy to forget
which things are common knowledge and which are not. :) I don't mind at
all if you want to amend the commit message as you apply.

> > It may also be a good direction for this function in general, as there
> > are other possible optimizations that rely on doing some analysis before
> > parsing:
> >
> >   - we could detect blobs and avoid reading their contents; they can't
> >     link to other objects, but parse_object() doesn't know that we don't
> >     care about checking their hashes.
> >
> >   - we could avoid allocating object structs entirely for most objects
> >     (since we really only need them in the oidset), which would save
> >     some memory.
> >
> >   - promisor commits could use the commit-graph rather than loading the
> >     object from disk
> >
> > This commit doesn't do any of those optimizations, but I think it argues
> > that this direction is reasonable, rather than relying on parse_object()
> > and trying to teach it to give us more information about whether it
> > parsed.
> 
> Yeah, all of the future bits sound sensible. 

I very intentionally didn't work on those things yet, because I wanted
to make sure we got a simple fix in as quickly as possible. That said, I
don't have immediate plans for them. They are perhaps not quite small
enough for #leftoverbits, but I think they might also be nice bite-sized
chunks for somebody wanting to get their feet wet in that part of the
code.

-Peff