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

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?

> I think the normal teardown path happens to be fine despite this
> issue: when odb_pack_generator_files_finish() calls
> finish_command(&generator->cp), it clears the child by matching pid
> (which was copied separately from p->process), so the stale pointer
> never gets dereferenced in the successful path.  But with an
> abnormal-exit, which is where clean_on_exit comes into play, then
> cleanup_children() will be called and start attempting to read
> p->process, which now points to some long-reclaimed function stack
> space.

Yeah, it's a bug waiting to happen. Will fix, thanks!

Patrick