Eric Sunshine [off-list ref] 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.
Very true. Relying on the "if argv is null, point it at args.v"
assignment at the very beginning of the start_command() function is
safe because by that time the reallocations have happened already
if needed.
The pattern with or without NULLing is
initialize cp
push to cp.args
use cp
/* 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.
Thanks for spotting this. Has this patch ever been tested with
sanitizer? Do we have gap in test coverage?
On Tue, Nov 23, 2021 at 8:54 PM Junio C Hamano [off-list ref] wrote:
Eric Sunshine [off-list ref] writes:
quoted
... 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.
On Wed, Nov 24, 2021 at 1:00 AM Eric Sunshine [off-list ref] wrote:
On Tue, Nov 23, 2021 at 8:54 PM Junio C Hamano [off-list ref] wrote:
quoted
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.
As a follow-up, I just applied this patch alone and ran the tests, and
they do indeed fail as expected (on my macOS). In
t2400-worktree-add.sh, alone, 44 out of 71 tests failed, thus I don't
think there's a gap in test coverage. So, the most likely explanation
of how this problem slipped through is that Ævar only tested after
applying the full series, in which case the dangling pointer bug would
be gone, rather than testing after each patch.