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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 24, 2021, 06:00 UTC
Message-ID: <CAPig+cRMYiCvq7=ESSk7+PM50p4PeoKFat+w9svXMxsCHA6HEw@mail.gmail.com>
In-Reply-To: <xmqqy25e48u5.fsf@gitster.g>

On Tue, Nov 23, 2021 at 8:54 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> 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.