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

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

Thanks for the fixes!

See 2 comments below.

> diff --git a/src/box/error.cc b/src/box/error.cc
> index 8e77c2e9e..897aa9261 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -102,6 +102,23 @@ box_error_new(const char *file, unsigned line, uint32_t code,
>  	return e;
>  }
>  
> +int
> +box_error_add(const char *file, unsigned line, uint32_t code,
> +	      const char *fmt, ...)
> +{
> +	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> +	ClientError *client_error = type_cast(ClientError, e);
> +	if (client_error) {
> +		client_error->m_errcode = code;
> +		va_list ap;
> +		va_start(ap, fmt);
> +		error_vformat_msg(e, fmt, ap);
> +		va_end(ap);
> +	}
> +	diag_add_error(&fiber()->diag, e);
> +	return -1;

1. Why do we return -1 instead of the new error object?
box_error_new() returns an error, and this function does not.
Seems inconsistent.

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

2. I added box_error_code() check to ensure this.

But more importantly that the original bug I was referring to
still is here. About overflows and integer truncation. I fixed it
and added tests. See them below and on the branch in a separate
commit.

====================
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 9d30bcaf9..be026a43c 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1105,11 +1105,13 @@ iproto_decode_error_stack(const char **pos)
 		for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
 			if (mp_typeof(**pos) != MP_UINT)
 				return -1;
-			uint32_t key = mp_decode_uint(pos);
+			uint64_t key = mp_decode_uint(pos);
 			if (key == IPROTO_ERROR_CODE) {
 				if (mp_typeof(**pos) != MP_UINT)
 					return -1;
 				code = mp_decode_uint(pos);
+				if (code > UINT32_MAX)
+					return -1;
 			} else if (key == IPROTO_ERROR_MESSAGE) {
 				if (mp_typeof(**pos) != MP_STR)
 					return -1;
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index ae45f18b0..718ef15a2 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -32,6 +32,7 @@ extern "C" {
 #include "unit.h"
 } /* extern "C" */
 #include "trivia/util.h"
+#include "box/error.h"
 #include "box/xrow.h"
 #include "box/iproto_constants.h"
 #include "uuid/tt_uuid.h"
@@ -255,7 +256,7 @@ error_stack_entry_encode(char *pos, const char *err_str)
 void
 test_xrow_error_stack_decode()
 {
-	plan(17);
+	plan(24);
 	char buffer[2048];
 	/*
 	 * To start with, let's test the simplest and obsolete
@@ -392,9 +393,51 @@ test_xrow_error_stack_decode()
 	xrow_decode_error(&header);
 	last = diag_last_error(diag_get());
 	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(box_error_code(last), 0, "xrow_decode last error code is default 0");
 	is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode corrupted stack: "
 	   "stack's map wrong key");
 
+	/* Overflow error code. */
+	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, IPROTO_ERROR_CODE);
+	pos = mp_encode_uint(pos, (uint64_t)1 << 40);
+	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(box_error_code(last), 159, "xrow_decode failed, took code from "
+	   "header");
+	is(strcmp(last->errmsg, ""), 0, "xrow_decode failed, message is not "
+	   "decoded");
+
+	/* Overflow error key. */
+	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, 0xff00000000 | 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(box_error_code(last), 0, "xrow_decode last error code is default 0");
+	is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode message is fine "
+	   "even without the code");
+
 	check_plan();
 }
 
diff --git a/test/unit/xrow.result b/test/unit/xrow.result
index d24e9ea4f..7213ab6c7 100644
--- a/test/unit/xrow.result
+++ b/test/unit/xrow.result
@@ -53,7 +53,7 @@ ok 1 - subtests
     ok 9 - decoded sync
     ok 10 - decoded bodycnt
 ok 2 - subtests
-    1..17
+    1..24
     ok 1 - xrow_decode succeed: diag has been set
     ok 2 - xrow_decode succeed: error is parsed
     ok 3 - xrow_decode succeed: diag has been set
@@ -70,7 +70,14 @@ ok 2 - subtests
     ok 14 - xrow_decode succeed: diag has been set
     ok 15 - xrow_decode corrupted stack: stack's map wrong value type
     ok 16 - xrow_decode succeed: diag has been set
-    ok 17 - xrow_decode corrupted stack: stack's map wrong key
+    ok 17 - xrow_decode last error code is default 0
+    ok 18 - xrow_decode corrupted stack: stack's map wrong key
+    ok 19 - xrow_decode succeed: diag has been set
+    ok 20 - xrow_decode failed, took code from header
+    ok 21 - xrow_decode failed, message is not decoded
+    ok 22 - xrow_decode succeed: diag has been set
+    ok 23 - xrow_decode last error code is default 0
+    ok 24 - xrow_decode message is fine even without the code
 ok 3 - subtests
     1..1
     ok 1 - request_str