From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Tue, 14 Jul 2026 19:33:25 GMT
Subject: Re: [PATCH GSoC v17 10/13] transport: add client support for object-info
Message-ID: <DJYJN18OMC4G.2YYRORCZN519Y@gmail.com>
In-Reply-To: <xmqqik6htpv4.fsf@gitster.g>

On Tue Jul 14, 2026 at 7:58 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> +	for (size_t i = 0; packet_reader_read(reader) == PACKET_READ_NORMAL && i < args->oids->nr; i++) {
>
> An overly long line.  Format it like this, perhaps?
>
> 	for (size_t i = 0;
> 	     packet_reader_read(reader) == PACKET_READ_NORMAL && i < args->oids->nr;
> 	     i++) {
>
> or even:
>
> 	for (size_t i = 0;
> 	     packet_reader_read(reader) == PACKET_READ_NORMAL &&
> 	     i < args->oids->nr;
> 	     i++) {
>

Will wrap that line, thanks.

>
>> +		struct string_list object_info_values = STRING_LIST_INIT_DUP;
>> +
>> +		string_list_split(&object_info_values, reader->line, " ", -1);
>> +		if (size_index >= 0) {
>> +			if (!strcmp(object_info_values.items[1 + size_index].string, "")) {
>> +				FREE_AND_NULL(object_info_data[i].sizep);
>> +				string_list_clear(&object_info_values, 0);
>> +				continue;
>> +			}
>> +
>> +			if (parse_object_size(object_info_values.items[1 + size_index].string,
>> +					      object_info_data[i].sizep))
>> +				die("object-info: ref %s has invalid size %s",
>> +				    object_info_values.items[0].string,
>> +				    object_info_values.items[1 + size_index].string);
>> +		}
>> +
>> +		string_list_clear(&object_info_values, 0);
>
> Is this not trusting the other side too much?
>
> If the other end returns fewer values than expected (e.g., if a
> buggy or malicious server returns only "<oid>" without a trailing
> space for an unrecognized object, or if we request multiple
> attributes in the future and the server returns fewer values than
> expected), string_list_split may return a list with fewer elements
> than size_index + 1.  Accessing object_info_values.items[size_index
> + 1] will then result in an out-of-bounds read/crash.

I will add a check for a malformed response from the server so "<oid>" is
considered corrupted, similar to the size values a few lines below.

A subsequent commit in this series (13) adds a filter that drops
attributes requested by the client but that the server doesn't support so
we should expect full return of the attributes asked or "<oid> SP".

I'll add a guard just in case in a future what we expect changes.

>
> By the way, from a stylistic standpoint, "size_index + 1" reads a
> bit more naturally than the "1 + size_index" used in the current
> patch.


Will change it.

Thanks for the feedback,
Pablo.