Re: [PATCH GSoC v4 2/9] fetch-object-info: detect malformed server responses

Pablo Sabater <[email protected]> writes:

> 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.

> Helped-by: Junio C Hamano <[email protected]>
> Mentored-by: Karthik Nayak <[email protected]>
> Mentored-by: Chandra Pratap <[email protected]>
> Signed-off-by: Pablo Sabater <[email protected]>
> ---
>  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;