Re: [PATCH GSoC v4 2/9] fetch-object-info: detect malformed server responses
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 4, 2026, 20:40 UTC
- Message-ID
- <xmqqpkzxtyac.fsf@gitster.g>
- In-Reply-To
- <20260804-objecttype-support-v4-2-31511b0231be@gmail.com>
Pablo Sabater <pabloosabaterr@gmail.com> writes:
Show 15 quoted lines
> The loop reading the object-info response stops as soon as the reader > returns something other than PACKET_READ_NORMAL, or once it has read as > many lines as we requested. Neither end is checked. > > A server that answers with fewer objects leaves the end of the result > arrays empty, and the caller trusts that every requested object was > filled in. A server that answers with more leaves the extra packets > unread. On stateless transports check_stateless_delimiter() notices, but > on the others it passes unnoticed. > > Check both limits by extracting the packet_reader_read() from the loop > condition, so the loop no longer consumes the last packet (flush). If > while looping the read is different from a PACKET_READ_NORMAL, die() > meaning there are fewer objects than expected. After iterating, we only > expect a flush, so if the last packet is not a flush, die().
OK.
After looking at "if (i != oids->nr)" now gone from the end of the function, but before looking at how the loop terminates its iterations, I wondered how the "there are too few" detection went. It now not just stops iterating when seeing a status other than PACKET_READ_NORMAL, but it actively barfs by dying, to detect a short read. So the only thing we need to check after the loop is if we are truly at the end of the "list of oids->nr things".
Makes sense.
Show 42 quoted lines
> Helped-by: Junio C Hamano <gitster@pobox.com>
> 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 | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> index ba7e179c44..c2d4bf9403 100644
> --- a/fetch-object-info.c
> +++ b/fetch-object-info.c
> @@ -106,12 +106,13 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
> }
> }
>
> - for (size_t i = 0;
> - packet_reader_read(reader) == PACKET_READ_NORMAL &&
> - i < args->oids->nr;
> - i++) {
> + for (size_t i = 0; i < args->oids->nr; i++) {
> struct string_list object_info_values = STRING_LIST_INIT_DUP;
>
> + if (packet_reader_read(reader) != PACKET_READ_NORMAL)
> + die(_("object-info: expected %" PRIuMAX " objects, got %" PRIuMAX),
> + (uintmax_t)args->oids->nr, (uintmax_t)i);
> +
> string_list_split(&object_info_values, reader->line, " ", -1);
>
> if (strcmp(object_info_values.items[0].string,
> @@ -150,6 +151,11 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
>
> string_list_clear(&object_info_values, 0);
> }
> +
> + if (packet_reader_read(reader) != PACKET_READ_FLUSH)
> + die(_("object-info: expected flush after %"PRIuMAX" objects"),
> + (uintmax_t)args->oids->nr);
> +
> check_stateless_delimiter(stateless_rpc, reader, "stateless delimiter expected");
>
> return 0;