[Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Apr 3 01:20:32 MSK 2020
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
More information about the Tarantool-patches
mailing list