[tarantool-patches] [PATCH] sql: update ptr to VDBE after its creation in sql_txn

[tarantool-patches] [PATCH] sql: update ptr to VDBE after its creation in sql_txn

[Nikita Pettik]

VDBE object is used in struct sql_txn to add new autoincrement ids in
sequence_next(). List of these ids is returned later as a query
execution result. sql_txn is created once SQL statement is executed
inside transaction and exists till commit or rollback. After its
creation it contains pointer to current VDBE. Each VDBE is freed after
statement is executed. Hence, after first SQL statement within
transaction is executed, sql_txn will point to freed memory (dangling
pointer). This leads to crash in the next processed statement. Fix to
this bug is simple: we must re-assign pointer to VDBE in sql_txn before
VDBE execution.

Closes #4157
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-4157-fix-autoincrement-in-transaction
Issue: https://github.com/tarantool/tarantool/issues/4157

 src/box/sql/vdbeaux.c                     |  1 +
 test/sql/transitive-transactions.result   | 28 ++++++++++++++++++++++++++++
 test/sql/transitive-transactions.test.lua | 15 +++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 0cc3c1487..1a9762d5e 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -110,6 +110,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
 			if (txn->psql_txn == NULL)
 				return -1;
 		}
+		txn->psql_txn->vdbe = vdbe;
 	}
 	return 0;
 }
diff --git a/test/sql/transitive-transactions.result b/test/sql/transitive-transactions.result
index 883cc00f6..ee9b4218d 100644
--- a/test/sql/transitive-transactions.result
+++ b/test/sql/transitive-transactions.result
@@ -134,3 +134,31 @@ box.execute('DROP TABLE parent;');
 ---
 - row_count: 1
 ...
+-- gh-4157: autoincrement within transaction started in SQL
+-- leads to seagfault.
+--
+box.execute('CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT);');
+---
+- row_count: 1
+...
+box.execute('START TRANSACTION')
+box.execute('INSERT INTO t VALUES (null), (null);')
+box.execute('INSERT INTO t VALUES (null), (null);')
+box.execute('SAVEPOINT sp;')
+box.execute('INSERT INTO t VALUES (null);')
+box.execute('ROLLBACK TO sp;')
+box.execute('INSERT INTO t VALUES (null);')
+box.commit();
+---
+...
+box.space.T:select();
+---
+- - [1]
+  - [2]
+  - [3]
+  - [4]
+  - [6]
+...
+box.space.T:drop();
+---
+...
diff --git a/test/sql/transitive-transactions.test.lua b/test/sql/transitive-transactions.test.lua
index 97b06e592..54f12739e 100644
--- a/test/sql/transitive-transactions.test.lua
+++ b/test/sql/transitive-transactions.test.lua
@@ -67,3 +67,18 @@ box.execute('PRAGMA defer_foreign_keys = 0;')
 -- Cleanup
 box.execute('DROP TABLE child;');
 box.execute('DROP TABLE parent;');
+
+-- gh-4157: autoincrement within transaction started in SQL
+-- leads to seagfault.
+--
+box.execute('CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT);');
+box.execute('START TRANSACTION')
+box.execute('INSERT INTO t VALUES (null), (null);')
+box.execute('INSERT INTO t VALUES (null), (null);')
+box.execute('SAVEPOINT sp;')
+box.execute('INSERT INTO t VALUES (null);')
+box.execute('ROLLBACK TO sp;')
+box.execute('INSERT INTO t VALUES (null);')
+box.commit();
+box.space.T:select();
+box.space.T:drop();
-- 
2.15.1

[tarantool-patches] Re: [PATCH] sql: update ptr to VDBE after its creation in sql_txn

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/test/sql/transitive-transactions.result b/test/sql/transitive-transactions.result
> index 883cc00f6..ee9b4218d 100644
> --- a/test/sql/transitive-transactions.result
> +++ b/test/sql/transitive-transactions.result
> @@ -134,3 +134,31 @@ box.execute('DROP TABLE parent;');
>  ---
>  - row_count: 1
>  ...
> +-- gh-4157: autoincrement within transaction started in SQL
> +-- leads to seagfault.
> +--
> +box.execute('CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT);');
> +---
> +- row_count: 1
> +...
> +box.execute('START TRANSACTION')
> +box.execute('INSERT INTO t VALUES (null), (null);')
> +box.execute('INSERT INTO t VALUES (null), (null);')
> +box.execute('SAVEPOINT sp;')
> +box.execute('INSERT INTO t VALUES (null);')
> +box.execute('ROLLBACK TO sp;')
> +box.execute('INSERT INTO t VALUES (null);')
> +box.commit();
> +---
> +...
> +box.space.T:select();
> +---
> +- - [1]
> +  - [2]
> +  - [3]
> +  - [4]
> +  - [6]
> +...
> +box.space.T:drop();

Why so huge test? I see that you have 4-line test in
the issue. Maybe it would be simpler to use it? In the
test above savepoints confuse me.

> +---
> +...

Just in case somebody doubts that Vdbe in txn was
necessary and even forced by Kostja (who of course
has already forgot it and proposes contradictory
things again), I paste here Kostja's answer on the
original patchset:

"""
    We're going to do a lot of integration of box and vdbe - triggers,
    constraints, etc, so I think this patch is on track from architecture
    point of view as well, we will need to access vdbe from box increasingly
    more as we progress along with the integration.
"""

