Re: [PATCH GSoC v17 10/13] transport: add client support for object-info

[Junio C Hamano <gitster@pobox.com>]

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++) {


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

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.