From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 62E32430408 for ; Wed, 12 Aug 2020 00:24:16 +0300 (MSK) References: <53095890-c0b6-6eae-b3b1-9898e7ca05e3@tarantool.org> From: Vladislav Shpilevoy Message-ID: <8d39159c-31d3-1394-fb04-4793da531772@tarantool.org> Date: Tue, 11 Aug 2020 23:24:14 +0200 MIME-Version: 1.0 In-Reply-To: <53095890-c0b6-6eae-b3b1-9898e7ca05e3@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 1/2] tuple: fix multikey field JSON access crash List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , tarantool-patches@dev.tarantool.org, korablev@tarantool.org Hi! Thanks for the review! > 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. You would be right if not the fact that there is always a guarantee, that if offset_slot_hint != NULL, then either multikey_idx != MULTIKEY_NONE or it is not a multikey part. It is unreachable. So it wouldn't be correct to put it 2 lines above, nor it wouldn't be incorrect - it does not change anything. But it is possible to put it *instead*. Into 'else' branch. Then it will be -1 condition check. New patch for this file: ==================== diff --git a/src/box/tuple.h b/src/box/tuple.h index 4752323e4..09ebeecf3 100644 --- a/src/box/tuple.h +++ b/src/box/tuple.h @@ -626,8 +626,28 @@ tuple_field_raw_by_path(struct tuple_format *format, const char *tuple, offset_slot = field->offset_slot; if (offset_slot == TUPLE_OFFSET_SLOT_NIL) goto parse; - if (offset_slot_hint != NULL) + if (offset_slot_hint != NULL) { *offset_slot_hint = offset_slot; + /* + * Hint is never requested for a multikey field without + * providing a concrete multikey index. + */ + assert(!field->is_multikey_part || + (multikey_idx != MULTIKEY_NONE && + field->is_multikey_part)); + } else if (field->is_multikey_part && + multikey_idx == MULTIKEY_NONE) { + /* + * 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. It is + * not known when the field is accessed not in an index. + * For example, in an application's Lua code by a JSON + * path. + */ + goto parse; + } offset_slot_access: /* Indexed field */ offset = field_map_get_offset(field_map, offset_slot,