[Tarantool-patches] [PATCH v1 1/1] sql: fix assert when MP_EXT received via netbox

Mergen Imeev imeevma at tarantool.org
Mon Mar 14 19:17:43 MSK 2022


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 at 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.



More information about the Tarantool-patches mailing list