From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alyapunov@tarantool.org>
Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 12205430407
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 11 Aug 2020 12:44:02 +0300 (MSK)
References: <cover.1596584571.git.v.shpilevoy@tarantool.org>
 <f1b60f4011e19aa04c0ff7d49c82e46955a968fc.1596584571.git.v.shpilevoy@tarantool.org>
From: Aleksandr Lyapunov <alyapunov@tarantool.org>
Message-ID: <53095890-c0b6-6eae-b3b1-9898e7ca05e3@tarantool.org>
Date: Tue, 11 Aug 2020 12:44:01 +0300
MIME-Version: 1.0
In-Reply-To: <f1b60f4011e19aa04c0ff7d49c82e46955a968fc.1596584571.git.v.shpilevoy@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Subject: Re: [Tarantool-patches] [PATCH 1/2] tuple: fix multikey field JSON
	access crash
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@dev.tarantool.org, korablev@tarantool.org

Hi! thanks again for the patch. see one comment below.

On 8/5/20 2:45 AM, Vladislav Shpilevoy wrote:
>   			goto parse;
>   		if (offset_slot_hint != NULL)
>   			*offset_slot_hint = offset_slot;
> +		/*
> +		 * When the field is multikey, the offset slot points not at the
> +		 * data. It points at 'extra' array of offsets for this multikey
> +		 * index. That array can only be accessed if index in that array
> +		 * is known.
> +		 */
> +		if (field->is_multikey_part && multikey_idx == MULTIKEY_NONE)
> +			goto parse;
>   offset_slot_access:
>   		/* Indexed field */
>   		offset = field_map_get_offset(field_map, offset_slot,
I'm sure that your check must be moved for two lines up. I mean the check
must be done before setting *offset_slot_hint.

As I understood offset_slot_hint will contain a hint for further 
tuple_field_raw_by_path
calls with the same path. That is a kind of agreement, we may call 
tuple_field_raw_by_path
twice and must get the same results.

But in your code you set *offset_slot_hint before a check that could go 
to 'parse' label.
Meanwhile in the second call of tuple_field_raw_by_path it'll check 
*offset_slot_hint and
will go to 'offset_slot_access' label. That's wrong.