Re: [PATCH GSoC 2/5] fetch-object-info: parse type from server response

From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Jul 29, 2026, 12:05 UTC
Message-ID: <DKB1II4Z88SG.38KG7RAF9Q7VW@gmail.com>
In-Reply-To: <CA+J6zkSQYuK-ZJoiQkEJDS9fBypOrBEmgYZRj1yYU00ws2u_HA@mail.gmail.com>

On Wed Jul 29, 2026 at 11:57 AM CEST, Chandra Pratap wrote:

Show 54 quoted lines

> On Sat, 25 Jul 2026 at 17:25, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
>>
>> The server can handle type requests but does not advertise the
>> capability yet. Prepare the client to know how to parse the server
>> response once the server advertises the capability.
>>
>> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
>> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
>> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
>> ---
>>  fetch-object-info.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/fetch-object-info.c b/fetch-object-info.c
>> index ba7e179c44..cf6b94afb8 100644
>> --- a/fetch-object-info.c
>> +++ b/fetch-object-info.c
>> @@ -50,6 +50,7 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>>                       const int stateless_rpc, const int fd_out)
>>  {
>>         int size_index = -1;
>> +       int type_index = -1;
>>
>>         switch (version) {
>>         case protocol_v2:
>> @@ -101,8 +102,13 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>>                         for (size_t j = 0; j < args->oids->nr; j++)
>>                                 object_info_data[j].sizep =
>>                                         xcalloc(1, sizeof(*object_info_data[j].sizep));
>> +               } else if (!strcmp(reader->line, "type")) {
>> +                       type_index = (int)i;
>> +                       for (size_t j = 0; j < args->oids->nr; j++)
>> +                               object_info_data[j].typep =
>> +                                       xcalloc(1, sizeof(*object_info_data[j].typep));
>>                 } else {
>> -                       BUG("only size is supported");
>> +                       BUG("unexpected object-info option: %s", reader->line);
>>                 }
>>         }
>>
>> @@ -148,6 +154,10 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>>                             object_info_values.items[0].string,
>>                             object_info_values.items[size_index + 1].string);
>>
>> +               if (type_index >= 0)
>> +                       *object_info_data[i].typep =
>> +                               type_from_string(object_info_values.items[type_index + 1].string);
>> +
>>                 string_list_clear(&object_info_values, 0);
>
> Is there a risk of an out-of-bounds array access here if the server
> responds with a truncated or malformed packet?
>
> If object_info_values.nr <= type_index + 1, this will segfault.

This shouldn't be a possible case because of:

fetch_object_info()

	for (size_t i = 0; i < args->object_info_options->nr; i++) {

		[snip]

		} else if (!strcmp(reader->line, "type")) {
			type_index = (int)i;

		[snip]

type_index is set based of the range of object_info_options->nr so:
  type_index < object_info_options->nr

and a few lines below:

	if (args->object_info_options->nr + 1 != object_info_values.nr)
		die("object-info: unexpected number of attributes: %s",
		    reader->line);

so we also know that type_index + 1 < object_info_values.nr. After that we get to those lines that this patch introduced:

 +               if (type_index >= 0)
 +                       *object_info_data[i].typep =
 +                               type_from_string(object_info_values.items[type_index + 1].string);

 And because type_index + 1 < object_info_values.nr we can be sure that
 this cannot segfault once we reach this code.

Show 6 quoted lines

>
> If there isn't a bounds check slightly higher up in this loop, we should
> add one. Either way, we should definitely add a test using a mocked
> server response (e.g., via test-tool pkt-line) to ensure the client
> gracefully dies with a protocol error rather than segfaulting when it
> receives a malformed packet.

Ok, that's sounds a good test, I think there's none where a malicious server is simulated, in part because I don't know how and I think I haven't seen a test that does that yet. test-tool and pkt-line are used for the opposite: simulating the client to test the real server.

I'll see what I can do about it.

Thanks for the feedback, Pablo