[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 1 comment below.

On 04/12/2018 20:39, Kirill Shcherbatov wrote:
> http://github.com/tarantool/tarantool/tree/kshch/gh-3772-dirty-memory-access
> https://github.com/tarantool/tarantool/issues/3772
> 
> The tarantoolSqlite3TupleColumnFast routine used to lookup
> offset_slot in unallocated memory in some cases.

In which 'some'? Yes, assert is wrong, but I do not understand
how is it possible that fieldno >= field_count if such errors
are caught during parsing stage.

> The assert with exact_field_count from 7a8de28 is not correct.
> assert(format->exact_field_count == 0 ||
>         fieldno < format->exact_field_count);
> The tarantoolSqlite3TupleColumnFast routine requires offset_slot
> that has been allocated during tuple_format_create call. This
> value is stored in indexed field with index that limited with
> index_field_count that is <= field_count.
> Look at tuple_format_alloc for more details.
> 
> Closes #3772
> ---
>   src/box/sql.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 171fd966a..25b3213fd 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -199,9 +199,8 @@ tarantoolSqlite3TupleColumnFast(BtCursor *pCur, u32 fieldno, u32 *field_size)
>   	assert(pCur->last_tuple != NULL);
>   
>   	struct tuple_format *format = tuple_format(pCur->last_tuple);
> -	assert(format->exact_field_count == 0
> -	       || fieldno < format->exact_field_count);
> -	if (format->fields[fieldno].offset_slot == TUPLE_OFFSET_SLOT_NIL)
> +	if (fieldno >= format->field_count ||
> +	    format->fields[fieldno].offset_slot == TUPLE_OFFSET_SLOT_NIL)
>   		return NULL;
>   	const char *field = tuple_field(pCur->last_tuple, fieldno);
>   	const char *end = field;
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[Kirill Shcherbatov]

> In which 'some'? Yes, assert is wrong, but I do not understand
> how is it possible that fieldno >= field_count if such errors
> are caught during parsing stage.
Please, read commit message referenced by my ticket 7a8de28.

kyukhin wrote:
"This assert is not always hold, e.g. for _schema space where max_id
  is stored behind formatted fields."
about redefined to invalid assert

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[Vladislav Shpilevoy]

On 05/12/2018 23:41, Kirill Shcherbatov wrote:
>> In which 'some'? Yes, assert is wrong, but I do not understand
>> how is it possible that fieldno >= field_count if such errors
>> are caught during parsing stage.
> Please, read commit message referenced by my ticket 7a8de28.

It should be written in this commit message. Message in 7a8de28
is wrong.

> 
> kyukhin wrote:
> "This assert is not always hold, e.g. for _schema space where max_id
>    is stored behind formatted fields."
> about redefined to invalid assert
> 

max_id has nothing to do with the problem. From SQL it is impossible to
select not named fields.

The problem is that first 4 tuples in _space: 257, 272, 276 and 280 have
an old format of _space with only one field (format->field_count == 1).
It happens because these 4 tuples are recovered not after tuple with id
280 which stores actual format of _space. After tuple 280 is recovered,
an actual format is set in struct space of _space and all next tuples
have full featured formats.

So for these 4 tuples tarantoolSqlite3TupleColumnFast can fail even if a
field exists, is indexed and has a name. Those features are just described
in a newer format.

Moreover, even on these tuples a memory lookup is not always dirty. This
request fails:

	box.sql.execute("SELECT \"name\" FROM \"_space\"")

But these do not:

	box.sql.execute("SELECT \"name\" FROM \"_space\" WHERE \"id\" < 258")
	box.sql.execute("SELECT \"name\" FROM \"_space\" WHERE \"id\" = 272")
	box.sql.execute("SELECT \"name\" FROM \"_space\" WHERE \"id\" = 276")

For an unknown reason the latters do not lookup 'name' field via
tarantoolSqlite3TupleColumnFast. Looks like a strange behaviour of
the planner. But this is another issue.

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[Kirill Shcherbatov]

Hi! Thank you for research, I've included your conclusions to
the commit message.


The tarantoolSqlite3TupleColumnFast routine used to lookup
offset_slot in unallocated memory in some cases.
The assert with exact_field_count same as motivation to change
old correct assert with field_count in 7a8de28 is not correct.
assert(format->exact_field_count == 0 ||
       fieldno < format->exact_field_count);
The tarantoolSqlite3TupleColumnFast routine requires offset_slot
that has been allocated during tuple_format_create call. This
value is stored in indexed field with index that limited with
index_field_count that is <= field_count. Look at
tuple_format_alloc for more details.

The format in cursor triggering valid assertion has such
structure because first 4 tuples in _space: 257, 272, 276 and
280 have an old format of _space with only one field
(format->field_count == 1).
It happens because these 4 tuples are recovered not after tuple
with id 280 which stores actual format of _space. After tuple
280 is recovered, an actual format is set in struct space of
_space and all next tuples have full featured formats.

So for these 4 tuples tarantoolSqlite3TupleColumnFast can fail
even if a field exists, is indexed and has a name. Those
features are just described in a newer format.
(thank Gerold103 for problem explanation)

Closes #3772

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[Vladislav Shpilevoy]

On 06/12/2018 09:59, Kirill Shcherbatov wrote:
> Hi! Thank you for research, I've included your conclusions to
> the commit message.
> 
> 
> The tarantoolSqlite3TupleColumnFast routine used to lookup
> offset_slot in unallocated memory in some cases.
> The assert with exact_field_count same as motivation to change
> old correct assert with field_count in 7a8de28 is not correct.
> assert(format->exact_field_count == 0 ||
>         fieldno < format->exact_field_count);
> The tarantoolSqlite3TupleColumnFast routine requires offset_slot
> that has been allocated during tuple_format_create call. This
> value is stored in indexed field with index that limited with
> index_field_count that is <= field_count. Look at
> tuple_format_alloc for more details.
> 
> The format in cursor triggering valid assertion has such
> structure because first 4 tuples in _space: 257, 272, 276 and
> 280 have an old format of _space with only one field
> (format->field_count == 1).
> It happens because these 4 tuples are recovered not after tuple
> with id 280 which stores actual format of _space. After tuple
> 280 is recovered, an actual format is set in struct space of
> _space and all next tuples have full featured formats.
> 
> So for these 4 tuples tarantoolSqlite3TupleColumnFast can fail
> even if a field exists, is indexed and has a name. Those
> features are just described in a newer format.
> (thank Gerold103 for problem explanation)
> 
> Closes #3772
> 

LGTM. Nikita, please, review. Especially English part.

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[n.pettik]

> On 6 Dec 2018, at 12:17, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> On 06/12/2018 09:59, Kirill Shcherbatov wrote:
>> Hi! Thank you for research, I've included your conclusions to
>> the commit message.
>> The tarantoolSqlite3TupleColumnFast routine used to lookup
>> offset_slot in unallocated memory in some cases.
>> The assert with exact_field_count same as motivation to change
>> old correct assert with field_count in 7a8de28 is not correct.
>> assert(format->exact_field_count == 0 ||
>>        fieldno < format->exact_field_count);
>> The tarantoolSqlite3TupleColumnFast routine requires offset_slot
>> that has been allocated during tuple_format_create call. This
>> value is stored in indexed field with index that limited with
>> index_field_count that is <= field_count. Look at
>> tuple_format_alloc for more details.
>> The format in cursor triggering valid assertion has such
>> structure because first 4 tuples in _space: 257, 272, 276 and
>> 280 have an old format of _space with only one field
>> (format->field_count == 1).
>> It happens because these 4 tuples are recovered not after tuple
>> with id 280 which stores actual format of _space. After tuple
>> 280 is recovered, an actual format is set in struct space of
>> _space and all next tuples have full featured formats.
>> So for these 4 tuples tarantoolSqlite3TupleColumnFast can fail
>> even if a field exists, is indexed and has a name. Those
>> features are just described in a newer format.
>> (thank Gerold103 for problem explanation)
>> Closes #3772
> 
> LGTM. Nikita, please, review. Especially English part.

LGTM as well.

[tarantool-patches] Re: [PATCH v1 1/1] sql: fix tarantoolSqlite3TupleColumnFast

[Kirill Yukhin]

Hello,
On 05 Dec 23:23, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! See 1 comment below.
> 
> On 04/12/2018 20:39, Kirill Shcherbatov wrote:
> > http://github.com/tarantool/tarantool/tree/kshch/gh-3772-dirty-memory-access
> > https://github.com/tarantool/tarantool/issues/3772

I've checked your patch into 2.1 branch.

--
Regards, Kirill Yukhin