* [Tarantool-patches] [PATCH v1 1/1] sql: fix assert when MP_EXT received via netbox
@ 2022-01-13 9:37 Mergen Imeev via Tarantool-patches
2022-01-14 22:36 ` Vladislav Shpilevoy via Tarantool-patches
0 siblings, 1 reply; 3+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2022-01-13 9:37 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
This patch fixes an assertion or segmentation fault when getting the
value of MP_EXT via netbox.
Closes #6766
---
https://github.com/tarantool/tarantool/issues/6766
https://github.com/tarantool/tarantool/tree/imeevma/gh-6766-fix-assert-when-decoding-mp-ext
src/box/bind.c | 30 ++++++++++++++++++-
test/sql-tap/engine.cfg | 1 +
.../gh-6766-fix-bind-for-mp-ext.test.lua | 30 +++++++++++++++++++
3 files changed, 60 insertions(+), 1 deletion(-)
create mode 100755 test/sql-tap/gh-6766-fix-bind-for-mp-ext.test.lua
diff --git a/src/box/bind.c b/src/box/bind.c
index 441c9f46f..6672d1271 100644
--- a/src/box/bind.c
+++ b/src/box/bind.c
@@ -34,6 +34,8 @@
#include "sql/sqlInt.h"
#include "sql/sqlLimit.h"
#include "sql/vdbe.h"
+#include "mp_decimal.h"
+#include "mp_uuid.h"
const char *
sql_bind_name(const struct sql_bind *bind)
@@ -99,9 +101,35 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
case MP_BIN:
bind->s = mp_decode_bin(packet, &bind->bytes);
break;
+ case MP_EXT: {
+ int8_t ext_type;
+ const char *svp = *packet;
+ uint32_t size = mp_decode_extl(packet, &ext_type);
+ if (ext_type != MP_UUID && ext_type != MP_DECIMAL) {
+ bind->s = svp;
+ *packet += size;
+ bind->bytes = *packet - svp;
+ break;
+ }
+ *packet = svp;
+ if (ext_type == MP_UUID) {
+ if (mp_decode_uuid(packet, &bind->uuid) == NULL) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_UUID MsgPack format");
+ return -1;
+ }
+ } else {
+ if (mp_decode_decimal(packet, &bind->dec) == NULL) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_DECIMAL MsgPack format");
+ return -1;
+ }
+ }
+ bind->ext_type = ext_type;
+ break;
+ }
case MP_ARRAY:
case MP_MAP:
- case MP_EXT:
bind->s = *packet;
mp_next(packet);
bind->bytes = *packet - bind->s;
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 528212ab6..3bd416dd5 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -41,6 +41,7 @@
"gh-6375-assert-on-unsupported-ext.test.lua": {},
"gh-6485-bugs-in-decimal.test.lua": {},
"gh-6113-assert-in-hex-on-zeroblob.test.lua": {},
+ "gh-6766-fix-bind-for-mp-ext.test.lua": {},
"*": {
"memtx": {"engine": "memtx"},
"vinyl": {"engine": "vinyl"}
diff --git a/test/sql-tap/gh-6766-fix-bind-for-mp-ext.test.lua b/test/sql-tap/gh-6766-fix-bind-for-mp-ext.test.lua
new file mode 100755
index 000000000..8190917fd
--- /dev/null
+++ b/test/sql-tap/gh-6766-fix-bind-for-mp-ext.test.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+local test = require("sqltester")
+test:plan(2)
+
+box.cfg{listen = os.getenv('LISTEN')}
+local cn = require('net.box').connect(box.cfg.listen)
+
+test:do_test(
+ "gh-6766-1",
+ function()
+ local val = {require('decimal').new(1.5)}
+ local res = cn:execute([[SELECT typeof(?);]], val)
+ return {res.rows[1][1]}
+ end, {
+ 'decimal'
+ })
+
+test:do_test(
+ "gh-6766-2",
+ function()
+ local val = {require('uuid').new()}
+ local res = cn:execute([[SELECT typeof(?);]], val)
+ return {res.rows[1][1]}
+ end, {
+ 'uuid'
+ })
+
+cn:close()
+
+test:finish_test()
--
2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix assert when MP_EXT received via netbox
2022-01-13 9:37 [Tarantool-patches] [PATCH v1 1/1] sql: fix assert when MP_EXT received via netbox Mergen Imeev via Tarantool-patches
@ 2022-01-14 22:36 ` Vladislav Shpilevoy via Tarantool-patches
2022-03-14 16:17 ` Mergen Imeev via Tarantool-patches
0 siblings, 1 reply; 3+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2022-01-14 22:36 UTC (permalink / raw)
To: imeevma; +Cc: tarantool-patches
Hi! Thanks for the patch!
See 4 comments below.
On 13.01.2022 10:37, imeevma@tarantool.org wrote:
> This patch fixes an assertion or segmentation fault when getting the
> value of MP_EXT via netbox.
>
> Closes #6766
> ---
> https://github.com/tarantool/tarantool/issues/6766
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6766-fix-assert-when-decoding-mp-ext
>
> src/box/bind.c | 30 ++++++++++++++++++-
> test/sql-tap/engine.cfg | 1 +
> .../gh-6766-fix-bind-for-mp-ext.test.lua | 30 +++++++++++++++++++
1. Lets also add a changelog file.
> 3 files changed, 60 insertions(+), 1 deletion(-)
> create mode 100755 test/sql-tap/gh-6766-fix-bind-for-mp-ext.test.lua
>
> diff --git a/src/box/bind.c b/src/box/bind.c
> index 441c9f46f..6672d1271 100644
> --- a/src/box/bind.c
> +++ b/src/box/bind.c
> @@ -99,9 +101,35 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
> case MP_BIN:
> bind->s = mp_decode_bin(packet, &bind->bytes);
> break;
> + case MP_EXT: {
> + int8_t ext_type;
> + const char *svp = *packet;
> + uint32_t size = mp_decode_extl(packet, &ext_type);
> + if (ext_type != MP_UUID && ext_type != MP_DECIMAL) {
> + bind->s = svp;
> + *packet += size;
> + bind->bytes = *packet - svp;
> + break;
2. It might be better to move this below into 'else' branch after all
the type checks.
> + }
> + *packet = svp;
> + if (ext_type == MP_UUID) {
> + if (mp_decode_uuid(packet, &bind->uuid) == NULL) {
3. You already decoded the ext header. So you can call uuid_unpack().
Same below with decimal_unpack().
4. It still might be good to handle unknown MP_EXT values in sql_bind_column
or before it. Otherwise you will get a crash on any of them:
netbox = require('net.box')
msgpack = require('msgpack')
box.cfg{listen = 3313}
box.schema.user.grant('guest', 'super')
str = '\xc7\x00\x0f'
val = msgpack.object_from_raw(str)
con = netbox.connect(box.cfg.listen)
con:execute([[SELECT typeof(?);]], {val})
Assertion failed: (p->ext_type == MP_UUID || p->ext_type == MP_DECIMAL),
function sql_bind_column, file
/Users/gerold/Work/Repositories/tarantool/src/box/bind.c, line 220.
Here I created a value of extension 15, which we don't support.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix assert when MP_EXT received via netbox
2022-01-14 22:36 ` Vladislav Shpilevoy via Tarantool-patches
@ 2022-03-14 16:17 ` Mergen Imeev via Tarantool-patches
0 siblings, 0 replies; 3+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2022-03-14 16:17 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hi! Thanks for the review! And sorry for such a late reply. I rewrote this patch
and created a PR instead of pasting the changes here:
https://github.com/tarantool/tarantool/pull/6922
On Fri, Jan 14, 2022 at 11:36:34PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> See 4 comments below.
>
> On 13.01.2022 10:37, imeevma@tarantool.org wrote:
> > This patch fixes an assertion or segmentation fault when getting the
> > value of MP_EXT via netbox.
> >
> > Closes #6766
> > ---
> > https://github.com/tarantool/tarantool/issues/6766
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-6766-fix-assert-when-decoding-mp-ext
> >
> > src/box/bind.c | 30 ++++++++++++++++++-
> > test/sql-tap/engine.cfg | 1 +
> > .../gh-6766-fix-bind-for-mp-ext.test.lua | 30 +++++++++++++++++++
>
> 1. Lets also add a changelog file.
>
Added.
> > 3 files changed, 60 insertions(+), 1 deletion(-)
> > create mode 100755 test/sql-tap/gh-6766-fix-bind-for-mp-ext.test.lua
> >
> > diff --git a/src/box/bind.c b/src/box/bind.c
> > index 441c9f46f..6672d1271 100644
> > --- a/src/box/bind.c
> > +++ b/src/box/bind.c
> > @@ -99,9 +101,35 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet)
> > case MP_BIN:
> > bind->s = mp_decode_bin(packet, &bind->bytes);
> > break;
> > + case MP_EXT: {
> > + int8_t ext_type;
> > + const char *svp = *packet;
> > + uint32_t size = mp_decode_extl(packet, &ext_type);
> > + if (ext_type != MP_UUID && ext_type != MP_DECIMAL) {
> > + bind->s = svp;
> > + *packet += size;
> > + bind->bytes = *packet - svp;
> > + break;
>
> 2. It might be better to move this below into 'else' branch after all
> the type checks.
>
True. However, this part is actually wrong, we should throw an error here.
Fixed.
> > + }
> > + *packet = svp;
> > + if (ext_type == MP_UUID) {
> > + if (mp_decode_uuid(packet, &bind->uuid) == NULL) {
>
> 3. You already decoded the ext header. So you can call uuid_unpack().
> Same below with decimal_unpack().
>
Thanks, fixed.
> 4. It still might be good to handle unknown MP_EXT values in sql_bind_column
> or before it. Otherwise you will get a crash on any of them:
>
> netbox = require('net.box')
> msgpack = require('msgpack')
> box.cfg{listen = 3313}
> box.schema.user.grant('guest', 'super')
>
> str = '\xc7\x00\x0f'
> val = msgpack.object_from_raw(str)
> con = netbox.connect(box.cfg.listen)
> con:execute([[SELECT typeof(?);]], {val})
>
> Assertion failed: (p->ext_type == MP_UUID || p->ext_type == MP_DECIMAL),
> function sql_bind_column, file
> /Users/gerold/Work/Repositories/tarantool/src/box/bind.c, line 220.
>
> Here I created a value of extension 15, which we don't support.
Thank you the test! This problem was fixed by fixing 2. I added this test.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-14 16:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 9:37 [Tarantool-patches] [PATCH v1 1/1] sql: fix assert when MP_EXT received via netbox Mergen Imeev via Tarantool-patches
2022-01-14 22:36 ` Vladislav Shpilevoy via Tarantool-patches
2022-03-14 16:17 ` Mergen Imeev via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox