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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> This can happen during a "rev-list --missing=allow-promisor" traversal
> in a partial clone that is missing one or more trees or blobs. The
> backtrace for the free looks like this:
>
>       #1 free_tree_buffer tree.c:147
>       #2 add_promisor_object packfile.c:2250
>       #3 for_each_object_in_pack packfile.c:2190
>       #4 for_each_packed_object packfile.c:2215
>       #5 is_promisor_object packfile.c:2272
>       #6 finish_object__ma builtin/rev-list.c:245
>       #7 finish_object builtin/rev-list.c:261
>       #8 show_object builtin/rev-list.c:274
>       #9 process_blob list-objects.c:63
>       #10 process_tree_contents list-objects.c:145
>       #11 process_tree list-objects.c:201
>       #12 traverse_trees_and_blobs list-objects.c:344
>       [...]
>
> 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 ;-). 

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

Will queue.

Thanks.