Re: [PATCH] fetch-pack: fix segfault when fscking without --lock-pack

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Before that commit, if pack_lockfiles was NULL, we wouldn't bother
> reading the output from index-pack at all. But since that commit,
> index-pack may produce extra output if we asked it to fsck. So even if
> nobody cares about the lockfile path, we still need to read it to skip
> to the output we do care about.

True.  Looking at that problematic commit, I wonder how it passed
the review process.  As a design, adding a list of bare object IDs
without marking what they are for is way too inextensible by our
standard practice.

It is probably not too late to fix it, as this is purely an internal
implementation detail between fetch-pack and index-pack that is not
even documented ("git index-pack --help" does talk about the
"(pack|keep)\t<pack-name>" output, but never the output after that).

> We correctly check that we didn't get a NULL lockfile path (which can
> happen if we did not ask it to create a .keep file at all), but we
> missed the case where the lockfile path is not NULL (due to "-k -k") but
> the pack_lockfiles string_list is NULL (because nobody passed
> "--lock-pack"), and segfault trying to add to the NULL string-list.
>
> We can fix this by skipping the append to the string list when either
> the value or the list is NULL. In that case we must also free the
> lockfile path to avoid leaking it when it's non-NULL.

OK.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index eba9e420ea..42f48fbc31 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1038,8 +1038,10 @@ static int get_pack(struct fetch_pack_args *args,
>  
>  		if (!is_well_formed)
>  			die(_("fetch-pack: invalid index-pack output"));
> -		if (pack_lockfile)
> +		if (pack_lockfiles && pack_lockfile)
>  			string_list_append_nodup(pack_lockfiles, pack_lockfile);
> +		else
> +			free(pack_lockfile);
>  		parse_gitmodules_oids(cmd.out, gitmodules_oids);
>  		close(cmd.out);
>  	}

That looks like a very safe thing to do.

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index b26f367620..585ea0ee16 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -993,6 +993,16 @@ test_expect_success 'ensure bogus fetch.negotiationAlgorithm yields error' '
>  		       fetch origin server_has both_have_2
>  '
>  
> +test_expect_success 'fetch-pack with fsckObjects and keep-file does not segfault' '
> +	rm -rf server client &&
> +	test_create_repo server &&
> +	test_commit -C server one &&
> +
> +	test_create_repo client &&
> +	git -c fetch.fsckObjects=true \
> +	    -C client fetch-pack -k -k ../server HEAD
> +'
> +

And the test is quite straight-forward.

Will queue.  Thanks.