[Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area

Nikita Pettik korablev at tarantool.org
Thu Apr 2 17:01:00 MSK 2020


On 02 Apr 02:29, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> See 6 comments below.
> 
> > diff --git a/src/box/xrow.c b/src/box/xrow.c
> > index 28adbdc1f..f33226dc5 100644
> > --- a/src/box/xrow.c
> > +++ b/src/box/xrow.c
> > @@ -1070,19 +1083,62 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
> >  	return 0;
> >  }
> >  
> > +static int
> > +iproto_decode_error_stack(const char **pos)
> > +{
> > +	const char *reason = NULL;
> > +	static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\
> > +		      "expected to be larger than error message max length");
> > +	/*
> > +	 * Erase previously set diag errors. It is required since
> > +	 * box_error_add() does not replace previous errors.
> > +	 */
> > +	box_error_clear();
> > +	assert(mp_typeof(**pos) == MP_ARRAY);
> 
> 1. Better keep this check here instead of xrow_decode_error(). It looks
> strange, when iproto_decode_error_stack() validates everything except
> the top header, which is somewhy expected to be validated by the
> caller.

Ok:

diff --git a/src/box/xrow.c b/src/box/xrow.c
index f33226dc5..680f9329a 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1094,7 +1094,8 @@ iproto_decode_error_stack(const char **pos)
         * box_error_add() does not replace previous errors.
         */
        box_error_clear();