[tarantool-patches] Re: [PATCH] sql: update ptr to VDBE after its creation in sql_txn

[n.pettik]

>> diff --git a/test/sql/transitive-transactions.result b/test/sql/transitive-transactions.result
>> index 883cc00f6..ee9b4218d 100644
>> --- a/test/sql/transitive-transactions.result
>> +++ b/test/sql/transitive-transactions.result
>> @@ -134,3 +134,31 @@ box.execute('DROP TABLE parent;');
>> ---
>> - row_count: 1
>> ...
>> +-- gh-4157: autoincrement within transaction started in SQL
>> +-- leads to seagfault.
>> +--
>> +box.execute('CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT);');
>> +---
>> +- row_count: 1
>> +...
>> +box.execute('START TRANSACTION')
>> +box.execute('INSERT INTO t VALUES (null), (null);')
>> +box.execute('INSERT INTO t VALUES (null), (null);')
>> +box.execute('SAVEPOINT sp;')
>> +box.execute('INSERT INTO t VALUES (null);')
>> +box.execute('ROLLBACK TO sp;')
>> +box.execute('INSERT INTO t VALUES (null);')
>> +box.commit();
>> +---
>> +...
>> +box.space.T:select();
>> +---
>> +- - [1]
>> +  - [2]
>> +  - [3]
>> +  - [4]
>> +  - [6]
>> +...
>> +box.space.T:drop();
> 
> Why so huge test? I see that you have 4-line test in
> the issue. Maybe it would be simpler to use it? In the
> test above savepoints confuse me.

As test cases shows - we don’t have tests involving insertions
to auto-increment field within transaction. So, I just wanted to
make sure that savepoints are working as well. If you wan’t,
I can make this test be 2-lines-like. 

> 
>> +---
>> +...
> 
> Just in case somebody doubts that Vdbe in txn was
> necessary and even forced by Kostja (who of course
> has already forgot it and proposes contradictory
> things again), I paste here Kostja's answer on the
> original patchset:
> 
> """
>    We're going to do a lot of integration of box and vdbe - triggers,
>    constraints, etc, so I think this patch is on track from architecture
>    point of view as well, we will need to access vdbe from box increasingly
>    more as we progress along with the integration.
> """
> 

[tarantool-patches] Re: [PATCH] sql: update ptr to VDBE after its creation in sql_txn

[Vladislav Shpilevoy]

On 18/04/2019 22:20, n.pettik wrote:
> 
>>> diff --git a/test/sql/transitive-transactions.result b/test/sql/transitive-transactions.result
>>> index 883cc00f6..ee9b4218d 100644
>>> --- a/test/sql/transitive-transactions.result
>>> +++ b/test/sql/transitive-transactions.result
>>> @@ -134,3 +134,31 @@ box.execute('DROP TABLE parent;');
>>> ---
>>> - row_count: 1
>>> ...
>>> +-- gh-4157: autoincrement within transaction started in SQL
>>> +-- leads to seagfault.
>>> +--
>>> +box.execute('CREATE TABLE t (id INT PRIMARY KEY AUTOINCREMENT);');
>>> +---
>>> +- row_count: 1
>>> +...
>>> +box.execute('START TRANSACTION')
>>> +box.execute('INSERT INTO t VALUES (null), (null);')
>>> +box.execute('INSERT INTO t VALUES (null), (null);')
>>> +box.execute('SAVEPOINT sp;')
>>> +box.execute('INSERT INTO t VALUES (null);')
>>> +box.execute('ROLLBACK TO sp;')
>>> +box.execute('INSERT INTO t VALUES (null);')
>>> +box.commit();
>>> +---
>>> +...
>>> +box.space.T:select();
>>> +---
>>> +- - [1]
>>> +  - [2]
>>> +  - [3]
>>> +  - [4]
>>> +  - [6]
>>> +...
>>> +box.space.T:drop();
>>
>> Why so huge test? I see that you have 4-line test in
>> the issue. Maybe it would be simpler to use it? In the
>> test above savepoints confuse me.
> 
> As test cases shows - we don’t have tests involving insertions
> to auto-increment field within transaction. So, I just wanted to
> make sure that savepoints are working as well. If you wan’t,
> I can make this test be 2-lines-like. 

If this test works as well, it is up to you.

LGTM anyway.

[tarantool-patches] Re: [PATCH] sql: update ptr to VDBE after its creation in sql_txn

[Kirill Yukhin]

Hello,

On 16 Apr 02:25, Nikita Pettik wrote:
> VDBE object is used in struct sql_txn to add new autoincrement ids in
> sequence_next(). List of these ids is returned later as a query
> execution result. sql_txn is created once SQL statement is executed
> inside transaction and exists till commit or rollback. After its
> creation it contains pointer to current VDBE. Each VDBE is freed after
> statement is executed. Hence, after first SQL statement within
> transaction is executed, sql_txn will point to freed memory (dangling
> pointer). This leads to crash in the next processed statement. Fix to
> this bug is simple: we must re-assign pointer to VDBE in sql_txn before
> VDBE execution.
> 
> Closes #4157
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-4157-fix-autoincrement-in-transaction
> Issue: https://github.com/tarantool/tarantool/issues/4157

I've checked your patch into 2.1 and master branches.

--
Regards, Kirill Yukhin