git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Aug 15, 2022, 22:53 UTC
Message-ID
<YvrOWY2x19G1jCTK@coredump.intra.peff.net>
In-Reply-To
<xmqqmtc63wdf.fsf@gitster.g>
On Sun, Aug 14, 2022 at 10:32:12PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> > 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.

Show 21 quoted lines
> > 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
Previous: Junio C Hamano
Message 5 of 5 in “[BUG] git rev-list --missing=allow-promisor”
  1. Andrew OlsenAug 10, 2022
  2. Jeff KingAug 11, 2022
  3. is_promisor_object(): fix use-after-free of tree bufferJeff King, Aug 14, 2022
  4. Junio C HamanoAug 15, 2022
  5. Jeff KingAug 15, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.