Re: [PATCH v5 5/6] transport: add client support for object-info

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 8, 2022, 09:16 UTC
Message-ID: <00a58dbb-32e8-98dd-6c72-235e9c33af3b@gmail.com>
In-Reply-To: <20220728230210.2952731-6-calvinwan@google.com>

Hi Calvin

On 29/07/2022 00:02, Calvin Wan wrote:

Show 14 quoted lines

> diff --git a/transport.c b/transport.c
> index 52db7a3cb0..2d503e2fbd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -363,10 +437,12 @@ static int fetch_refs_via_pack(struct transport *transport,
>   			       int nr_heads, struct ref **to_fetch)
>   {
>   	int ret = 0;
> +	size_t i;
>   	struct git_transport_data *data = transport->data;
>   	struct ref *refs = NULL;
>   	struct fetch_pack_args args;
>   	struct ref *refs_tmp = NULL;
> +	struct ref *object_info_refs = xcalloc(1, sizeof (struct ref));

When git is compiled with SANITIZE=address then one of the cat-file tests added in patch 6 fails with an out of bounds access. The problem is that the last member of struct ref is a flexible array that is expected to contain a NUL terminated string but we don't allocate any memory for the string here. We could just add one to the size of the allocation but as there is a dedicated allocation function it would be better to use alloc_ref("").

I think it would also be worth delaying this allocation until we're sure it is going to be needed.

Show 15 quoted lines

>   	memset(&args, 0, sizeof(args));
>   	args.uploadpack = data->options.uploadpack;
> @@ -392,8 +468,25 @@ static int fetch_refs_via_pack(struct transport *transport,
>   	args.server_options = transport->server_options;
>   	args.negotiation_tips = data->options.negotiation_tips;
>   	args.reject_shallow_remote = transport->smart_options->reject_shallow;
> -
> -	if (!data->got_remote_heads) {
> +	args.object_info = transport->smart_options->object_info;
> +
> +	if (transport->smart_options && transport->smart_options->object_info) {
> +		struct ref *ref = object_info_refs;
> +
> +		if (!fetch_object_info(transport, data->options.object_info_data))
> +			goto cleanup;

If we allocate object_info_refs and initialize ref here then we avoid allocating it when it is not needed.

Show 5 quoted lines

> +		args.object_info_data = data->options.object_info_data;
> +		args.quiet = 1;
> +		args.no_progress = 1;
> +		for (i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
> +			struct ref *temp_ref = xcalloc(1, sizeof (struct ref));

This needs to use alloc_ref("") as well

> +			temp_ref->old_oid = *(transport->smart_options->object_info_oids->oid + i);

This would be clearer as ...oids->oid[i] I think

Best Wishes

Phillip