Re: [Tarantool-patches] [PATCH 1/2] tuple: fix multikey field JSON access crash

[Vladislav Shpilevoy <v.shpilevoy@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,