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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 20, 2026 at 09:41:03PM -0700, Elijah Newren wrote:
> On Wed, Aug 19, 2026 at 11:01 PM Patrick Steinhardt <ps@pks.im> 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 <ps@pks.im> 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.

I agree. For all the pain AI is causing, doing reviews is one of the
things where it's helping me a ton. Both by reviewing my own patch
series before I send them out, and by reviewing others.

Thanks!

Patrick