Re: [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Nov 23, 2021 at 8:54 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > ... At best, following this change,
> > git-worktree is only working "by accident" if the underlying
> > child_process::args.v doesn't get reallocated between run_command()
> > invocations. Relying upon this "by accident" behavior feels rather
> > unsafe.
>
> The pattern with or without NULLing is
>         /* cp.argv = NULL */
>         strvec_clear(&cp.args);
>         push to cp.args
>
> and strvec_clear() frees the underying array, and the first push
> will reallocates from NULL, so there is no guarantee that cp.argv
> in the first use that used to be pointing at cp.args that has
> already been freed is still valid.

Indeed, so this is even worse than I thought. I was somewhat pressed
for time when I wrote the review, thus didn't look at the
implementation of strvec_clear(), and incorrectly thought that it only
reset its `nr` member to 0 but kept the array allocated (much like
strbuf_reset()). That's why I thought it was only working "by
accident". But, as you point out, strvec_clear() does free its
allocated array (much like strbuf_release()), so -- with this patch
applied -- each subsequent run_command() invocation is _definitely_
accessing the dangling pointer in child_process::argv, and that
dangling pointer would (in the "best" case) be referencing the
original populated value of child_process::args, not the repopulated
value. So, even if it didn't crash outright, it would just re-run the
same command three times (unless by chance it reallocated the same
memory it had freed earlier.)

> Thanks for spotting this.  Has this patch ever been tested with
> sanitizer?  Do we have gap in test coverage?

The question about potential gap in test coverage is a good one.
Maybe, by chance it reallocated the same memory that it had earlier
freed, thus did indeed work "by accident". Another possibility is that
Ævar only ran the tests after applying the full patch series, in which
case this dangling-pointer bug would be gone, rather than running the
tests after each patch.