* [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info
@ 2020-01-27 14:04 Chris Sosnin
2020-01-28 0:24 ` Vladislav Shpilevoy
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Chris Sosnin @ 2020-01-27 14:04 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
We should first check that primary key is not NULL.
Closes #4745
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4745-table-info-assertion
issue: https://github.com/tarantool/tarantool/issues/4745
src/box/sql/pragma.c | 6 +++---
test/sql/engine.cfg | 1 +
test/sql/gh-4745-table-info-assertion.result | 8 ++++++++
test/sql/gh-4745-table-info-assertion.test.lua | 5 +++++
4 files changed, 17 insertions(+), 3 deletions(-)
create mode 100644 test/sql/gh-4745-table-info-assertion.result
create mode 100644 test/sql/gh-4745-table-info-assertion.test.lua
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 636f519ef..a77bf4b16 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -112,10 +112,10 @@ sql_pragma_table_info(struct Parse *parse, const char *tbl_name)
struct Vdbe *v = sqlGetVdbe(parse);
struct field_def *field = space->def->fields;
for (uint32_t i = 0, k; i < space->def->field_count; ++i, ++field) {
- if (!sql_space_column_is_in_pk(space, i)) {
- k = 0;
- } else if (pk == NULL) {
+ if (pk == NULL) {
k = 1;
+ } else if (!sql_space_column_is_in_pk(space, i)) {
+ k = 0;
} else {
struct key_def *kdef = pk->def->key_def;
k = key_def_find_by_fieldno(kdef, i) - kdef->parts + 1;
diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg
index 1e9f08c6a..06c761166 100644
--- a/test/sql/engine.cfg
+++ b/test/sql/engine.cfg
@@ -2,6 +2,7 @@
"vinyl-opts.test.lua" : {
"vinyl": {"engine": "vinyl"}
},
+ "gh-4745-table-info-assertion.test.lua": { },
"bind.test.lua": {
"remote": {"remote": "true"},
"local": {"remote": "false"}
diff --git a/test/sql/gh-4745-table-info-assertion.result b/test/sql/gh-4745-table-info-assertion.result
new file mode 100644
index 000000000..51a75b816
--- /dev/null
+++ b/test/sql/gh-4745-table-info-assertion.result
@@ -0,0 +1,8 @@
+-- test-run result file version 2
+--
+-- The following pragma shouldn't cause
+-- assertion fault
+--
+table_info = box.execute('pragma table_info("_vinyl_deferred_delete")')
+ | ---
+ | ...
diff --git a/test/sql/gh-4745-table-info-assertion.test.lua b/test/sql/gh-4745-table-info-assertion.test.lua
new file mode 100644
index 000000000..19d2f6955
--- /dev/null
+++ b/test/sql/gh-4745-table-info-assertion.test.lua
@@ -0,0 +1,5 @@
+--
+-- The following pragma shouldn't cause
+-- assertion fault
+--
+table_info = box.execute('pragma table_info("_vinyl_deferred_delete")')
--
2.21.1 (Apple Git-122.3)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info
2020-01-27 14:04 [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info Chris Sosnin
@ 2020-01-28 0:24 ` Vladislav Shpilevoy
2020-01-30 12:04 ` Nikita Pettik
2020-02-05 22:22 ` Vladislav Shpilevoy
2 siblings, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy @ 2020-01-28 0:24 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
Hi! Thanks for the fixes!
It is not a review. I just wanted to say that I am going to
spend less time on reviewes to the end of this week due to
intensive preparations to FOSDEM. I will get back to your
patches as soon as I can, I didn't forget.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info
2020-01-27 14:04 [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info Chris Sosnin
2020-01-28 0:24 ` Vladislav Shpilevoy
@ 2020-01-30 12:04 ` Nikita Pettik
2020-01-30 12:53 ` Chris Sosnin
2020-02-05 22:22 ` Vladislav Shpilevoy
2 siblings, 1 reply; 6+ messages in thread
From: Nikita Pettik @ 2020-01-30 12:04 UTC (permalink / raw)
To: Chris Sosnin; +Cc: tarantool-patches, v.shpilevoy
On 27 Jan 17:04, Chris Sosnin wrote:
> We should first check that primary key is not NULL.
>
> Closes #4745
> ---
Patch is OK as obvious. I've come up with few nits concernings test.
> --- a/test/sql/engine.cfg
> +++ b/test/sql/engine.cfg
> @@ -2,6 +2,7 @@
> "vinyl-opts.test.lua" : {
> "vinyl": {"engine": "vinyl"}
> },
> + "gh-4745-table-info-assertion.test.lua": { },
> "bind.test.lua": {
> "remote": {"remote": "true"},
> "local": {"remote": "false"}
> diff --git a/test/sql/gh-4745-table-info-assertion.result b/test/sql/gh-4745-table-info-assertion.result
> new file mode 100644
> index 000000000..51a75b816
> --- /dev/null
> +++ b/test/sql/gh-4745-table-info-assertion.result
> @@ -0,0 +1,8 @@
> +-- test-run result file version 2
> +--
> +-- The following pragma shouldn't cause
> +-- assertion fault
> +--
> +table_info = box.execute('pragma table_info("_vinyl_deferred_delete")')
> + | ---
> + | ...
> diff --git a/test/sql/gh-4745-table-info-assertion.test.lua b/test/sql/gh-4745-table-info-assertion.test.lua
> new file mode 100644
> index 000000000..19d2f6955
> --- /dev/null
> +++ b/test/sql/gh-4745-table-info-assertion.test.lua
> @@ -0,0 +1,5 @@
> +--
> +-- The following pragma shouldn't cause
> +-- assertion fault
> +--
I'd add a bit of details:
-- Make sure that 'pragma table_info()' correctly handles tables
-- without primary key.
> +table_info = box.execute('pragma table_info("_vinyl_deferred_delete")')
To ignore statement result as a rule we use underscore:
_ = box.execute(...)
Finally, personally I wouldn't focus on particular '_vinyl_deferred_delete'
system space, but rather create casual space without pk and/or format and
use it in the test. Then we could run it using both engines.
> --
> 2.21.1 (Apple Git-122.3)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info
2020-01-30 12:04 ` Nikita Pettik
@ 2020-01-30 12:53 ` Chris Sosnin
0 siblings, 0 replies; 6+ messages in thread
From: Chris Sosnin @ 2020-01-30 12:53 UTC (permalink / raw)
To: Nikita Pettik; +Cc: tarantool-patches
Hi! Thank you for the review!
> I'd add a bit of details:
>
> -- Make sure that 'pragma table_info()' correctly handles tables
> -- without primary key.
I agree, inserted your version.
>
>> +table_info = box.execute('pragma table_info("_vinyl_deferred_delete")')
>
> To ignore statement result as a rule we use underscore:
>
> _ = box.execute(…)
Fixed.
> Finally, personally I wouldn't focus on particular '_vinyl_deferred_delete'
> system space, but rather create casual space without pk and/or format and
> use it in the test. Then we could run it using both engines.
I rewrote tests, new diff:
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+--
+-- Make sure that 'pragma table_info()' correctly handles tables
+-- without primary key.
+--
+T = box.schema.create_space('T', { \
+ engine = engine, \
+ format = {{'i', 'integer'}} \
+})
+_ = box.execute('pragma table_info(T)')
+T:drop()
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info
2020-01-27 14:04 [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info Chris Sosnin
2020-01-28 0:24 ` Vladislav Shpilevoy
2020-01-30 12:04 ` Nikita Pettik
@ 2020-02-05 22:22 ` Vladislav Shpilevoy
2020-02-06 17:59 ` Nikita Pettik
2 siblings, 1 reply; 6+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-05 22:22 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
Hi! Thanks for the patch! LGTM.
On 27/01/2020 15:04, Chris Sosnin wrote:
> We should first check that primary key is not NULL.
>
> Closes #4745
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4745-table-info-assertion
> issue: https://github.com/tarantool/tarantool/issues/4745
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info
2020-02-05 22:22 ` Vladislav Shpilevoy
@ 2020-02-06 17:59 ` Nikita Pettik
0 siblings, 0 replies; 6+ messages in thread
From: Nikita Pettik @ 2020-02-06 17:59 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On 05 Feb 23:22, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! LGTM.
Pushed to master, 2.3 and 2.2
> On 27/01/2020 15:04, Chris Sosnin wrote:
> > We should first check that primary key is not NULL.
> >
> > Closes #4745
> > ---
> > branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4745-table-info-assertion
> > issue: https://github.com/tarantool/tarantool/issues/4745
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-06 17:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-27 14:04 [Tarantool-patches] [PATCH] sql: fix segfault in pragma table_info Chris Sosnin
2020-01-28 0:24 ` Vladislav Shpilevoy
2020-01-30 12:04 ` Nikita Pettik
2020-01-30 12:53 ` Chris Sosnin
2020-02-05 22:22 ` Vladislav Shpilevoy
2020-02-06 17:59 ` Nikita Pettik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox