Re: [PATCH v2 1/6] odb: introduce interface to generate packfiles

On Wed, Aug 19, 2026 at 11:01 PM Patrick Steinhardt <[email protected]> wrote:
>
> On Wed, Aug 19, 2026 at 09:56:56AM -0700, Elijah Newren wrote:
> > On Sun, Aug 16, 2026 at 10:40 PM Patrick Steinhardt <[email protected]> wrote:
> > >
> > > +static int odb_source_files_generate_pack(struct odb_source *source UNUSED,
> > > +                                         struct odb_pack_generator **out,
> > > +                                         const struct odb_generate_pack_options *opts)
> > > +{
> > > +       struct child_process cp = CHILD_PROCESS_INIT;
> > > +       struct odb_pack_generator_files *generator;
> > > +       FILE *in;
> > [...]
> > > +       cp.clean_on_exit = 1;
> > > +
> > > +       if (start_command(&cp))
> > > +               return error(_("could not spawn pack-objects"));
> > [...]
> > > +       CALLOC_ARRAY(generator, 1);
> > > +       generator->base.out = opts->pack_fd < 0 ? cp.out : -1;
> > > +       generator->base.err = opts->progress_fd < 0 ? cp.err : -1;
> > > +       generator->base.finish = odb_pack_generator_files_finish;
> > > +       generator->cp = cp;
> > > +
> > > +       *out = &generator->base;
> > > +       return 0;
> > > +}
> >
> > Does this have a use-after-scope bug lurking here, due to the
> > combination of clean_on_exit = 1 (which makes a copy of &cp for later
> > use), and the fact that cp is a function-local?  If I'm reading the
> > code right, start_command() calls mark_child_for_cleanup(), which does
> >
> >     p->process = process;  /* where process is &cp */
> >
> > and then cleanup_children() accesses various fields under p->process.
> > You do copy the necessary fields from cp to generator->cp, but
> > &generator->cp was not passed to start_command(), so p->process points
> > to the function-local cp.
>
> Oh, that's a very good catch indeed. Out of curiosity, how did you end
> up discovering this? Did you just happen to remember that we store the
> pointer out of scope or did the copy make you have a deeper look?

Neither.  Went to review the series, but I was worried I'd be missing
context from not reviewing earlier odb refactorings.  Used AI to help
orient me and give me its own findings from reviewing your patches.
(AI will sometimes spot things I miss in a review, though it'll also
miss some things I catch.)  And sometimes I iterate with AI to dig
into various areas.  Anyway, it flagged the potential problem, and I
dug in to make sure it didn't look like a hallucination before
cleaning it up and passing it on.  I'm still looking through your
other patches in this series, but should finish soon.

On a related note, one of my local patches happens to have a semantic
conflict with this series (namely 3/6), which piqued my interest.
I'll submit it soon, using your series as a base so I can submit my
patch with the conflict fixed.