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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

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

> +			if (key == IPROTO_ERROR_CODE) {
> +				if (mp_typeof(**pos) != MP_UINT)
> +					return -1;
> +				code = mp_decode_uint(pos);
> +			} else if (key == IPROTO_ERROR_MESSAGE) {
> +				if (mp_typeof(**pos) != MP_STR)
> +					return -1;
> +				uint32_t len;
> +				const char *str = mp_decode_str(pos, &len);
> +				reason = tt_cstr(str, len);
> +			} else {
> +				mp_next(pos);
> +				continue;
> +			}
> +			box_error_add(__FILE__, __LINE__, code, reason);
> +		}
> +	}
> +	return 0;
> +}
> +
>  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.

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

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

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.

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