-       assert(mp_typeof(**pos) == MP_ARRAY);
+       if (mp_typeof(**pos) != MP_ARRAY)
+               return -1;
        uint32_t stack_sz = mp_decode_array(pos);
        for (uint32_t i = 0; i < stack_sz; i++) {
                uint32_t code = 0;
@@ -1163,8 +1164,6 @@ xrow_decode_error(struct xrow_header *row)
                        snprintf(error, sizeof(error), "%.*s", len, str);
                        box_error_set(__FILE__, __LINE__, code, error);
                } else if (key == IPROTO_ERROR_STACK) {
-                       if (mp_typeof(*pos) != MP_ARRAY)
-                               goto error;
                        if (iproto_decode_error_stack(&pos) != 0)
                                goto error;
                } else {

 
> > +	uint32_t stack_sz = mp_decode_array(pos);
> > +	for (uint32_t i = 0; i < stack_sz; i++) {
> > +		uint32_t code = 0;
> > +		if (mp_typeof(**pos) != MP_MAP)
> > +			return -1;
> > +		uint32_t map_sz = mp_decode_map(pos);
> > +		for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
> > +			if (mp_typeof(**pos) != MP_UINT)
> > +				return -1;
> > +			uint8_t key = mp_decode_uint(pos);
> 
> 2. If I will send value 0xffffff01, it will be considered valid.
> Better save it into uint64_t, since mp_decode_uint() returns this
> type. In that case all will be fine.
> 
> The same with code. Better get the code as uint64_t and compare
> with UINT32_MAX. Because ClientError has uint32_t error code type.

diff --git a/src/box/xrow.c b/src/box/xrow.c
index f33226dc5..323dc8f4d 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1094,17 +1094,18 @@ iproto_decode_error_stack(const char **pos)
        uint32_t stack_sz = mp_decode_array(pos);
        for (uint32_t i = 0; i < stack_sz; i++) {
-               uint32_t code = 0;
+               uint64_t code = 0;
                if (mp_typeof(**pos) != MP_MAP)
                        return -1;
                uint32_t map_sz = mp_decode_map(pos);
                for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
                        if (mp_typeof(**pos) != MP_UINT)
                                return -1;
-                       uint8_t key = mp_decode_uint(pos);
+                       uint32_t key = mp_decode_uint(pos);
                        if (key == IPROTO_ERROR_CODE) {
                                if (mp_typeof(**pos) != MP_UINT)
                                        return -1;


> >  void
> >  xrow_decode_error(struct xrow_header *row)
> >  {
> >  	uint32_t code = row->type & (IPROTO_TYPE_ERROR - 1);
> >  
> >  	char error[DIAG_ERRMSG_MAX] = { 0 };
> > -	const char *pos;
> > +	const char *pos, *end;
> >  	uint32_t map_size;
> >  
> >  	if (row->bodycnt == 0)
> >  		goto error;
> >  	pos = (char *) row->body[0].iov_base;
> > -	if (mp_check(&pos, pos + row->body[0].iov_len))
> > +	end = pos + row->body[0].iov_len;
> > +	if (mp_check(&pos, end))
> >  		goto error;
> 
> 3. This hunk is clearly an artifact from the old patch
> version. Dropped it and nothing changed.

Yes, sorry. Reverted change:

@@ -1131,14 +1132,13 @@ xrow_decode_error(struct xrow_header *row)
        uint32_t code = row->type & (IPROTO_TYPE_ERROR - 1);
 
        char error[DIAG_ERRMSG_MAX] = { 0 };
-       const char *pos, *end;
+       const char *pos;
        uint32_t map_size;
 
        if (row->bodycnt == 0)
                goto error;
        pos = (char *) row->body[0].iov_base;
-       end = pos + row->body[0].iov_len;
-       if (mp_check(&pos, end))
+       if (mp_check(&pos, pos + row->body[0].iov_len))
                goto error;
 
        pos = (char *) row->body[0].iov_base;

> > diff --git a/test/box/iproto.result b/test/box/iproto.result
> > new file mode 100644
> > index 000000000..b6dc7ed4c
> > --- /dev/null
> > +++ b/test/box/iproto.result
> > @@ -0,0 +1,142 @@
> > +---
> > +...
> > +-- gh-1148: test capabilities of stacked diagnostics bypassing net.box.
> > +--
> > +test_run:cmd("setopt delimiter ';'")
> > +---
> > +- true
> > +...
> > +stack_err = function()
> > +    local e1 = box.error.new({code = 111, reason = "e1"})
> > +    local e2 = box.error.new({code = 111, reason = "e2"})
> > +    local e3 = box.error.new({code = 111, reason = "e3"})
> > +    assert(e1 ~= nil)
> > +    e2:set_prev(e1)
> > +    assert(e2.prev == e1)
> > +    e3:set_prev(e2)
> > +    box.error(e3)
> > +end;
> > +---
> > +...
> > +test_run:cmd("setopt delimiter ''");
> > +---
> > +- true
> > +...
> > +box.schema.user.grant('guest', 'read, write, execute', 'universe')
> 
> 4. Useful command, easy to remember:
> 
>     box.schema.user.grant/revoke('guest', 'super')
> 
> Is shorter, and has basically the same effect + a few more permissions.
> In case you like me look for this 'grant' command in other test files to
> copy paste it every time. Up to you what to use. JFYI.

Thx, will keep it in mind.
 
> > diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
> > index 68a334239..954f22f16 100644
> > --- a/test/unit/xrow.cc
> > +++ b/test/unit/xrow.cc
> 
> 5. This test crashes on travis and locally on my machine. Probably
> a typo somewhere.

Yep, accidentally forgot to update result file. Fixed.

> I also added a test for the uint64 vs uint8 result of mp_decode_uint().
> See it below and pushed on top of the branch as a separate commit.

Nice, thanks, squashed with some changes:

@@ -372,6 +375,26 @@ test_xrow_error_stack_decode()
        isnt(last, NULL, "xrow_decode succeed: diag has been set");
        is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
           "stack's map wrong value type");
+
+       /* Bad key in the packet. */
+       pos = mp_encode_map(buffer, 1);
+       pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
+       pos = mp_encode_array(pos, 1);
+       pos = mp_encode_map(pos, 2);
+       pos = mp_encode_uint(pos, 0xff000000 | IPROTO_ERROR_CODE);
+       pos = mp_encode_uint(pos, 159);
+       pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
+       pos = mp_encode_str(pos, "test msg", strlen("test msg"));
+       header.body[0].iov_base = buffer;
+       header.body[0].iov_len = pos - buffer;
+
+       diag_clear(diag_get());
+       xrow_decode_error(&header);
+       last = diag_last_error(diag_get());
+       isnt(last, NULL, "xrow_decode succeed: diag has been set");
+       is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode corrupted stack: "
+          "stack's map wrong key");
+
        check_plan();
 }

Diag won't be empty since error will be set anyway - with default
(i.e. wrong) error code (0), but correct message.

I've also found one more bug: box_error_add() was called in the wrong
place. As a result, number of errors in stack might be doubled:

@@ -1119,8 +1120,8 @@ iproto_decode_error_stack(const char **pos)
                                mp_next(pos);
                                continue;
                        }
-                       box_error_add(__FILE__, __LINE__, code, reason);
                }
