Re: [PATCH GSoC v17 10/13] transport: add client support for object-info
- From
Pablo Sabater <pabloosabaterr@gmail.com>
- Date
- Jul 14, 2026, 19:33 UTC
- 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:
Show 17 quoted lines
> 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.
Show 29 quoted lines
>
>> + 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.