On Fri, Mar 12, 2021 at 02:22:08AM -0800, Junio C Hamano wrote:
>
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
>
> > @@ -3070,6 +3071,8 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
> > unsigned flags)
> > {
> > int ret;
> > + struct run_hooks_opt hook_opt;
> > + run_hooks_opt_init_async(&hook_opt);
> >
>
> Nit. blank line between the last of decls and the first stmt (many
> identical nits exist everywhere in this series).
>
> > /*
> > * TODO trace2: replace "the_repository" with the actual repo instance
> > @@ -3088,9 +3091,13 @@ static int do_write_locked_index(s
> > else
> > ret = close_lock_file_gently(lock);
> >
> > - run_hook_le(NULL, "post-index-change",
> > - istate->updated_workdir ? "1" : "0",
> > - istate->updated_skipworktree ? "1" : "0", NULL);
> > + strvec_pushl(&hook_opt.args,
> > + istate->updated_workdir ? "1" : "0",
> > + istate->updated_skipworktree ? "1" : "0",
> > + NULL);
> > + run_hooks("post-index-change", &hook_opt);
> > + run_hooks_opt_clear(&hook_opt);
>
> There is one early return before the precontext of this hunk that
> bypasses this opt_clear() call. It is before any member of hook_opt
> structure that was opt_init()'ed gets touched, so with the current
> code, there is no leak, but it probably is laying a landmine for the
> future, where opt_init() may allocate some resource to its member,
> with the expectation that all users of the API would call
> opt_clear() to release. Or the caller of the API (like this one) may
> start mucking with the opt structure before the existing early return,
> at which point the current assumption that it is safe to return from
> that point without opt_clear() would be broken.
>
> I saw that there are other early returns in the series that are safe
> right now but may become unsafe when the API implementation gets
> extended that way. If it does not involve too much code churning,
> we may want to restructure the code to make these early returns into
> "goto"s that jump to a single exit point, so that we can always
> match opt_init() with opt_clear(), like the structure of the
> existing code allowed cmd_rebase() to use the hooks API cleanly in
> [v8 22/37].
OK. I'll audit this second half of the series looking for this type of
thing and try to clean up/use gotos if appropriate/etc. Thanks for
pointing it out.
- Emily