+               box_error_add(__FILE__, __LINE__, code, reason);
        }
        return 0;
 }

Added another one test case covering this change:

@@ -295,12 +295,15 @@ test_xrow_error_stack_decode()
        isnt(last, NULL, "xrow_decode succeed: 'cause' is present in stack")
        is(strcmp(last->errmsg, "e1"), 0, "xrow_decode succeed: "
           "stack has been parsed");
+       last = last->cause;
+       is(last, NULL, "xrow_decode succeed: only two errors in the stack")
        /*
         * Let's try decode broken stack. Variations:
 
> ====================
> diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
> index 954f22f16..1449d49a5 100644
> --- a/test/unit/xrow.cc
> +++ b/test/unit/xrow.cc
> @@ -255,7 +255,7 @@ error_stack_entry_encode(char *pos, const char *err_str)
>  void
>  test_xrow_error_stack_decode()
>  {
> -	plan(14);
> +	plan(15);
>  	char buffer[2048];
>  	/*
>  	 * To start with, let's test the simplest and obsolete
> @@ -372,6 +372,24 @@ test_xrow_error_stack_decode()
>  	isnt(last, NULL, "xrow_decode succeed: diag has been set");
>  	is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
>  	   "stack's map wrong value type");
> +
> +	/* Bad key in the packet. */
> +	diag_clear(diag_get());
> +	pos = mp_encode_map(buffer, 1);
> +	pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
> +	pos = mp_encode_array(pos, 1);
> +	pos = mp_encode_map(pos, 2);
> +	pos = mp_encode_uint(pos, 0xff000000 | IPROTO_ERROR_CODE);
> +	pos = mp_encode_uint(pos, 159);
> +	pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
> +	pos = mp_encode_str(pos, "", 0);
> +	header.body[0].iov_base = buffer;
> +	header.body[0].iov_len = pos - buffer;
> +	xrow_decode_error(&header);
> +	ok(diag_is_empty(diag_get()),
> +	   "xrow_decode failed: bad error object key");
> +	last = diag_last_error(diag_get());
> +
>  	check_plan();
>  }
>  ====================
> 
> On 02/04/2020 00:24, Nikita Pettik wrote:
> > On 01 Apr 16:26, Nikita Pettik wrote:
> >> @@ -1169,7 +1166,7 @@ xrow_decode_error(struct xrow_header *row)
> >>  		} else if (key == IPROTO_ERROR_STACK ) {
> >>  			if (mp_check_array(pos, end) != 0)
> >>  				goto error;
> >> -			if (iproto_decode_error_stack(&pos, end) != 0)
> >> +			if (iproto_decode_error_stack(&pos) != 0)
> >>  				continue;
> >>  		} else {
> >>  			mp_next(&pos);
> >>
> >>> I don't see any tests on that. Are there some? Maybe at least
> >>> unit tests, in xrow.cc? Although a proper test would try to
> >>> send an error stack through applier. Because xrow_decode_error()
> >>> is used only there.
> >>
> >> I'm a bit new to replication tests. I've been working on it.
> >> Could you please review the rest of changes so far?
> 
> 6. I tried to find how to send a stacked error to applier. This
> should be done from relay. And seems like it is impossible to do
> from relay. Simply because there are no any user defined code to
> create stacked errors, and because internals of relay don't use
> stacked errors so far. Probably an error injection could help to
> create an artificial error, but I think the unit test should be
> enough.


More information about the Tarantool-patches mailing list